| Message ID | b38d122f-3325-c51c-2c3d-0f064304a200@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
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. Richard. > Thanks, > Martin > >> >> Richard. >> >>> Martin >
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. Thanks, Richard
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? Thanks, Martin > > Thanks, > Richard >
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. Is that wi::min_precision fault? The way it is documented suggests that it should be equal to tree_fits_uwhi_p ... Richard. > Thanks, > Martin > >> >> Thanks, >> Richard >> >
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. Thanks, Richard
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). Richard. > > Thanks, > Richard
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. Thanks, Richard
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. > Thanks, > Richard
From 608ed3ac6b743846e9bce62cd0b0e83e2b018684 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 predicat gcc/ChangeLog: 2016-10-26 Martin Liska <mliska@suse.cz> * fold-const-call.c (host_size_t_cst_p): Test whether it can fit to uhwi. 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 | 3 +-- gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/gcc/fold-const-call.c b/gcc/fold-const-call.c index 05a15f9..b8154c8 100644 --- a/gcc/fold-const-call.c +++ b/gcc/fold-const-call.c @@ -57,8 +57,7 @@ complex_cst_p (tree t) static inline bool host_size_t_cst_p (tree t, size_t *size_out) { - if (integer_cst_p (t) - && wi::min_precision (t, UNSIGNED) <= sizeof (size_t) * CHAR_BIT) + if (tree_fits_uhwi_p (t)) { *size_out = tree_to_uhwi (t); return true; 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