| Message ID | 20151127185531.GA28072@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 27, 2015 at 07:55:43PM +0100, Marek Polacek wrote: > +/* The C version of the save_expr langhook. Either call save_expr or c_save_expr, > + depending on IN_LATE_PROCESSING. */ Consider this too long line fixed. Marek
On Fri, 27 Nov 2015, Marek Polacek wrote: > I didn't know where to put setting of in_late_processing. With the current > placement, we won't (for valid programs) call c_save_expr from c_genericize > or c_gimplify_expr. Well, the placement in this patch (in c_parser_compound_statement) is certainly wrong. It doesn't even save and restore, so after one compound statement inside another, parsing would continue with in_late_processing wrongly set. And c_save_expr is logically right for any parsing outside compound statements as well (arbitrary expressions can occur in sizeof outside functions and in VLA parameter sizes and should follow the normal rules for what's a constant expression - there's a known bug that statement expressions are wrongly rejected in such contexts). Starting from first principles: parsing takes place from within c_parse_file as the sole external entry point to the parser. So you could have a parsing_input variable that starts off as false, and where c_parse_file saves it, sets to true, and restores the saved value at the end. Then you'd use c_save_expr if parsing_input && !in_late_binary_op. If that doesn't work, it means there are cases where the hook gets called from folding that takes place during parsing, on expressions that will not subsequently go through c_fully_fold, but without in_late_binary_op set. Knowing what those cases are might help work out any fix for them that is needed. > I suppose I should also modify save_expr in fold-const.c to call it via the > langhook, if this approach is sane. Dunno. That's a complication. When the folding is taking place from within c_fully_fold (and so the sub-expressions have already been folded, and had their C_MAYBE_CONST_EXPRs removed, and the result of folding will not be re-folded), it should be using save_expr not c_save_expr. So maybe the hook needs to say: use c_save_expr, if parsing, not in_late_binary_op and not folding from within c_fully_fold. Again long term we should aim for the representation during parsing not to need SAVE_EXPRs and for the folding that creates them (and the other folding for optimization in general) to happen only after parsing....
On November 27, 2015 7:55:43 PM GMT+01:00, Marek Polacek <polacek@redhat.com> wrote: >As suggested here ><https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03271.html> >and here <https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03282.html>, >this patch >adds a new langhook to distinguish whether to call c_save_expr or >save_expr >from match.pd. Does this look reasonable? > >I didn't know where to put setting of in_late_processing. With the >current >placement, we won't (for valid programs) call c_save_expr from >c_genericize >or c_gimplify_expr. > >I suppose I should also modify save_expr in fold-const.c to call it via >the >langhook, if this approach is sane. Dunno. I don't like this at all. Different approach: after the FE folds (unexpectedly?), scan the result for SAVE_EXPRs and if found, drop the folding. Richard. >Bootstrapped/regtested on x86_64-linux. > >2015-11-27 Marek Polacek <polacek@redhat.com> > > PR c/68513 > * c-common.c (in_late_processing): New global. > (c_common_save_expr): New function. > * c-common.h (in_late_processing, c_common_save_expr): Declare. > > * c-objc-common.h (LANG_HOOKS_SAVE_EXPR): Define. > * c-parser.c (c_parser_compound_statement): Set IN_LATE_PROCESSING. > > * generic-match-head.c: Include "langhooks.h". > * genmatch.c (dt_simplify::gen_1): Call save_expr via langhook. > * langhooks-def.h (LANG_HOOKS_SAVE_EXPR): Define. > * langhooks.h (struct lang_hooks): Add save_expr langhook. > > * gcc.dg/torture/pr68513.c: New test. > >diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c >index fe0a235..850bee9 100644 >--- gcc/c-family/c-common.c >+++ gcc/c-family/c-common.c >@@ -271,6 +271,12 @@ int c_inhibit_evaluation_warnings; > be generated. */ > bool in_late_binary_op; > >+/* When true, all the constant expression checks from parsing should >have been >+ done. This is used so that fold knows whether to call c_save_expr >(thus >+ c_fully_fold is called on the expression), or whether to call >save_expr via >+ c_common_save_expr langhook. */ >+bool in_late_processing; >+ > /* Whether lexing has been completed, so subsequent preprocessor > errors should use the compiler's input_location. */ > bool done_lexing = false; >@@ -4928,6 +4934,15 @@ c_save_expr (tree expr) > return expr; > } > >+/* The C version of the save_expr langhook. Either call save_expr or >c_save_expr, >+ depending on IN_LATE_PROCESSING. */ >+ >+tree >+c_common_save_expr (tree expr) >+{ >+ return in_late_processing ? save_expr (expr) : c_save_expr (expr); >+} >+ > /* Return whether EXPR is a declaration whose address can never be > NULL. */ > >diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h >index bad8d05..e2d4ba9 100644 >--- gcc/c-family/c-common.h >+++ gcc/c-family/c-common.h >@@ -771,6 +771,7 @@ extern void c_register_addr_space (const char *str, >addr_space_t as); > > /* In c-common.c. */ > extern bool in_late_binary_op; >+extern bool in_late_processing; > extern const char *c_addr_space_name (addr_space_t as); > extern tree identifier_global_value (tree); > extern tree c_linkage_bindings (tree); >@@ -812,6 +813,7 @@ extern tree c_fully_fold (tree, bool, bool *); > extern tree decl_constant_value_for_optimization (tree); > extern tree c_wrap_maybe_const (tree, bool); > extern tree c_save_expr (tree); >+extern tree c_common_save_expr (tree); > extern tree c_common_truthvalue_conversion (location_t, tree); > extern void c_apply_type_quals_to_decl (int, tree); >extern tree c_sizeof_or_alignof_type (location_t, tree, bool, bool, >int); >diff --git gcc/c/c-objc-common.h gcc/c/c-objc-common.h >index 50c9f54..9fd3722 100644 >--- gcc/c/c-objc-common.h >+++ gcc/c/c-objc-common.h >@@ -60,6 +60,8 @@ along with GCC; see the file COPYING3. If not see > #define LANG_HOOKS_BUILTIN_FUNCTION c_builtin_function > #undef LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE >#define LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE >c_builtin_function_ext_scope >+#undef LANG_HOOKS_SAVE_EXPR >+#define LANG_HOOKS_SAVE_EXPR c_common_save_expr > > /* Attribute hooks. */ > #undef LANG_HOOKS_COMMON_ATTRIBUTE_TABLE >diff --git gcc/c/c-parser.c gcc/c/c-parser.c >index 0259f66..3f7c458 100644 >--- gcc/c/c-parser.c >+++ gcc/c/c-parser.c >@@ -4586,6 +4586,9 @@ c_parser_compound_statement (c_parser *parser) > { > tree stmt; > location_t brace_loc; >+ >+ in_late_processing = false; >+ > brace_loc = c_parser_peek_token (parser)->location; > if (!c_parser_require (parser, CPP_OPEN_BRACE, "expected %<{%>")) > { >@@ -4598,6 +4601,9 @@ c_parser_compound_statement (c_parser *parser) > stmt = c_begin_compound_stmt (true); > c_parser_compound_statement_nostart (parser); > >+ /* From now on, the fold machinery shouldn't call c_save_expr. */ >+ in_late_processing = true; >+ >/* If the compound stmt contains array notations, then we expand them. >*/ > if (flag_cilkplus && contains_array_notation_expr (stmt)) > stmt = expand_array_notation_exprs (stmt); >diff --git gcc/generic-match-head.c gcc/generic-match-head.c >index f55f91e..8fc9d40 100644 >--- gcc/generic-match-head.c >+++ gcc/generic-match-head.c >@@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see > #include "builtins.h" > #include "dumpfile.h" > #include "case-cfn-macros.h" >+#include "langhooks.h" > > > /* Routine to determine if the types T1 and T2 are effectively >diff --git gcc/genmatch.c gcc/genmatch.c >index 67d1c66..c73a94d 100644 >--- gcc/genmatch.c >+++ gcc/genmatch.c >@@ -3114,7 +3114,8 @@ dt_simplify::gen_1 (FILE *f, int indent, bool >gimple, operand *result) > continue; > if (cinfo.info[i].result_use_count > 1) > fprintf_indent (f, indent, >- "captures[%d] = save_expr (captures[%d]);\n", >+ "captures[%d] = lang_hooks.save_expr " >+ "(captures[%d]);\n", > i, i); > } > for (unsigned j = 0; j < e->ops.length (); ++j) >diff --git gcc/langhooks-def.h gcc/langhooks-def.h >index 18ac84d..6f5babe 100644 >--- gcc/langhooks-def.h >+++ gcc/langhooks-def.h >@@ -120,6 +120,7 @@ extern bool lhd_omp_mappable_type (tree); > #define LANG_HOOKS_BLOCK_MAY_FALLTHRU hook_bool_const_tree_true > #define LANG_HOOKS_EH_USE_CXA_END_CLEANUP false > #define LANG_HOOKS_DEEP_UNSHARING false >+#define LANG_HOOKS_SAVE_EXPR save_expr > > /* Attribute hooks. */ > #define LANG_HOOKS_ATTRIBUTE_TABLE NULL >@@ -313,7 +314,8 @@ extern void lhd_end_section (void); > LANG_HOOKS_EH_PROTECT_CLEANUP_ACTIONS, \ > LANG_HOOKS_BLOCK_MAY_FALLTHRU, \ > LANG_HOOKS_EH_USE_CXA_END_CLEANUP, \ >- LANG_HOOKS_DEEP_UNSHARING \ >+ LANG_HOOKS_DEEP_UNSHARING, \ >+ LANG_HOOKS_SAVE_EXPR \ > } > > #endif /* GCC_LANG_HOOKS_DEF_H */ >diff --git gcc/langhooks.h gcc/langhooks.h >index d8d01fa..18f332d 100644 >--- gcc/langhooks.h >+++ gcc/langhooks.h >@@ -488,6 +488,9 @@ struct lang_hooks > gimplification. */ > bool deep_unsharing; > >+ /* Used by the fold machinery to either call save_expr or >c_save_expr. */ >+ tree (*save_expr) (tree); >+ > /* Whenever you add entries here, make sure you adjust langhooks-def.h > and langhooks.c accordingly. */ > }; >diff --git gcc/testsuite/gcc.dg/torture/pr68513.c >gcc/testsuite/gcc.dg/torture/pr68513.c >index e69de29..4e08b29 100644 >--- gcc/testsuite/gcc.dg/torture/pr68513.c >+++ gcc/testsuite/gcc.dg/torture/pr68513.c >@@ -0,0 +1,13 @@ >+/* PR c/68513 */ >+/* { dg-do compile } */ >+ >+int i; >+unsigned u; >+volatile unsigned int *e; >+ >+void >+fn1 (void) >+{ >+ (short) ((i ? *e : 0) & ~u | i & u); >+ (short) (((0, 0) ? *e : 0) & ~u | i & u); >+} > > Marek
On Sat, 28 Nov 2015, Richard Biener wrote: > Different approach: after the FE folds (unexpectedly?), scan the result > for SAVE_EXPRs and if found, drop the folding. Or, if conversions are going to fold from language-independent code (which is the underlying problem here - a conversion without folding would be preferred once the fallout from that can be resolved), make the front end fold with c_fully_fold before doing the conversion, and wrap the result of the conversion in a C_MAYBE_CONST_EXPR with c_wrap_maybe_const in the same way as done in other places that fold early (if either c_fully_fold indicates it can't occur in a constant expression, or the result of folding / conversion is not an INTEGER_CST).
On Fri, Nov 27, 2015 at 10:43:42PM +0000, Joseph Myers wrote: > On Fri, 27 Nov 2015, Marek Polacek wrote: > > > I didn't know where to put setting of in_late_processing. With the current > > placement, we won't (for valid programs) call c_save_expr from c_genericize > > or c_gimplify_expr. > > Well, the placement in this patch (in c_parser_compound_statement) is > certainly wrong. It doesn't even save and restore, so after one compound > statement inside another, parsing would continue with in_late_processing > wrongly set. And c_save_expr is logically right for any parsing outside > compound statements as well (arbitrary expressions can occur in sizeof > outside functions and in VLA parameter sizes and should follow the normal > rules for what's a constant expression - there's a known bug that > statement expressions are wrongly rejected in such contexts). Indeed. I don't know what I was thinking. :/ > Starting from first principles: parsing takes place from within > c_parse_file as the sole external entry point to the parser. So you could > have a parsing_input variable that starts off as false, and where > c_parse_file saves it, sets to true, and restores the saved value at the > end. Then you'd use c_save_expr if parsing_input && !in_late_binary_op. > > If that doesn't work, it means there are cases where the hook gets called > from folding that takes place during parsing, on expressions that will not > subsequently go through c_fully_fold, but without in_late_binary_op set. > Knowing what those cases are might help work out any fix for them that is > needed. I'm not sanguine about doing this reliably in stage3. I think I'll try the other approach mentioned later in this thread. > > I suppose I should also modify save_expr in fold-const.c to call it via the > > langhook, if this approach is sane. Dunno. > > That's a complication. When the folding is taking place from within > c_fully_fold (and so the sub-expressions have already been folded, and had > their C_MAYBE_CONST_EXPRs removed, and the result of folding will not be > re-folded), it should be using save_expr not c_save_expr. So maybe the > hook needs to say: use c_save_expr, if parsing, not in_late_binary_op and > not folding from within c_fully_fold. Oh, I see :(. > Again long term we should aim for the representation during parsing not to > need SAVE_EXPRs and for the folding that creates them (and the other > folding for optimization in general) to happen only after parsing.... Yeah, let's strike that for gcc7. Thanks, Marek
On Sat, Nov 28, 2015 at 04:05:30PM +0000, Joseph Myers wrote: > On Sat, 28 Nov 2015, Richard Biener wrote: > > > Different approach: after the FE folds (unexpectedly?), scan the result > > for SAVE_EXPRs and if found, drop the folding. > > Or, if conversions are going to fold from language-independent code (which > is the underlying problem here - a conversion without folding would be > preferred once the fallout from that can be resolved), make the front end > fold with c_fully_fold before doing the conversion, and wrap the result of > the conversion in a C_MAYBE_CONST_EXPR with c_wrap_maybe_const in the same > way as done in other places that fold early (if either c_fully_fold > indicates it can't occur in a constant expression, or the result of > folding / conversion is not an INTEGER_CST). Unfortunately, even this doesn't seem to work :(; I'm getting leaked C_MAYBE_CONST_EXPRs e.g. when converting to (_Complex float), and a bunch of missing warnings resulting in big testsuite fallout. Marek
On Sat, Nov 28, 2015 at 08:50:12AM +0100, Richard Biener wrote: > Different approach: after the FE folds (unexpectedly?), scan the result for > SAVE_EXPRs and if found, drop the folding. Neither this fixes this problem completely, because we simply don't know where those SAVE_EXPRs might be introduced: it might be convert(), but e.g. when I changed the original testcase a tiny bit (added -), then those SAVE_EXPRs were introduced in a different spot (via c_process_stmt_expr -> c_fully_fold). Marek
On Mon, 30 Nov 2015, Marek Polacek wrote: > On Sat, Nov 28, 2015 at 08:50:12AM +0100, Richard Biener wrote: > > Different approach: after the FE folds (unexpectedly?), scan the result for > > SAVE_EXPRs and if found, drop the folding. > > Neither this fixes this problem completely, because we simply don't know where > those SAVE_EXPRs might be introduced: it might be convert(), but e.g. when I > changed the original testcase a tiny bit (added -), then those SAVE_EXPRs were > introduced in a different spot (via c_process_stmt_expr -> c_fully_fold). Well, c_fully_fold should eliminate all C_MAYBE_CONST_EXPRs in its argument and never pass anything containing them to the language-independent folders. So it shouldn't matter if something called by c_fully_fold introduces a SAVE_EXPR. If it does matter, that indicates the problem was earlier (something earlier putting a tree that c_fully_fold doesn't fold around a tree containing a C_MAYBE_CONST_EXPR, without folding first).
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c index fe0a235..850bee9 100644 --- gcc/c-family/c-common.c +++ gcc/c-family/c-common.c @@ -271,6 +271,12 @@ int c_inhibit_evaluation_warnings; be generated. */ bool in_late_binary_op; +/* When true, all the constant expression checks from parsing should have been + done. This is used so that fold knows whether to call c_save_expr (thus + c_fully_fold is called on the expression), or whether to call save_expr via + c_common_save_expr langhook. */ +bool in_late_processing; + /* Whether lexing has been completed, so subsequent preprocessor errors should use the compiler's input_location. */ bool done_lexing = false; @@ -4928,6 +4934,15 @@ c_save_expr (tree expr) return expr; } +/* The C version of the save_expr langhook. Either call save_expr or c_save_expr, + depending on IN_LATE_PROCESSING. */ + +tree +c_common_save_expr (tree expr) +{ + return in_late_processing ? save_expr (expr) : c_save_expr (expr); +} + /* Return whether EXPR is a declaration whose address can never be NULL. */ diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h index bad8d05..e2d4ba9 100644 --- gcc/c-family/c-common.h +++ gcc/c-family/c-common.h @@ -771,6 +771,7 @@ extern void c_register_addr_space (const char *str, addr_space_t as); /* In c-common.c. */ extern bool in_late_binary_op; +extern bool in_late_processing; extern const char *c_addr_space_name (addr_space_t as); extern tree identifier_global_value (tree); extern tree c_linkage_bindings (tree); @@ -812,6 +813,7 @@ extern tree c_fully_fold (tree, bool, bool *); extern tree decl_constant_value_for_optimization (tree); extern tree c_wrap_maybe_const (tree, bool); extern tree c_save_expr (tree); +extern tree c_common_save_expr (tree); extern tree c_common_truthvalue_conversion (location_t, tree); extern void c_apply_type_quals_to_decl (int, tree); extern tree c_sizeof_or_alignof_type (location_t, tree, bool, bool, int); diff --git gcc/c/c-objc-common.h gcc/c/c-objc-common.h index 50c9f54..9fd3722 100644 --- gcc/c/c-objc-common.h +++ gcc/c/c-objc-common.h @@ -60,6 +60,8 @@ along with GCC; see the file COPYING3. If not see #define LANG_HOOKS_BUILTIN_FUNCTION c_builtin_function #undef LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE #define LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE c_builtin_function_ext_scope +#undef LANG_HOOKS_SAVE_EXPR +#define LANG_HOOKS_SAVE_EXPR c_common_save_expr /* Attribute hooks. */ #undef LANG_HOOKS_COMMON_ATTRIBUTE_TABLE diff --git gcc/c/c-parser.c gcc/c/c-parser.c index 0259f66..3f7c458 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -4586,6 +4586,9 @@ c_parser_compound_statement (c_parser *parser) { tree stmt; location_t brace_loc; + + in_late_processing = false; + brace_loc = c_parser_peek_token (parser)->location; if (!c_parser_require (parser, CPP_OPEN_BRACE, "expected %<{%>")) { @@ -4598,6 +4601,9 @@ c_parser_compound_statement (c_parser *parser) stmt = c_begin_compound_stmt (true); c_parser_compound_statement_nostart (parser); + /* From now on, the fold machinery shouldn't call c_save_expr. */ + in_late_processing = true; + /* If the compound stmt contains array notations, then we expand them. */ if (flag_cilkplus && contains_array_notation_expr (stmt)) stmt = expand_array_notation_exprs (stmt); diff --git gcc/generic-match-head.c gcc/generic-match-head.c index f55f91e..8fc9d40 100644 --- gcc/generic-match-head.c +++ gcc/generic-match-head.c @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see #include "builtins.h" #include "dumpfile.h" #include "case-cfn-macros.h" +#include "langhooks.h" /* Routine to determine if the types T1 and T2 are effectively diff --git gcc/genmatch.c gcc/genmatch.c index 67d1c66..c73a94d 100644 --- gcc/genmatch.c +++ gcc/genmatch.c @@ -3114,7 +3114,8 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple, operand *result) continue; if (cinfo.info[i].result_use_count > 1) fprintf_indent (f, indent, - "captures[%d] = save_expr (captures[%d]);\n", + "captures[%d] = lang_hooks.save_expr " + "(captures[%d]);\n", i, i); } for (unsigned j = 0; j < e->ops.length (); ++j) diff --git gcc/langhooks-def.h gcc/langhooks-def.h index 18ac84d..6f5babe 100644 --- gcc/langhooks-def.h +++ gcc/langhooks-def.h @@ -120,6 +120,7 @@ extern bool lhd_omp_mappable_type (tree); #define LANG_HOOKS_BLOCK_MAY_FALLTHRU hook_bool_const_tree_true #define LANG_HOOKS_EH_USE_CXA_END_CLEANUP false #define LANG_HOOKS_DEEP_UNSHARING false +#define LANG_HOOKS_SAVE_EXPR save_expr /* Attribute hooks. */ #define LANG_HOOKS_ATTRIBUTE_TABLE NULL @@ -313,7 +314,8 @@ extern void lhd_end_section (void); LANG_HOOKS_EH_PROTECT_CLEANUP_ACTIONS, \ LANG_HOOKS_BLOCK_MAY_FALLTHRU, \ LANG_HOOKS_EH_USE_CXA_END_CLEANUP, \ - LANG_HOOKS_DEEP_UNSHARING \ + LANG_HOOKS_DEEP_UNSHARING, \ + LANG_HOOKS_SAVE_EXPR \ } #endif /* GCC_LANG_HOOKS_DEF_H */ diff --git gcc/langhooks.h gcc/langhooks.h index d8d01fa..18f332d 100644 --- gcc/langhooks.h +++ gcc/langhooks.h @@ -488,6 +488,9 @@ struct lang_hooks gimplification. */ bool deep_unsharing; + /* Used by the fold machinery to either call save_expr or c_save_expr. */ + tree (*save_expr) (tree); + /* Whenever you add entries here, make sure you adjust langhooks-def.h and langhooks.c accordingly. */ }; diff --git gcc/testsuite/gcc.dg/torture/pr68513.c gcc/testsuite/gcc.dg/torture/pr68513.c index e69de29..4e08b29 100644 --- gcc/testsuite/gcc.dg/torture/pr68513.c +++ gcc/testsuite/gcc.dg/torture/pr68513.c @@ -0,0 +1,13 @@ +/* PR c/68513 */ +/* { dg-do compile } */ + +int i; +unsigned u; +volatile unsigned int *e; + +void +fn1 (void) +{ + (short) ((i ? *e : 0) & ~u | i & u); + (short) (((0, 0) ? *e : 0) & ~u | i & u); +}