This is an archive of the discontinued LLVM Phabricator instance.

thread_local support for AIX
ClosedPublic

Authored by jamieschmeiser on Jun 16 2021, 2:08 PM.

Details

Summary

Changes to support thread_local storage on AIX.

The AIX linker will produce errors on unresolved weak symbols. Change the
generated code to not check for the initialization function but just call
it and ensure that it always exists. Also, the AIX atexit routine has a
different name (and signature) so call it correctly. Update the lit tests
to test on AIX appropriately.

Diff Detail

Event Timeline

jamieschmeiser requested review of this revision.Jun 16 2021, 2:08 PM
jamieschmeiser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 2:08 PM

Fix formatting problem.

clang/test/CodeGenCXX/cxx11-thread-local-reference.cpp
12

Fix typo. Also, if the Linux and the AIX pattern is the same, adding a NONDARWIN prefix could help.

25–26

Can use {{.*}} in place of comdat to common up the Linux and the AIX pattern.

clang/test/CodeGenCXX/cxx11-thread-local.cpp
13

Minor nit: I think this should be moved to after line 5.

18

Same comment as for the earlier file re: adding a prefix to handle the common Linux and AIX lines.

214

Same comment as earlier about comdat.

Respond to review comments: Update to tests to create common LINUX/AIX portions.

clang/lib/CodeGen/ItaniumCXXABI.cpp
4773

The function registered needs to be something more like what createAtExitStub stub creates. Otherwise, the destructor will not be able to reference *this correctly.

As it is, the registered function is currently just the destructor itself:

$ clang++ -target powerpc64-ibm-aix -emit-llvm -S -o - -xc++ -<<<$'struct A { ~A(); }; thread_local A a;' | grep -C2 __pt_atexit_np
define internal void @__cxx_global_var_init() #0 {
entry:
  %0 = call i32 (i32, i32 (i32, ...)*, ...) @__pt_atexit_np(i32 0, i32 (i32, ...)* bitcast (void (%struct.A*)* @_ZN1AD1Ev to i32 (i32, ...)*)) #3
  ret void
}
--
declare void @_ZN1AD1Ev(%struct.A* nonnull align 1 dereferenceable(1)) unnamed_addr #1

declare i32 @__pt_atexit_np(i32, i32 (i32, ...)*, ...)

; Function Attrs: noinline
Return:  0x00:0   Mon Jul 12 15:24:15 2021 EDT
clang/lib/CodeGen/ItaniumCXXABI.cpp
2995–2996

Minor nit: The coding guidelines have been updated for some time to discourage mixed bracing on if/else chains (see example

// Use braces for the `if` block to keep it uniform with the else block.

).

4773

Just noting that the callback function registered by the IBM XL compiler does not make an effort to return 0 (but the documentation says the handler "must" return 0: https://www.ibm.com/docs/en/aix/7.2?topic=p-pthread-atexit-np-subroutine).

clang/lib/CodeGen/ItaniumCXXABI.cpp
2996

Clarify that this statement does not apply if the type is not a class type or (possibly multi-dimenional) array of class type and the variable is also constinit.

clang/lib/CodeGen/ItaniumCXXABI.cpp
2954

The linkage of the init function here should not be hard-coded as weak: If the strong definition of the variable has no dynamic initialization/finalization, then the init function the wrapper functions call should resolve to the empty version created here.

clang/lib/CodeGen/ItaniumCXXABI.cpp
2959

Unfortunately needsDestruction cannot be counted on at this time for some incomplete types, see https://llvm.org/pr51079.

2960–2963

This comment block is a copy/paste from above and is meant to describe a case where only a declaration of the init func is being emitted into the IR.

Respond to review comments: create stub functions, return 0 from function and other fixes

Respond to review comments: update comments and make functions not weak

clang/lib/CodeGen/ItaniumCXXABI.cpp
2959

I think it is okay to leave the code as-is as it will then be fixed when that problem is fixed.

Remove accidental inclusion

clang/lib/CodeGen/CGDeclCXX.cpp
265

Since the function has some specifics about the stub function type and return value behaviour:
s/atexit/__pt_atexit_np/;

clang/lib/CodeGen/ItaniumCXXABI.cpp
2959

An assertion that Init is null should be appropriate here: If it is non-null, then the pre-existing logic above would either be defining the function to be an alias or is declaring the function with the expectation that the definition of the variable is elsewhere.

2961

The linkage should be weak for a variable defined to be weak. For example, the code higher up uses Var->getLinkage() to produce the alias.

2988–2989

This does not say that a thread_local variable of type int that is not constinit does have a guaranteed init routine.

Suggestion:
On AIX, except if constinit and also neither of class type or of (possibly multi-dimensional) array of class type, thread_local vars will have init routines regardless of whether they are const-initialized.

clang/test/CodeGenCXX/cxx11-thread-local.cpp
241

Since the code to generate the stub is new, I believe inspecting the body of the stub in the test would be appropriate.

Respond to review comments, formatting: fix comments, add assertion, fix
linkage, expand test to include body of generated function.

I think we're good after some last updates.

clang/lib/CodeGen/ItaniumCXXABI.cpp
340–343

See suggested change for renaming the parameter name from IgnoreAttrs to InspectInitForWeakDef.

349
2957–2958

The block here is meant to be entered only if there is a definition of the variable (even if weak) in the current translation unit. This is mostly enforced with isEmittedWithConstantInitializer, but that will return true for constinit even if only a declaration is available.

2959

Yes: I think (if the code works correctly), the needsDestruction query here only becomes significant when the type is complete -- and, in that case, we want it to work the way it currently does in relation to classes.

Respond to review comments: Change parameter name and tighten up conditions.

This revision is now accepted and ready to land.Jul 16 2021, 4:28 PM
This revision was landed with ongoing or failed builds.Jul 19 2021, 7:04 AM
This revision was automatically updated to reflect the committed changes.