Noticed new warning on shaderc-2021.0 which uses -Werror. Last week's gcc-12 snapshot did not flag it as a warning. Here is the extracted example: // $ cat glslc/src/main.cc #include <string> #include <vector> #include <cstring> struct string_piece { const char * begin_ = nullptr; const char * end_ = nullptr; std::string str() const { return std::string(begin_, end_); } string_piece(const char* string) : begin_(string), end_(string) { end_ += strlen(string); } string_piece(const string_piece& other) { begin_ = other.begin_; end_ = other.end_; } }; struct Z { std::string name; int stage; }; extern int to_stage(string_piece file_name); int main(int argc, char** argv) { std::vector<Z> input_files; const std::string current_entry_point_name("main"); const string_piece arg = argv[0]; input_files.emplace_back(Z{ arg.str(), // what is the lifetime of this temporary? to_stage(arg), // why does this constructor matter? }); return 0; } $ g++-12.0.0 -O1 -Wall -Werror -std=c++11 -o main.cc.o -c glslc/src/main.cc In file included from /<<NIX>>/gcc-12.0.0/include/c++/12.0.0/string:53, from glslc/src/main.cc:1: In member function 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::pointer std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_data() const [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]', inlined from 'bool std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_is_local() const [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /<<NIX>>/gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:275:23, inlined from 'void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_dispose() [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /<<NIX>>/gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:286:18, inlined from 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::~basic_string() [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /<<NIX>>/gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:793:19, inlined from 'int main(int, char**)' at glslc/src/main.cc:35:1: /<<NIX>>/gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:235:28: error: '<unnamed>.Z::name.std::__cxx11::basic_string<char>::_M_dataplus.std::__cxx11::basic_string<char>::_Alloc_hider::_M_p' may be used uninitialized [-Werror=maybe-uninitialized] 235 | { return _M_dataplus._M_p; } | ^~~~ glslc/src/main.cc: In function 'int main(int, char**)': glslc/src/main.cc:32:3: note: '<anonymous>' declared here 32 | }); | ^ cc1plus: all warnings being treated as errors $ g++-12.0.0 -v Using built-in specs. COLLECT_GCC=/<<NIX>>/gcc-12.0.0/bin/g++ COLLECT_LTO_WRAPPER=/<<NIX>>/gcc-12.0.0/libexec/gcc/x86_64-unknown-linux-gnu/12.0.0/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: Thread model: posix Supported LTO compression algorithms: zlib gcc version 12.0.0 20220109 (experimental) (GCC)
Before GCC 12 we had: {.name=TARGET_EXPR ... In GCC 12 we get: <<< Unknown tree: expr_stmt D.31068.name = TARGET_EXPR ....
Looks related to one of the following PRs: PR 52320 PR 66139 PR 66451
Jason pushed a whole set of changes in this area lately, we'd better understand what's going on here.
Started with r12-6329-g4f6bc28fc7dd86bd9e7408cbf28de1e973dd1eda
(In reply to Sergei Trofimovich from comment #0) > input_files.emplace_back(Z{ > arg.str(), // what is the lifetime of this temporary? This prvalue initializes the 'name' member of the Z temporary, there is no separate string temporary. > to_stage(arg), // why does this constructor matter? My fix for PR66139 fixed EH in aggregate initialization so that if evaluation of to_stage(arg) throws, we properly destroy the previously initialized 'name' member. That's the difference pinskia mentions. It is surprising that we warn with a user-provided copy constructor and don't with a defaulted constructor. It changes the code a bit because it means string_piece isn't trivially copyable, so we need to create a string_piece temporary for passing to to_stage instead of passing arg directly by bitwise copy, but I don't know why that would confuse the optimizers into thinking that _M_p might be uninitialized.
(In reply to Jason Merrill from comment #5) > It is surprising that we warn with a user-provided copy constructor and > don't with a defaulted constructor. It changes the code a bit because it > means string_piece isn't trivially copyable, so we need to create a > string_piece temporary for passing to to_stage instead of passing arg > directly by bitwise copy, but I don't know why that would confuse the > optimizers into thinking that _M_p might be uninitialized. The diff of the GIMPLE is @@ -40,10 +39,7 @@ D$ = 1; try { - string_piece::string_piece (&D$, &arg); - try - { - _2 = to_stage (&D$); + _2 = to_stage (arg); D$.stage = _2; D$ = 0; try @@ -62,11 +58,6 @@ D$ = {CLOBBER}; } } - finally - { - D$ = {CLOBBER}; - } - } catch { if (D$ != 0) goto <D$>; else goto <D$>;
This doesn't seem to be a C++ issue, unassigning myself.
Note I have a different test-case from benchmark package that show the same: $ cat benchmark.ii namespace std { inline namespace __cxx11 {} struct __new_allocator { void deallocate(char *, long) { operator delete(0); } }; template <typename> using __allocator_base = __new_allocator; template <typename> struct allocator_traits; template <typename> struct allocator : __allocator_base<int> {}; template <typename _Tp> struct allocator_traits<allocator<_Tp>> { using allocator_type = allocator<_Tp>; using pointer = _Tp *; using size_type = long; static void deallocate(allocator_type __a, pointer __p, size_type __n) { __a.deallocate(__p, __n); } }; } // namespace std struct __alloc_traits : std::allocator_traits<std::allocator<char>> {}; namespace std { template <class> struct initializer_list { int *_M_array; unsigned long _M_len; }; namespace __cxx11 { struct basic_string { struct _Alloc_hider : allocator_traits<allocator<char>> { pointer _M_p; } _M_dataplus; basic_string(const char *, const allocator<char> & = allocator<char>()); ~basic_string() { if (_M_dataplus._M_p) { allocator<char> __trans_tmp_1; __alloc_traits::deallocate(__trans_tmp_1, 0, 1); } } }; } // namespace __cxx11 } // namespace std struct TestCase { std::basic_string name; bool error_occurred; std::basic_string error_message; }; int AddCases(std::initializer_list<TestCase>); int dummy68 = AddCases({{"", true, ""}});
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:ad66b03b3c84786e73e73f09be19977b8f3c4ea3 commit r12-7434-gad66b03b3c84786e73e73f09be19977b8f3c4ea3 Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Mar 1 09:33:21 2022 +0000 libstdc++: Fix -Wmaybe-uninitialized false positive [PR103984] This fixes a false positive warning seen with LTO: 12/bits/regex_compiler.tcc:443:32: error: '__last_char._M_char' may be used uninitialized [-Werror=maybe-uninitialized] Given that the std::regex code is not very efficient anyway, the overhead of initializing this byte should be minimal. libstdc++-v3/ChangeLog: PR middle-end/103984 * include/bits/regex_compiler.h (_BracketMatcher::_M_char): Use default member initializer.
What the C++ FE emits looks just weird to me. In the gimple dump: [pr103984.C:35:1] D.31524.name = string_piece::str ([pr103984.C:35:1] &arg); [return slot optimization] [pr103984.C:35:1] D.31544 = 1; [pr103984.C:29:27] try { [pr103984.C:31:15] string_piece::string_piece ([pr103984.C:31:15] &D.31523, [pr103984.C:31:16] &arg); try { [pr103984.C:31:15] _2 = to_stage ([pr103984.C:31:15] &D.31523); [pr103984.C:35:1] D.31524.stage = _2; [pr103984.C:35:1] D.31544 = 0; try { try { [pr103984.C:29:27] std::vector<Z>::emplace_back<Z> ([pr103984.C:29:27] &input_files, [pr103984.C:29:28] &D.31524); } finally { [pr103984.C:29:28] Z::~Z ([pr103984.C:29:28] &D.31524); } } finally { [pr103984.C:29:28] D.31524 = {CLOBBER(eol)}; } } finally { [pr103984.C:31:15] D.31523 = {CLOBBER(eol)}; } } catch { [pr103984.C:35:1] if (D.31544 != 0) goto <D.35853>; else goto <D.35854>; <D.35853>: [pr103984.C:35:1] std::__cxx11::basic_string<char>::~basic_string ([pr103984.C:35:1] &D.31524.name); goto <D.35855>; <D.35854>: <D.35855>: } Note the D.31524 CLOBBER being added only conditionally (if string_piece ctor doesn't throw), while if it throws, there is a dtor on its element after that and no CLOBBER. The first spot that initializes part of the D.31524 temporary is the above first line, so it would make more sense to me to wrap this whole sequence above into try { ... } finally { [pr103984.C:29:28] D.31524 = {CLOBBER(eol)}; } and remove the inner try finally that does that.
And it is ehcleanup that sinks the clobber. Before ehcleanup1 we have: ;; basic block 16, loop depth 0, maybe hot ;; prev block 15, next block 17, flags: (NEW, REACHABLE, VISITED) ;; pred: 15 (EH,EXECUTABLE) <L7>: D.31524 ={v} {CLOBBER(eol)}; resx 10 ;; succ: 17 (EH,EXECUTABLE) ;; basic block 17, loop depth 0, maybe hot ;; prev block 16, next block 18, flags: (NEW, REACHABLE, VISITED) ;; pred: 4 (EH,EXECUTABLE) ;; 16 (EH,EXECUTABLE) # _3 = PHI <1(4), 0(16)> <L8>: D.31523 ={v} {CLOBBER(eol)}; resx 9 ;; succ: 18 (EH,EXECUTABLE) ;; basic block 18, loop depth 0, maybe hot ;; prev block 17, next block 19, flags: (NEW, REACHABLE, VISITED) ;; pred: 17 (EH,EXECUTABLE) <L9>: if (_3 != 0) goto <bb 19>; [INV] else goto <bb 22>; [INV] ;; succ: 19 (TRUE_VALUE,EXECUTABLE) ;; 22 (FALSE_VALUE,EXECUTABLE) ;; basic block 19, loop depth 0, maybe hot ;; prev block 18, next block 20, flags: (NEW, REACHABLE, VISITED) ;; pred: 18 (TRUE_VALUE,EXECUTABLE) _55 = MEM[(const struct basic_string *)&D.31524]._M_dataplus._M_p; so at this point, if we enter bb 17 from the edge from bb 4, D.31524 isn't clobbered and the _55 load is done (and conditional operator delete later), if we enter bb 16 then D.31524 is clobbered and then _3 is 0 and we don't try to delete it again. But ehcleanup1, probably on the assumption that such conditional cleanups aren't valid, turns that into: ... ;; basic block 14, loop depth 0, maybe hot ;; prev block 13, next block 15, flags: (NEW, REACHABLE, VISITED) ;; pred: 4 (EH,EXECUTABLE) ;; 13 (EH,EXECUTABLE) # _3 = PHI <1(4), 0(13)> <L8>: D.31524 ={v} {CLOBBER(eol)}; D.31523 ={v} {CLOBBER(eol)}; if (_3 != 0) goto <bb 15>; [INV] else goto <bb 18>; [INV] ;; succ: 15 (TRUE_VALUE,EXECUTABLE) ;; 18 (FALSE_VALUE,EXECUTABLE) ;; basic block 15, loop depth 0, maybe hot ;; prev block 14, next block 16, flags: (NEW, REACHABLE, VISITED) ;; pred: 14 (TRUE_VALUE,EXECUTABLE) _55 = MEM[(const struct basic_string *)&D.31524]._M_dataplus._M_p; i.e. now D.31524 ={v} {CLOBBER(eol)}; is done no matter which path we enver that bb from and then uninit pass is right about the warning, because the _55 = MEM... load reads an uninitialized var.
In particular, in ehcleanup1 it is the sink_clobbers optimization added for PR51117. I think it would be strongly preferrable to fix this in the FE, because if we'd need to change sink_clobbers, I'd be afraid we'd get significantly worse code generation with -flifetime-dse than before.
What the gimplifier sees is: <target_expr 0x7fffe8f61a48 type <record_type 0x7fffe8f0bd20 Z addressable needs-constructing cxx-odr-p type_4 type_5 type_6 BLK size <integer_cst 0x7fffe9dd8840 constant 320> unit-size <integer_cst 0x7fffe9dd8888 constant 40> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffe8f0bd20 fields <function_decl 0x7fffe8dfad00 operator= type <method_type 0x7fffe8a07690> addressable used public external autoinline decl_3 QI pr103984.C:21:8 align:16 warn_if_not_align:0 context <record_type 0x7fffe8f0bd20 Z> full-name "Z& Z::operator=(Z&&) noexcept" not-really-extern chain <function_decl 0x7fffe8dfac00 operator=>> context <translation_unit_decl 0x7fffea24c168 pr103984.ii> full-name "struct Z" needs-constructor needs-destructor X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown pointer_to_this <pointer_type 0x7fffe8f0f690> reference_to_this <reference_type 0x7fffe8f0f9d8> chain <type_decl 0x7fffe8f0c558 Z>> side-effects addressable arg:0 <var_decl 0x7fffe8e3f510 D.31524 type <record_type 0x7fffe8f0bd20 Z> addressable used ignored read BLK pr103984.C:32:3 size <integer_cst 0x7fffe9dd8840 320> unit-size <integer_cst 0x7fffe9dd8888 40> align:64 warn_if_not_align:0 context <function_decl 0x7fffe8f0e000 main>> arg:1 <statement_list 0x7fffe8e860c0 type <void_type 0x7fffea25ff18 void type_6 VOID align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffea25ff18 pointer_to_this <pointer_type 0x7fffea267000>> side-effects head 0x7fffe8e73660 tail 0x7fffe8e73780 stmts 0x7fffe8e860e0 0x7fffe8f61af0 0x7fffe8e861a0 0x7fffe8e3b758 stmt <expr_stmt 0x7fffe8e860e0 type <void_type 0x7fffea25ff18 void> side-effects arg:0 <init_expr 0x7fffe8e3b708 type <record_type 0x7fffea4010a8 string> side-effects arg:0 <component_ref 0x7fffe95572d0 type <record_type 0x7fffea4010a8 string> arg:0 <var_decl 0x7fffe8e3f510 D.31524> arg:1 <field_decl 0x7fffe8f0c688 name>> arg:1 <target_expr 0x7fffe8f618f8 type <record_type 0x7fffea4010a8 string> side-effects tree_0 arg:0 <var_decl 0x7fffe8e3f360 D.31522> arg:1 <aggr_init_expr 0x7fffe8f61888 type <void_type 0x7fffea25ff18 void> side-effects arg:0 <integer_cst 0x7fffea2611b0 constant 4> arg:1 <addr_expr 0x7fffe8e7c600 type <pointer_type 0x7fffe8f67540> constant arg:0 <function_decl 0x7fffe8f04400 str>> arg:2 <var_decl 0x7fffe8e3f360 D.31522> arg:3 <addr_expr 0x7fffe8e7c5c0 type <pointer_type 0x7fffe8ed7dc8> readonly arg:0 <var_decl 0x7fffe8e3f120 arg> pr103984.C:30:7 start: pr103984.C:30:7 finish: pr103984.C:30:9>> arg:2 <call_expr 0x7fffe8f618c0 type <void_type 0x7fffea25ff18 void> side-effects nothrow fn <addr_expr 0x7fffe8e7c6a0 type <pointer_type 0x7fffe95c30a8> constant arg:0 <function_decl 0x7fffe95aa200 __dt_comp >> arg:0 <addr_expr 0x7fffe8e7c660 type <pointer_type 0x7fffe95bf930> arg:0 <var_decl 0x7fffe8e3f360 D.31522>>> pr103984.C:30:14 start: pr103984.C:30:7 finish: pr103984.C:30:15>> pr103984.C:35:1 start: pr103984.C:35:1 finish: pr103984.C:35:1> stmt <target_expr 0x7fffe8f61af0 type <boolean_type 0x7fffea25fb28 bool> side-effects static arg:0 <var_decl 0x7fffe8e3f5a0 D.31544> arg:1 <integer_cst 0x7fffea2612d0 constant 1> arg:2 <cond_expr 0x7fffe9557390 type <void_type 0x7fffea25ff18 void> side-effects arg:0 <var_decl 0x7fffe8e3f5a0 D.31544> arg:1 <call_expr 0x7fffe8f61ab8 type <void_type 0x7fffea25ff18 void> side-effects nothrow fn <addr_expr 0x7fffe8e86180 type <pointer_type 0x7fffe95c30a8> constant arg:0 <function_decl 0x7fffe95aa200 __dt_comp >> arg:0 <addr_expr 0x7fffe8e86140 type <pointer_type 0x7fffe95bf930> arg:0 <component_ref 0x7fffe8a35300 type <record_type 0x7fffea4010a8 string> arg:0 <var_decl 0x7fffe8e3f510 D.31524> arg:1 <field_decl 0x7fffe8f0c688 name>>>> arg:2 <void_cst 0x7fffea252c60 type <void_type 0x7fffea25ff18 void> constant>> pr103984.C:35:1 start: pr103984.C:35:1 finish: pr103984.C:35:1> stmt <expr_stmt 0x7fffe8e861a0 type <void_type 0x7fffea25ff18 void> side-effects arg:0 <init_expr 0x7fffe8e3b730 type <integer_type 0x7fffea25f5e8 int> side-effects arg:0 <component_ref 0x7fffe95573c0 type <integer_type 0x7fffea25f5e8 int> arg:0 <var_decl 0x7fffe8e3f510 D.31524> arg:1 <field_decl 0x7fffe8f0c720 stage>> arg:1 <call_expr 0x7fffe8f61968 type <integer_type 0x7fffea25f5e8 int> side-effects fn <addr_expr 0x7fffe8e7c840 type <pointer_type 0x7fffe8f67738> constant arg:0 <function_decl 0x7fffe8f0af00 to_stage>> arg:0 <addr_expr 0x7fffe8e7c820 type <reference_type 0x7fffe8f0bf18> side-effects arg:0 <target_expr 0x7fffe8f61930 type <record_type 0x7fffe8ed7690 string_piece> side-effects tree_0 arg:0 <var_decl 0x7fffe8e3f480 D.31523> arg:1 <aggr_init_expr 0x7fffe8f51f80>>> pr103984.C:31:15 start: pr103984.C:31:7 finish: pr103984.C:31:19>> pr103984.C:35:1 start: pr103984.C:35:1 finish: pr103984.C:35:1> stmt <modify_expr 0x7fffe8e3b758 type <boolean_type 0x7fffea25fb28 bool> side-effects arg:0 <var_decl 0x7fffe8e3f5a0 D.31544> arg:1 <integer_cst 0x7fffea2612a0 constant 0> pr103984.C:35:1 start: pr103984.C:35:1 finish: pr103984.C:35:1>> arg:2 <call_expr 0x7fffe8f61b28 type <void_type 0x7fffea25ff18 void> side-effects nothrow fn <addr_expr 0x7fffe8e7cda0 type <pointer_type 0x7fffe8f67e70> constant arg:0 <function_decl 0x7fffe8f66b00 __dt_comp >> arg:0 <addr_expr 0x7fffe8e7c8a0 type <pointer_type 0x7fffe8f0f690> arg:0 <var_decl 0x7fffe8e3f510 D.31524>>> pr103984.C:29:28 start: pr103984.C:29:28 finish: pr103984.C:32:3> i.e. the ~Z (&D.31524) is a cleanup is outer, while the ~string (&D.31524.name) cleanup is on an TARGET_EXPR inside of it (the one with D.31544 var). But the inner TARGET_EXPR has CLEANUP_EH_ONLY set on it, while the outer one (with D.31524 var) doesn't. Jason, is there any way how to arrange for such cases (with help of the C++ FE, say with some new flag on something or just pure gimplifier) to defer the D.31524 clobber only to outside of where the eh only cleanup is handled (that is on the enclosing CLEANUP_POINT_EXPR, right?)?
Wonder about: --- gcc/gimplify.cc.jj 2022-03-04 15:14:53.197812540 +0100 +++ gcc/gimplify.cc 2022-03-15 17:44:45.110734179 +0100 @@ -6997,8 +6997,6 @@ gimplify_target_expr (tree *expr_p, gimp if (init) { - tree cleanup = NULL_TREE; - /* TARGET_EXPR temps aren't part of the enclosing block, so add it to the temps list. Handle also variable length TARGET_EXPRs. */ if (!poly_int_tree_p (DECL_SIZE (temp))) @@ -7019,37 +7017,6 @@ gimplify_target_expr (tree *expr_p, gimp gimple_add_tmp_var (temp); } - /* If TARGET_EXPR_INITIAL is void, then the mere evaluation of the - expression is supposed to initialize the slot. */ - if (VOID_TYPE_P (TREE_TYPE (init))) - ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none); - else - { - tree init_expr = build2 (INIT_EXPR, void_type_node, temp, init); - init = init_expr; - ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none); - init = NULL; - ggc_free (init_expr); - } - if (ret == GS_ERROR) - { - /* PR c++/28266 Make sure this is expanded only once. */ - TARGET_EXPR_INITIAL (targ) = NULL_TREE; - return GS_ERROR; - } - if (init) - gimplify_and_add (init, pre_p); - - /* If needed, push the cleanup for the temp. */ - if (TARGET_EXPR_CLEANUP (targ)) - { - if (CLEANUP_EH_ONLY (targ)) - gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ), - CLEANUP_EH_ONLY (targ), pre_p); - else - cleanup = TARGET_EXPR_CLEANUP (targ); - } - /* Add a clobber for the temporary going out of scope, like gimplify_bind_expr. */ if (gimplify_ctxp->in_cleanup_point_expr @@ -7079,8 +7046,32 @@ gimplify_target_expr (tree *expr_p, gimp } } } - if (cleanup) - gimple_push_cleanup (temp, cleanup, false, pre_p); + + /* If TARGET_EXPR_INITIAL is void, then the mere evaluation of the + expression is supposed to initialize the slot. */ + if (VOID_TYPE_P (TREE_TYPE (init))) + ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none); + else + { + tree init_expr = build2 (INIT_EXPR, void_type_node, temp, init); + init = init_expr; + ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none); + init = NULL; + ggc_free (init_expr); + } + if (ret == GS_ERROR) + { + /* PR c++/28266 Make sure this is expanded only once. */ + TARGET_EXPR_INITIAL (targ) = NULL_TREE; + return GS_ERROR; + } + if (init) + gimplify_and_add (init, pre_p); + + /* If needed, push the cleanup for the temp. */ + if (TARGET_EXPR_CLEANUP (targ)) + gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ), + CLEANUP_EH_ONLY (targ), pre_p); /* Only expand this once. */ TREE_OPERAND (targ, 3) = init; i.e. emitting the clobber gimple_push_cleanup for the TARGET_EXPRs before the gimplification of the TARGET_EXPR_INITIAL instead of after it (and emitting the WCE earlier means the cleanup is later (as in, outer try/finally).
Created attachment 52632 [details] gcc12-pr103984.patch Full untested patch.
Created attachment 52633 [details] gcc12-pr103984.patch
Unfortunately the #c15 patch regressed: +FAIL: g++.dg/opt/pr80032.C -std=gnu++11 (test for excess errors) +FAIL: g++.dg/opt/pr80032.C -std=gnu++14 (test for excess errors) +FAIL: g++.dg/opt/pr80032.C -std=gnu++17 (test for excess errors) +FAIL: g++.dg/opt/pr80032.C -std=gnu++20 (test for excess errors) +FAIL: g++.dg/opt/pr80032.C -std=gnu++2b (test for excess errors) +FAIL: g++.dg/opt/stack2.C -std=gnu++11 (test for excess errors) +FAIL: g++.dg/opt/stack2.C -std=gnu++14 (test for excess errors) +FAIL: g++.dg/opt/stack2.C -std=gnu++17 (test for excess errors) +FAIL: g++.dg/opt/stack2.C -std=gnu++20 (test for excess errors) +FAIL: g++.dg/opt/stack2.C -std=gnu++2b (test for excess errors) +FAIL: g++.dg/opt/stack2.C -std=gnu++98 (test for excess errors) E.g. on stack2.C, the problem is that needs_to_live_in_memory (temp) is false before TARGET_EXPR_INITIAL is gimplified, so we don't add a clobber at all in that case with the first patch and so the stack sharing doesn't happen. This patch is a new attempt, do the TARGET_EXPR_INITIAL gimplification first, but into a temporary sequence, then push the cleanups (in the order CLOBBER first (in the end last), then asan poisioning, then TARGET_EXPR_CLEANUP, which means the opposite order later on, so first some destructors etc., then poisioning and finally a clobber).
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:7276a18aba41eed65c0cf535ae029e0ceeca6c77 commit r12-7686-g7276a18aba41eed65c0cf535ae029e0ceeca6c77 Author: Jakub Jelinek <jakub@redhat.com> Date: Thu Mar 17 09:23:45 2022 +0100 gimplify: Emit clobbers for TARGET_EXPR_SLOT vars later [PR103984] As mentioned in the PR, we emit a bogus uninitialized warning but easily could emit wrong-code for it or similar testcases too. The bug is that we emit clobber for a TARGET_EXPR_SLOT too early: D.2499.e = B::qux (&h); [return slot optimization] D.2516 = 1; try { B::B (&D.2498, &h); try { _2 = baz (&D.2498); D.2499.f = _2; D.2516 = 0; try { try { bar (&D.2499); } finally { C::~C (&D.2499); } } finally { D.2499 = {CLOBBER(eol)}; } } finally { D.2498 = {CLOBBER(eol)}; } } catch { if (D.2516 != 0) goto <D.2517>; else goto <D.2518>; <D.2517>: A::~A (&D.2499.e); goto <D.2519>; <D.2518>: <D.2519>: } The CLOBBER for D.2499 is essentially only emitted on the non-exceptional path, if B::B or baz throws, then there is no CLOBBER for it but there is a conditional destructor A::~A (&D.2499.e). Now, ehcleanup1 sink_clobbers optimization assumes that clobbers in the EH cases are emitted after last use and so sinks the D.2499 = {CLOBBER(eol)}; later, so we then have # _3 = PHI <1(3), 0(9)> <L2>: D.2499 ={v} {CLOBBER(eol)}; D.2498 ={v} {CLOBBER(eol)}; if (_3 != 0) goto <bb 11>; [INV] else goto <bb 15>; [INV] <bb 11> : _35 = D.2499.a; if (&D.2499.b != _35) where that _35 = D.2499.a comes from inline expansion of the A::~A dtor, and that is a load from a clobbered memory. Now, what the gimplifier sees in this case is a CLEANUP_POINT_EXPR with somewhere inside of it a TARGET_EXPR for D.2499 (with the C::~C (&D.2499) cleanup) which in its TARGET_EXPR_INITIAL has another TARGET_EXPR for D.2516 bool flag which has CLEANUP_EH_ONLY which performs that conditional A::~A (&D.2499.e) call. The following patch ensures that CLOBBERs (and asan poisoning) are emitted after even those gimple_push_cleanup pushed cleanups from within the TARGET_EXPR_INITIAL gimplification (i.e. the last point where the slot could be in theory used). In my first version of the patch I've done it by just moving the /* Add a clobber for the temporary going out of scope, like gimplify_bind_expr. */ if (gimplify_ctxp->in_cleanup_point_expr && needs_to_live_in_memory (temp)) { ... } block earlier in gimplify_target_expr, but that regressed a couple of tests where temp is marked TREE_ADDRESSABLE only during (well, very early during that) the gimplification of TARGET_EXPR_INITIAL, so we didn't emit e.g. on pr80032.C or stack2.C tests any clobbers for the slots and thus stack slot reuse wasn't performed. So that we don't regress those tests, this patch gimplifies TARGET_EXPR_INITIAL as before, but doesn't emit it directly into pre_p, emits it into a temporary sequence. Then emits the CLOBBER cleanup into pre_p, then asan poisoning if needed, then appends the TARGET_EXPR_INITIAL temporary sequence and finally adds TARGET_EXPR_CLEANUP gimple_push_cleanup. The earlier a GIMPLE_WCE appears in the sequence, the outer try/finally or try/catch it is. So, with this patch the part of the testcase in gimple dump cited above looks instead like: try { D.2499.e = B::qux (&h); [return slot optimization] D.2516 = 1; try { try { B::B (&D.2498, &h); _2 = baz (&D.2498); D.2499.f = _2; D.2516 = 0; try { bar (&D.2499); } finally { C::~C (&D.2499); } } finally { D.2498 = {CLOBBER(eol)}; } } catch { if (D.2516 != 0) goto <D.2517>; else goto <D.2518>; <D.2517>: A::~A (&D.2499.e); goto <D.2519>; <D.2518>: <D.2519>: } } finally { D.2499 = {CLOBBER(eol)}; } 2022-03-17 Jakub Jelinek <jakub@redhat.com> PR middle-end/103984 * gimplify.cc (gimplify_target_expr): Gimplify type sizes and TARGET_EXPR_INITIAL into a temporary sequence, then push clobbers and asan unpoisioning, then append the temporary sequence and finally the TARGET_EXPR_CLEANUP clobbers. * g++.dg/opt/pr103984.C: New test.
Fixed.