Bug 104031 - [12 regression] Global nested constructors generate invalid code since r12-6329-g4f6bc28fc7dd86bd
Summary: [12 regression] Global nested constructors generate invalid code since r12-63...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 12.0
: P1 normal
Target Milestone: 12.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2022-01-14 14:51 UTC by Sergei Trofimovich
Modified: 2022-01-18 10:26 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-01-14 00:00:00


Attachments
gcc12-pr104031.patch (721 bytes, patch)
2022-01-16 20:37 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2022-01-14 14:51:50 UTC
Extracted from original example of nix-2.4 where global constructors register invalid primops. Single-file example:

// $ cat main.cc
#include <string>
#include <vector>

struct Info {
    std::vector<std::string> args;
    size_t arity = 0;
};

struct RegisterPrimOp
{
    RegisterPrimOp(Info && info) __attribute__((noipa, noinline)) {
        if (info.arity != 0)
            __builtin_trap();
    }
};

static RegisterPrimOp s_op({
    .args = {"path"},
    .arity = 0,
});

int main() {}

# ok:
$ g++-11.2.0 main.cc -o main -O2 && ./main
# bad:
$ g++-12.0.0 main.cc -o main -O2 && ./main
Illegal instruction (core dumped)

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)

Must be a recent regression. Possibly a close sibling of diagnostic false positive: https://gcc.gnu.org/PR103984
Comment 1 Martin Liška 2022-01-14 14:56:50 UTC
Also started with r12-6329-g4f6bc28fc7dd86bd.
Comment 2 Andrew Pinski 2022-01-14 18:14:27 UTC
I can remove std::string but not figure out how to remove std::vector yet:
#include <vector>
#include <stddef.h>

struct string
{
  string(int){}
};

struct allocator
{
  allocator(){}
};

struct vector
{
  vector(allocator t = allocator{}){}
  vector(string, allocator t = allocator{}){}
  vector(std::initializer_list<string>&, allocator t = allocator{}){}
};

#if 1
typedef std::vector<string> t;
#else
typedef vector t;
#endif

struct Info {
    t args;
    int arity;
};

struct RegisterPrimOp
{
    RegisterPrimOp(Info && info) __attribute__((noipa, noinline)) {
        if (info.arity != 0)
            __builtin_trap();
    }
};

static RegisterPrimOp s_op({
    .args = {1},
    .arity = 0,
});

int main() {}
Comment 3 Andrew Pinski 2022-01-14 18:27:58 UTC
Reduced testcase without any headers:

struct vector
{
  vector(){}  ~vector(){}
};
struct Info {
    vector args;
    int arity = 0;
};
struct RegisterPrimOp
{
    [[gnu::noipa, gnu::noinline]]
    RegisterPrimOp(Info info) {
        if (info.arity != 0)
            __builtin_trap();
    }
};
static RegisterPrimOp s_op({
    .args = vector{},
    .arity = 0,
});
int main() {}
Comment 4 Sergei Trofimovich 2022-01-14 22:27:29 UTC
Great test, Andrew!

Something is completely dropped initialization of Info{} input argument to s_op. As if it's lifetime ends before RegisterPrimOp{} enters:

--- main.s.good 2022-01-14 21:53:42.334571321 +0000
+++ main.s.bad  2022-01-14 21:53:51.275722971 +0000
@@ -43,26 +43,25 @@
        .p2align 4
        .type   _GLOBAL__sub_I_main, @function
 _GLOBAL__sub_I_main:
 .LFB14:
        .cfi_startproc
        subq    $24, %rsp       #,
        .cfi_def_cfa_offset 32
 # ../main.cc:21: });
        movl    $_ZL4s_op, %edi #,
        leaq    8(%rsp), %rsi   #, tmp84
-       movl    $0, 12(%rsp)    #, D.2496.arityD.2380
        call    _ZN14RegisterPrimOpC1E4Info     #
 # ../main.cc:22: int main() {}
        addq    $24, %rsp       #,
        .cfi_def_cfa_offset 8
        ret
        .cfi_endproc
 .LFE14:
        .size   _GLOBAL__sub_I_main, .-_GLOBAL__sub_I_main
        .section        .init_array,"aw"
        .align 8
        .quad   _GLOBAL__sub_I_main
        .local  _ZL4s_op
        .comm   _ZL4s_op,1,1
