| Message ID | f2c32557-c973-5ac1-e78c-d41f675814a2@suse.cz |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 31, 2016 at 3:56 PM, Martin Liška <mliska@suse.cz> wrote: > On 10/31/2016 12:11 PM, Richard Biener wrote: >> On Mon, Oct 31, 2016 at 11:18 AM, Richard Sandiford >> <richard.sandiford@arm.com> wrote: >>> Richard Biener <richard.guenther@gmail.com> writes: >>>> On Mon, Oct 31, 2016 at 10:58 AM, Richard Sandiford >>>> <richard.sandiford@arm.com> wrote: >>>>> Richard Biener <richard.guenther@gmail.com> writes: >>>>>> On Mon, Oct 31, 2016 at 10:10 AM, Martin Liška <mliska@suse.cz> wrote: >>>>>>> On 10/31/2016 01:12 AM, Richard Sandiford wrote: >>>>>>>> Richard Biener <richard.guenther@gmail.com> writes: >>>>>>>>> On Thu, Oct 27, 2016 at 5:06 PM, Martin Liška <mliska@suse.cz> wrote: >>>>>>>>>> On 10/27/2016 03:35 PM, Richard Biener wrote: >>>>>>>>>>> On Thu, Oct 27, 2016 at 9:41 AM, Martin Liška <mliska@suse.cz> wrote: >>>>>>>>>>>> Running simple test-case w/o the proper header file causes ICE: >>>>>>>>>>>> strncmp ("a", "b", -1); >>>>>>>>>>>> >>>>>>>>>>>> 0xe74462 tree_to_uhwi(tree_node const*) >>>>>>>>>>>> ../../gcc/tree.c:7324 >>>>>>>>>>>> 0x90a23f host_size_t_cst_p >>>>>>>>>>>> ../../gcc/fold-const-call.c:63 >>>>>>>>>>>> 0x90a23f fold_const_call(combined_fn, tree_node*, tree_node*, >>>>>>>>>>>> tree_node*, tree_node*) >>>>>>>>>>>> ../../gcc/fold-const-call.c:1512 >>>>>>>>>>>> 0x787b01 fold_builtin_3 >>>>>>>>>>>> ../../gcc/builtins.c:8385 >>>>>>>>>>>> 0x787b01 fold_builtin_n(unsigned int, tree_node*, tree_node**, >>>>>>>>>>>> int, bool) >>>>>>>>>>>> ../../gcc/builtins.c:8465 >>>>>>>>>>>> 0x9052b1 fold(tree_node*) >>>>>>>>>>>> ../../gcc/fold-const.c:11919 >>>>>>>>>>>> 0x6de2bb c_fully_fold_internal >>>>>>>>>>>> ../../gcc/c/c-fold.c:185 >>>>>>>>>>>> 0x6e1f6b c_fully_fold(tree_node*, bool, bool*) >>>>>>>>>>>> ../../gcc/c/c-fold.c:90 >>>>>>>>>>>> 0x67cbbf c_process_expr_stmt(unsigned int, tree_node*) >>>>>>>>>>>> ../../gcc/c/c-typeck.c:10369 >>>>>>>>>>>> 0x67cfbd c_finish_expr_stmt(unsigned int, tree_node*) >>>>>>>>>>>> ../../gcc/c/c-typeck.c:10414 >>>>>>>>>>>> 0x6cb578 c_parser_statement_after_labels >>>>>>>>>>>> ../../gcc/c/c-parser.c:5430 >>>>>>>>>>>> 0x6cd333 c_parser_compound_statement_nostart >>>>>>>>>>>> ../../gcc/c/c-parser.c:4944 >>>>>>>>>>>> 0x6cdbde c_parser_compound_statement >>>>>>>>>>>> ../../gcc/c/c-parser.c:4777 >>>>>>>>>>>> 0x6c93ac c_parser_declaration_or_fndef >>>>>>>>>>>> ../../gcc/c/c-parser.c:2176 >>>>>>>>>>>> 0x6d51ab c_parser_external_declaration >>>>>>>>>>>> ../../gcc/c/c-parser.c:1574 >>>>>>>>>>>> 0x6d5c09 c_parser_translation_unit >>>>>>>>>>>> ../../gcc/c/c-parser.c:1454 >>>>>>>>>>>> 0x6d5c09 c_parse_file() >>>>>>>>>>>> ../../gcc/c/c-parser.c:18173 >>>>>>>>>>>> 0x72ffd2 c_common_parse_file() >>>>>>>>>>>> ../../gcc/c-family/c-opts.c:1087 >>>>>>>>>>>> >>>>>>>>>>>> Following patch improves the host_size_t_cst_p predicate. >>>>>>>>>>>> >>>>>>>>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives >>>>>>>>>>>> regression tests. >>>>>>>>>>>> >>>>>>>>>>>> Ready to be installed? >>>>>>>>>>> >>>>>>>>>>> I believe the wi::min_precision (t, UNSIGNED) <= sizeof (size_t) * >>>>>>>>>>> CHAR_BIT test is now redundant. >>>>>>>>>>> >>>>>>>>>>> OTOH it was probably desired to allow -1 here? A little looking back >>>>>>>>>>> in time should tell. >>>>>>>>>> >>>>>>>>>> Ok, it started with r229922, where it was changed from: >>>>>>>>>> >>>>>>>>>> if (tree_fits_uhwi_p (len) && p1 && p2) >>>>>>>>>> { >>>>>>>>>> const int i = strncmp (p1, p2, tree_to_uhwi (len)); >>>>>>>>>> ... >>>>>>>>>> >>>>>>>>>> to current version: >>>>>>>>>> >>>>>>>>>> case CFN_BUILT_IN_STRNCMP: >>>>>>>>>> { >>>>>>>>>> bool const_size_p = host_size_t_cst_p (arg2, &s2); >>>>>>>>>> >>>>>>>>>> Thus I'm suggesting to change to back to it. >>>>>>>>>> >>>>>>>>>> Ready to be installed? >>>>>>>>> >>>>>>>>> Let's ask Richard. >>>>>>>> >>>>>>>> The idea with the: >>>>>>>> >>>>>>>> wi::min_precision (t, UNSIGNED) <= sizeof (size_t) * CHAR_BIT >>>>>>>> >>>>>>>> test was to stop us attempting 64-bit size_t operations on ILP32 hosts. >>>>>>>> I think we still want that. >>>>>>> >>>>>>> OK, so is the consensus to add tree_fits_uhwi_p predicate to the current >>>>>>> wi::min_precision check, right? >>>>>> >>>>>> Not sure. If we have host_size_t_cst_p then we should have a corresponding >>>>>> size_t host_size_t (const_tree) and should use those in pairs. Not sure >>>>>> why we have sth satisfying host_size_t_cst_p but not tree_fits_uhwi_p. >>>>> >>>>> It's the other way around: something can satisfy tree_fits_uhwi_p >>>>> (i.e. fit within a uint64_t) but not fit within the host's size_t. >>>>> The kind of case I'm thinking of is: >>>>> >>>>> strncmp ("fi", "fo", (1L << 32) + 1) >>>>> >>>>> for an ILP32 host and LP64 target. There's a danger that by passing >>>>> the uint64_t value (1L << 32) + 1 to the host's strncmp that we'd >>>>> truncate it to 1, giving the wrong result. >>>> >>>> Yes, but if it passes host_size_t_cst_p why does tree_to_uhwi ICE then? >>>> (unless we have a > 64bit host size_t). >>> >>> Because in Martin's test case the length has a signed type. >>> tree_to_uhwi then treats the argument as -1 to infinite precision >>> rather than ~(size_t) 0 to infinite precision. >> >> Indeed. So the bug is kind-of in the caller then. OTOH we could simply >> re-interpret the input as target size_t before doing the range check / >> conversion. >> >> I believe fold_const_call has the general issue of not verifying argument types >> before recognizing things as BUILT_IN_* (or the FE is at fault - but that's an >> old discussion...) >> >> Richard. > > Updated and tested version of the patch that add types_compatible_p check > to host_size_t_cst_p. > > Ready to be installed? Ok. Richard. > Martin > >> >>> Thanks, >>> Richard >
From 0b0b36dc062fd0e9ffc7432d9d17bf4c6499b879 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 26 Oct 2016 13:48:39 +0200 Subject: [PATCH] Fix host_size_t_cst_p predicate gcc/ChangeLog: 2016-10-26 Martin Liska <mliska@suse.cz> * fold-const-call.c (host_size_t_cst_p): Test whether t is convertible to size_t. gcc/testsuite/ChangeLog: 2016-10-26 Martin Liska <mliska@suse.cz> * gcc.dg/tree-ssa/builtins-folding-gimple-ub.c (main): Add test case. --- gcc/fold-const-call.c | 4 +++- gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/gcc/fold-const-call.c b/gcc/fold-const-call.c index 05a15f9..1b3a755 100644 --- a/gcc/fold-const-call.c +++ b/gcc/fold-const-call.c @@ -29,6 +29,7 @@ along with GCC; see the file COPYING3. If not see #include "case-cfn-macros.h" #include "tm.h" /* For C[LT]Z_DEFINED_AT_ZERO. */ #include "builtins.h" +#include "gimple-expr.h" /* Functions that test for certain constant types, abstracting away the decision about whether to check for overflow. */ @@ -57,7 +58,8 @@ complex_cst_p (tree t) static inline bool host_size_t_cst_p (tree t, size_t *size_out) { - if (integer_cst_p (t) + if (types_compatible_p (size_type_node, TREE_TYPE (t)) + && integer_cst_p (t) && wi::min_precision (t, UNSIGNED) <= sizeof (size_t) * CHAR_BIT) { *size_out = tree_to_uhwi (t); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c index df0ede2..e1658d1 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c @@ -17,6 +17,10 @@ main (void) if (__builtin_memchr (foo1, 'x', 1000)) /* Not folded away. */ __builtin_abort (); + /* STRNCMP. */ + if (strncmp ("a", "b", -1)) /* { dg-warning "implicit declaration of function" } */ + __builtin_abort (); + return 0; } -- 2.10.1