On Mon, Oct 31, 2016 at 3:56 PM, Martin Liška <mli...@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.sandif...@arm.com> wrote: >>> Richard Biener <richard.guent...@gmail.com> writes: >>>> On Mon, Oct 31, 2016 at 10:58 AM, Richard Sandiford >>>> <richard.sandif...@arm.com> wrote: >>>>> Richard Biener <richard.guent...@gmail.com> writes: >>>>>> On Mon, Oct 31, 2016 at 10:10 AM, Martin Liška <mli...@suse.cz> wrote: >>>>>>> On 10/31/2016 01:12 AM, Richard Sandiford wrote: >>>>>>>> Richard Biener <richard.guent...@gmail.com> writes: >>>>>>>>> On Thu, Oct 27, 2016 at 5:06 PM, Martin Liška <mli...@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 <mli...@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 >