[Bug c++/109164] New: aarch64 thread_local initialization error with -ftree-pre and -foptimize-sibling-calls

[Bug c++/109164] New: aarch64 thread_local initialization error with -ftree-pre and -foptimize-sibling-calls

[loganh at synopsys dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

            Bug ID: 109164
           Summary: aarch64 thread_local initialization error with
                    -ftree-pre and -foptimize-sibling-calls
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: loganh at synopsys dot com
  Target Milestone: ---

Created attachment 54687
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54687&action=edit
Bash script that reproduces the issue

With -ftree-pre, -foptimize-sibling-calls, and -O1 enabled, on
aarch64-linux-gnu, GCC 12.1.0 can generate code to access parts of thread_local
variables before the corresponding TLS init function is called if the variable
is accessed from a different TU than the variable is defined in. This
reordering could likely cause a number of different issues, but the one that
I've run into is that:
- When the compiler generates code to call a virtual function on a reference to
a to a global thread_local instance of an object defined in a different
translation unit, and
- The function calls itself in at least once branch,
the address of the  object is fetched from TLS before it's initialized, and
when the vtable lookup is attempted on that object to call the virtual function
the program segfaults.

Here's an example of the kind of code that will trip it up:

struct Struct  {                                                                
  virtual void virtual_func();                                                  
};                                                                              

extern thread_local Struct& thread_local_ref;                                   

bool other_func(void);                                                          

bool test_func(void) {                                                          
  thread_local_ref.virtual_func();                                              
  return other_func() && test_func();                                           
}

When this is compiled (on aarch64-linux-gnu, with -O1 and -ftree-pre and
-foptimize-sibling-calls) to an object file and then dumped with objdump -C -d,
this is the code produced:

0000000000000000 <test_func()>:                                                 
    0: a9be7bfd  stp x29, x30, [sp, #-32]!                                      
    4: 910003fd  mov x29, sp                                                    
    8: a90153f3  stp x19, x20, [sp, #16]                                        
    c: 90000000  adrp  x0, 0 <thread_local_ref>                                 
   10: f9400000  ldr x0, [x0]                                                   
   14: d53bd041  mrs x1, tpidr_el0                                              
   18: f8606834  ldr x20, [x1, x0]                                              
   1c: 90000013  adrp  x19, 0 <TLS init function for thread_local_ref>          
   20: f9400273  ldr x19, [x19]                                                 
   24: b4000053  cbz x19, 2c <test_func()+0x2c>                                 
   28: 94000000  bl  0 <TLS init function for thread_local_ref>                 
   2c: f9400280  ldr x0, [x20]                                                  
   30: f9400001  ldr x1, [x0]                                                   
   34: aa1403e0  mov x0, x20                                                    
   38: d63f0020  blr x1                                                         
   3c: 94000000  bl  0 <other_func()>                                           
   40: 12001c00  and w0, w0, #0xff                                              
   44: 35ffff00  cbnz  w0, 24 <test_func()+0x24>                                
   48: a94153f3  ldp x19, x20, [sp, #16]                                        
   4c: a8c27bfd  ldp x29, x30, [sp], #32                                        
   50: d65f03c0  ret  

Looking at addresses 0x14 through 0x18, you can see that the address of
'thread_local_ref' is read from the TLS block for the thread; the first time
this function is called, this will result in register x20 containing zero,
since the TLS block isn't intialized until the function call at 0x28. Directly
after that, at location 0x2c, a read is attempted from the address in register
x20 (zero) causing a segfault. Without -ftree-pre and -foptimize-sibling calls,
and without letting `test_func` call itself on at least one path, the code to
get the address of `thread_local_ref` is generated after the TLS init call, so
the problem does not occur.

I've attached a script that will reproduce what I've shown here, as well as
demonstrate the issue in action with a full executable that will produce the
segfault I've described.

[Bug tree-optimization/109164] thread_local initialization error with -ftree-pre

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|thread_local initialization |thread_local initialization
                   |error with -ftree-pre       |error with -ftree-pre
                   |-foptimize-sibling-calls    |
      Known to fail|                            |11.3.0

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Here is a testcase which fails at `-O1 -ftree-pre` and does not need
`-foptimize-sibling-calls`:
struct Struct  {
  virtual void virtual_func();
};
extern thread_local Struct& thread_local_ref;
bool other_func(void);
bool test_func(void)
{
  while (1)
    {
      thread_local_ref.virtual_func();
      if (!other_func())
        return 0;
    }
}

[Bug tree-optimization/109164] thread_local initialization error with -ftree-pre

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-03-16
     Ever confirmed|0                           |1

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The bug has been in around since thread_local was added it seems back in GCC
4.8.0.

Confirmed.
Note the wrong code can be reproduced on x86 too.

[Bug tree-optimization/109164] thread_local initialization error with -ftree-pre

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I am not 100% if this is a front-end issue or a gimple level optimization
issue.

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |c++
                 CC|                            |jason at gcc dot gnu.org

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
So we have

;; Function Struct& _ZTW16thread_local_ref() (null)
;; enabled by -tree-original


if (_ZTH16thread_local_ref != 0B)
  {
    <<cleanup_point <<< Unknown tree: expr_stmt
      _ZTH16thread_local_ref () >>>>>;
  }
return <retval> = thread_local_ref;

which is eventually inlined.  But 'thread_local_ref' is TREE_READONLY and
thus may_be_aliased is false and the readonly-global exception triggers:

  /* If the reference is based on a decl that is not aliased the call
     cannot possibly clobber it.  */
  if (DECL_P (base)
      && !may_be_aliased (base)
      /* But local non-readonly statics can be modified through recursion
         or the call may implement a threading barrier which we must
         treat as may-def.  */
      && (TREE_READONLY (base)
          || !is_global_var (base)))
    return false;

I think it's a mistake for the frontend to set TREE_READONLY on the
decl if it requires initialization (we've been there for other runtime
initialized globals).

Thus -> C++ FE bug.

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54693
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54693&action=edit
gcc13-pr109164.patch

Untested fix.

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:0a846340b99675d57fc2f2923a0412134eed09d3

commit r13-6764-g0a846340b99675d57fc2f2923a0412134eed09d3
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Mar 20 20:29:47 2023 +0100

    c++: Drop TREE_READONLY on vars (possibly) initialized by tls wrapper
[PR109164]

    The following two testcases are miscompiled, because we keep TREE_READONLY
    on the vars even when they are (possibly) dynamically initialized by a TLS
    wrapper function.  Normally cp_finish_decl drops TREE_READONLY from vars
    which need dynamic initialization, but for TLS we do this kind of
    initialization upon every access to those variables.  Keeping them
    TREE_READONLY means e.g. PRE can hoist loads from those before loops
    which contain the TLS wrapper calls, so we can access the TLS variables
    before they are initialized.

    2023-03-20  Jakub Jelinek  <jakub@redhat.com>

            PR c++/109164
            * cp-tree.h (var_needs_tls_wrapper): Declare.
            * decl2.cc (var_needs_tls_wrapper): No longer static.
            * decl.cc (cp_finish_decl): Clear TREE_READONLY on TLS variables
            for which a TLS wrapper will be needed.

            * g++.dg/tls/thread_local13.C: New test.
            * g++.dg/tls/thread_local13-aux.cc: New file.
            * g++.dg/tls/thread_local14.C: New test.
            * g++.dg/tls/thread_local14-aux.cc: New file.

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

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

commit r12-9420-gc534a9e56cf16fd996c78f443b6c91f403d794ed
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Mar 20 20:29:47 2023 +0100

    c++: Drop TREE_READONLY on vars (possibly) initialized by tls wrapper
[PR109164]

    The following two testcases are miscompiled, because we keep TREE_READONLY
    on the vars even when they are (possibly) dynamically initialized by a TLS
    wrapper function.  Normally cp_finish_decl drops TREE_READONLY from vars
    which need dynamic initialization, but for TLS we do this kind of
    initialization upon every access to those variables.  Keeping them
    TREE_READONLY means e.g. PRE can hoist loads from those before loops
    which contain the TLS wrapper calls, so we can access the TLS variables
    before they are initialized.

    2023-03-20  Jakub Jelinek  <jakub@redhat.com>

            PR c++/109164
            * cp-tree.h (var_needs_tls_wrapper): Declare.
            * decl2.cc (var_needs_tls_wrapper): No longer static.
            * decl.cc (cp_finish_decl): Clear TREE_READONLY on TLS variables
            for which a TLS wrapper will be needed.

            * g++.dg/tls/thread_local13.C: New test.
            * g++.dg/tls/thread_local13-aux.cc: New file.
            * g++.dg/tls/thread_local14.C: New test.
            * g++.dg/tls/thread_local14-aux.cc: New file.

    (cherry picked from commit 0a846340b99675d57fc2f2923a0412134eed09d3)

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:6a4942ac236ce46499073daed7ebb8fa272aa7fa

commit r11-10728-g6a4942ac236ce46499073daed7ebb8fa272aa7fa
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Mar 20 20:29:47 2023 +0100

    c++: Drop TREE_READONLY on vars (possibly) initialized by tls wrapper
[PR109164]

    The following two testcases are miscompiled, because we keep TREE_READONLY
    on the vars even when they are (possibly) dynamically initialized by a TLS
    wrapper function.  Normally cp_finish_decl drops TREE_READONLY from vars
    which need dynamic initialization, but for TLS we do this kind of
    initialization upon every access to those variables.  Keeping them
    TREE_READONLY means e.g. PRE can hoist loads from those before loops
    which contain the TLS wrapper calls, so we can access the TLS variables
    before they are initialized.

    2023-03-20  Jakub Jelinek  <jakub@redhat.com>

            PR c++/109164
            * cp-tree.h (var_needs_tls_wrapper): Declare.
            * decl2.c (var_needs_tls_wrapper): No longer static.
            * decl.c (cp_finish_decl): Clear TREE_READONLY on TLS variables
            for which a TLS wrapper will be needed.

            * g++.dg/tls/thread_local13.C: New test.
            * g++.dg/tls/thread_local13-aux.cc: New file.
            * g++.dg/tls/thread_local14.C: New test.
            * g++.dg/tls/thread_local14-aux.cc: New file.

    (cherry picked from commit 0a846340b99675d57fc2f2923a0412134eed09d3)

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:73accd859e80e5d50937b8dc85a4e509fc174b97

commit r10-11381-g73accd859e80e5d50937b8dc85a4e509fc174b97
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Mar 20 20:29:47 2023 +0100

    c++: Drop TREE_READONLY on vars (possibly) initialized by tls wrapper
[PR109164]

    The following two testcases are miscompiled, because we keep TREE_READONLY
    on the vars even when they are (possibly) dynamically initialized by a TLS
    wrapper function.  Normally cp_finish_decl drops TREE_READONLY from vars
    which need dynamic initialization, but for TLS we do this kind of
    initialization upon every access to those variables.  Keeping them
    TREE_READONLY means e.g. PRE can hoist loads from those before loops
    which contain the TLS wrapper calls, so we can access the TLS variables
    before they are initialized.

    2023-03-20  Jakub Jelinek  <jakub@redhat.com>

            PR c++/109164
            * cp-tree.h (var_needs_tls_wrapper): Declare.
            * decl2.c (var_needs_tls_wrapper): No longer static.
            * decl.c (cp_finish_decl): Clear TREE_READONLY on TLS variables
            for which a TLS wrapper will be needed.

            * g++.dg/tls/thread_local13.C: New test.
            * g++.dg/tls/thread_local13-aux.cc: New file.
            * g++.dg/tls/thread_local14.C: New test.
            * g++.dg/tls/thread_local14-aux.cc: New file.

    (cherry picked from commit 0a846340b99675d57fc2f2923a0412134eed09d3)

[Bug c++/109164] wrong code with thread_local reference, loops and -ftree-pre

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109164

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 10.5 too.