-       .ident  "GCC: (GNU) 11.2.0"
+       .ident  "GCC: (GNU) 12.0.0 20220109 (experimental)"
        .section        .note.GNU-stack,"",@progbits

Initial gimple looks valid to me: main.cc.006t.gimple:

void __static_initialization_and_destruction_0 (int __initialize_p, int __priority)
{
  struct InfoD.2399 D.2453 = {.arityD.2402=0};
  ...
  _ZN6vectorC1EvD.2377 (&D.2453.argsD.2401);
  ...
  try { try { try {
    # USE = anything
    # CLB = anything
    _ZN14RegisterPrimOpC1E4InfoD.2413 (&s_opD.2424, &D.2453);
                  } finally {
      # USE = anything
      # CLB = anything
      _ZN4InfoD1EvD.2457 (&D.2453);
       } } finally {
    D.2453 = {CLOBBER};
       }

Final tree also looks ok: main.cc.250t.optimized:

voidD.48 _GLOBAL__sub_I_mainD.2486 ()
{
  struct InfoD.2399 D.2522 = {.arityD.2402=0};
  # USE = nonlocal escaped { D.2424 D.2522 } (nonlocal, escaped)
  # CLB = nonlocal escaped { D.2424 D.2522 } (nonlocal, escaped)
  _ZN14RegisterPrimOpC1E4InfoD.2413 (&_ZL4s_opD.2424, &D.2522);
  D.2522 ={v} {CLOBBER};

First RTL seems to lack the assignment: main.cc.252r.expand:

;; Function _GLOBAL__sub_I_main (_GLOBAL__sub_I_main, funcdef_no=14, decl_uid=2486, cgraph_uid=15, symbol_order=15) (executed once)
...
;;
;; Full RTL generated for this function:
;;
      1: NOTE_INSN_DELETED
      3: NOTE_INSN_BASIC_BLOCK 2
      2: NOTE_INSN_FUNCTION_BEG
    5: {r82:DI=r77:DI-0x8;clobber flags:CC;}
    6: si:DI=r82:DI
    7: di:DI=`_ZL4s_op'
    8: call [`_ZN14RegisterPrimOpC1E4Info'] argc:0
      REG_CALL_DECL `_ZN14RegisterPrimOpC1E4Info'
      REG_EH_REGION 0
Comment 5 Sergei Trofimovich 2022-01-14 22:46:02 UTC
gcc-11 for comparison did not seem to have `struct InfoD.2399 D.2453 = {.arityD.2402=0};` style nodes and encoded stores explicitly: main.cc.244t.optimized:

voidD.48 _GLOBAL__sub_I_main ()
{
  struct InfoD.2377 D.2496;
  ...
  D.2496.arityD.2380 = 0;

and produced expected RTL: main.cc.245r.expand:

;; Function _GLOBAL__sub_I_main (_GLOBAL__sub_I_main, funcdef_no=14, decl_uid=2463, cgraph_uid=15, symbol_order=15) (executed once)
...
;;
;; Full RTL generated for this function:
;;
      1: NOTE_INSN_DELETED
      3: NOTE_INSN_BASIC_BLOCK 2
      2: NOTE_INSN_FUNCTION_BEG

    5: [r77:DI-0x4]=0 // <<<--- our lost store

    6: {r82:DI=r77:DI-0x8;clobber flags:CC;}
    7: si:DI=r82:DI
    8: di:DI=`_ZL4s_op'
    9: call [`_ZN14RegisterPrimOpC1E4Info'] argc:0
      REG_CALL_DECL `_ZN14RegisterPrimOpC1E4Info'
      REG_EH_REGION 0
Comment 6 Sergei Trofimovich 2022-01-14 23:23:07 UTC
> void __static_initialization_and_destruction_0 (int __initialize_p, int
> __priority)
> {
>   struct InfoD.2399 D.2453 = {.arityD.2402=0};

Having poked at -fdump-tree-all-raw I now think `= {.arityD.2402=0};` is not a variable initialization itself, but a reference to `DECL_INITIAL` that would do an initialization.

Perhaps something was supposed to expand this `DECL_INITIAL` to an actual initializing statement, but did not for this case.
Comment 7 Jakub Jelinek 2022-01-15 18:31:20 UTC
My guess is that the problem is that when the TARGET_EXPR_SLOT is created using build_local_temp, current_function_decl is NULL, it is in s_op global namespace var initializer.
Eventually we add it to ssdf and call in the new r12-6329 spot split_nonconstant_init, which has:
743	      if (VAR_P (dest) && !is_local_temp (dest))
744		{
745		  DECL_INITIAL (dest) = init;
746		  TREE_READONLY (dest) = 0;
747		}
but due to the still unchanged DECL_CONTEXT being NULL is_local_temp returns false and we set DECL_INITIAL instead of what the code expects.
The temporary gets DECL_CONTEXT set only during gimplification.
So, I think either we should fill in TARGET_EXPR_SLOT's DECL_CONTEXT when we are adding those exprs into ssdf, or the r12-6329 code should ensure the slot has non-NULL DECL_CONTEXT if current_function_decl or something similar.
Comment 8 Jakub Jelinek 2022-01-16 20:32:30 UTC
That last testcase isn't very good for the testsuite, because 0 is pretty common value on the stack, so even without the store the chances that it will be already zero are high.

42 is less likely...

// PR c++/104031
// { dg-do run { target c++14 } }
// { dg-options "-O2" }

struct A {
  A () {}
  ~A () {}
};
struct B {
  A a;
  int b = 0;
};
struct C
{
  [[gnu::noipa]]
  C (B x) { if (x.b != 42) __builtin_abort (); }
};
static C c ({ .a = A{}, .b = 42 });

int
main ()
{
}
Comment 9 Jakub Jelinek 2022-01-16 20:37:22 UTC
Created attachment 52208 [details]
gcc12-pr104031.patch

This seems to work for the testcase, but dunno if there aren't better fixes.
Comment 10 Andrew Pinski 2022-01-16 22:50:34 UTC
(In reply to Jakub Jelinek from comment #8)
> That last testcase isn't very good for the testsuite, because 0 is pretty
> common value on the stack, so even without the store the chances that it
> will be already zero are high.

Yes I didn't think of that while reducing it further. But that explains why it would pass at -O0 really :)
Comment 11 GCC Commits 2022-01-17 17:11:17 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:aeca44768d54b089243004d1ef00d34dfa9f6530

commit r12-6643-gaeca44768d54b089243004d1ef00d34dfa9f6530
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Jan 17 18:10:34 2022 +0100

    c++: Fix cp_genericize_target_expr for TARGET_EXPRs created for global initialization [PR104031]
    
    The following patch is miscompiled, cp_genericize_target_expr expects
    that for the constant part split_nonconstant_init will emit an INIT_EXPR
    that will initialize it, but that doesn't happen and instead we get
    DECL_INITIAL on the TARGET_EXPR_SLOT that isn't initialized anywhere
    in the IL.
    
    The problem is that the TARGET_EXPR has been created while
    current_function_decl was NULL, it is inside a global var initializer.
    That means the build_local_temp created VAR_DECL has NULL DECL_CONTEXT.
    Later on when genericizing the ssdf (current_function_decl is already
    non-NULL), the new cp_genericize_target_expr is called and during
    split_nonconstant_init it checks is_local_temp, but that due to the NULL
    DECL_CONTEXT returns false.  DECL_CONTEXT is set only later on during
    gimplification.
    
    The following patch fixes it by setting DECL_CONTEXT also inside of
    cp_genericize_target_expr, which fixes the testcase.  But if there are
    better spots to do that, please let me know...
    
    2022-01-17  Jakub Jelinek  <jakub@redhat.com>
    
            PR c++/104031
            * cp-gimplify.c (cp_genericize_target_expr): Set DECL_CONTEXT of
            TARGET_EXPR_SLOT to current_function_decl if it was NULL.
    
            * g++.dg/cpp1y/pr104031.C: New test.
Comment 12 Jakub Jelinek 2022-01-17 17:11:47 UTC
Fixed now.
Comment 13 Sergei Trofimovich 2022-01-17 17:34:35 UTC
The change also fixed original nix-2.4 test failure. Thank you!