[C++20] [Modules] Don't generate global ctors/dtors for variables which are available externally

[ChuanqiXu]

Summary

Closes https://github.com/llvm/llvm-project/issues/59765.

Currently we will generate the global ctor/dtor for variables in importing modules. It will cause multiple initialization/destructions. It makes no sense. This patch tries to not generate global ctor/dtor for variables which are available externally. Note that the variables in header units and clang modules won't be available externally by default.

[iains]

your test case only covers itanium ABI - does the process still work properly for non-itanium ABIs?
.. if I remember correctly, the ctor/dtors are all run from the top level module (i.e. it pulls in the imported ones and constructs a composite ctor),

[ChuanqiXu]

In D140867#4022603, @iains wrote:

your test case only covers itanium ABI - does the process still work properly for non-itanium ABIs?

Yeah, CodeGenModule::EmitGlobalVarDefinition should be ABI independent. But we only support the initializer for modules in itanium ABI.

.. if I remember correctly, the ctor/dtors are all run from the top level module (i.e. it pulls in the imported ones and constructs a composite ctor),

Yes, the ctor/dtors are run from the top level. (To avoid confusing, given A import B and B import C, let's call C as the top level module).

[iains]

OK. So I've been thinking about this some more and ISTM that if the external source represents a Header Unit, then that has no stand-alone object or initialiser.

Header Units are permitted to contain (implicitly exported) declarations with internal linkage (although after D140261 we should reject exported definitions with external linkage).

Did we not fix a problem where ctors for static objects in headers were allowed (the issue with iostream?)

ISTM that if we permit static objects at file scope with dynamic inits, to appear in a header unit - then we would have to special-case external sources that are header units or those ctors would not be run (and we would again break iostream). Perhaps my memory is not right on this tho.

[ChuanqiXu]

Rewording external sources to available externally.

[ChuanqiXu]

@iains Oh, I used the incorrect word. I should use available externally. Header Units are external sources but the declarations in header units won't be available externally. I added a test to show this.

---

(following off is about the idea of available externally. You can ignore it if you are not interested)

The idea of available externally is originally the idea of LLVM. It means that we import a definition from another LLVM Module and the definition is available externally to current LLVM Modules. So that the optimizer can optimize away the available external definitions at any time.

Then modules in clang borrow the idea of available externally to speedup compilation. When clang generate codes, if clang found a definition is defined in a named modules (or object pcm file, which is another extension mode of clang modules. I am not sure if this mode is used or maintained), clang will mark the definition as available externally. So simply, the declarations in normal clang modules and header units won't be available externally. So this patch won't affect normal clang modules and header units.

[iains]

In D140867#4027628, @ChuanqiXu wrote:

@iains Oh, I used the incorrect word. I should use available externally. Header Units are external sources but the declarations in header units won't be available externally. I added a test to show this.

Yeah, that explains it and the patch LGTM (but please allow @dblaikie a few days to comment, if he wants to)

---

(following off is about the idea of available externally. You can ignore it if you are not interested)

The idea of available externally is originally the idea of LLVM. It means that we import a definition from another LLVM Module and the definition is available externally to current LLVM Modules. So that the optimizer can optimize away the available external definitions at any time.

Then modules in clang borrow the idea of available externally to speedup compilation. When clang generate codes, if clang found a definition is defined in a named modules (or object pcm file, which is another extension mode of clang modules. I am not sure if this mode is used or maintained), clang will mark the definition as available externally. So simply, the declarations in normal clang modules and header units won't be available externally. So this patch won't affect normal clang modules and header units.

sure, I am familiar with the externally available process, we should probably check on the cases that might not be maintained.

[ChuanqiXu]

sure, I am familiar with the externally available process, we should probably check on the cases that might not be maintained.

Yeah, it looks like @dblaikie bring the object pcm mode to clang. If it is not used now, I think we can remove the mode.

[dblaikie]

In D140867#4027965, @ChuanqiXu wrote:

sure, I am familiar with the externally available process, we should probably check on the cases that might not be maintained.

Yeah, it looks like @dblaikie bring the object pcm mode to clang. If it is not used now, I think we can remove the mode.

Modular code generation/debug info for Clang Header modules? It was never deployed, but I think it's probably worth keeping around for experimentation/further investigation. I think it might be valuable with some heuristics tweaks to not try to home/available_externally generated code like protobufs, maybe.

[ChuanqiXu]

In D140867#4029623, @dblaikie wrote:

In D140867#4027965, @ChuanqiXu wrote:

sure, I am familiar with the externally available process, we should probably check on the cases that might not be maintained.

Yeah, it looks like @dblaikie bring the object pcm mode to clang. If it is not used now, I think we can remove the mode.

Modular code generation/debug info for Clang Header modules? It was never deployed, but I think it's probably worth keeping around for experimentation/further investigation. I think it might be valuable with some heuristics tweaks to not try to home/available_externally generated code like protobufs, maybe.

Got it.

[Jake-Egan]

Hi, this test is failing on AIX, could you take a look please? https://lab.llvm.org/buildbot/#/builders/214/builds/5242/steps/6/logs/FAIL__Clang__pr59765-modules-global-ctor-dtor_cppm

[ChuanqiXu]

In D140867#4036239, @Jake-Egan wrote:

Hi, this test is failing on AIX, could you take a look please? https://lab.llvm.org/buildbot/#/builders/214/builds/5242/steps/6/logs/FAIL__Clang__pr59765-modules-global-ctor-dtor_cppm

I fixed the failure by restricting the targets in https://github.com/llvm/llvm-project/commit/9cb1298dcc85c357eb97ed0bdc3c82c5d011f6f6. And the best approach is to add a separate test for AIX but I don't have an AIX machine. You can add it if you are interested.

[Jake-Egan]

In D140867#4038388, @ChuanqiXu wrote:

In D140867#4036239, @Jake-Egan wrote:

Hi, this test is failing on AIX, could you take a look please? https://lab.llvm.org/buildbot/#/builders/214/builds/5242/steps/6/logs/FAIL__Clang__pr59765-modules-global-ctor-dtor_cppm

I fixed the failure by restricting the targets in https://github.com/llvm/llvm-project/commit/9cb1298dcc85c357eb97ed0bdc3c82c5d011f6f6. And the best approach is to add a separate test for AIX but I don't have an AIX machine. You can add it if you are interested.

Thanks for looking into it. Unfortunately the test is still failing on AIX after this commit. Which is odd because your fix does make sense to me. I'm not sure why the test is still being run on AIX. https://lab.llvm.org/buildbot/#/builders/214/builds/5267

[ChuanqiXu]

In D140867#4040590, @Jake-Egan wrote:

In D140867#4038388, @ChuanqiXu wrote:

In D140867#4036239, @Jake-Egan wrote:

Hi, this test is failing on AIX, could you take a look please? https://lab.llvm.org/buildbot/#/builders/214/builds/5242/steps/6/logs/FAIL__Clang__pr59765-modules-global-ctor-dtor_cppm

I fixed the failure by restricting the targets in https://github.com/llvm/llvm-project/commit/9cb1298dcc85c357eb97ed0bdc3c82c5d011f6f6. And the best approach is to add a separate test for AIX but I don't have an AIX machine. You can add it if you are interested.

Thanks for looking into it. Unfortunately the test is still failing on AIX after this commit. Which is odd because your fix does make sense to me. I'm not sure why the test is still being run on AIX. https://lab.llvm.org/buildbot/#/builders/214/builds/5267

Weird. I compiled PowerPC target locally and change the test into:

// RUN: %clang_cc1 -std=c++20 --target=powerpc-ibm-aix-xcoff %t/M.cppm -triple %itanium_abi_triple -emit-module-interface -o %t/M.pcm
// RUN: %clang_cc1 -std=c++20 --target=powerpc-ibm-aix-xcoff %t/M.pcm -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %t/M.cppm
// RUN: %clang_cc1 -std=c++20 --target=powerpc-ibm-aix-xcoff %t/Use.cpp -fprebuilt-module-path=%t -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %t/Use.cpp

Then when I run the test, it tells me it is unsupported (as expect).

Would you like to take a double look?

[Jake-Egan]

Would you like to take a double look?

Yes, it still fails on the bot and on my local machine. I've added an XFAIL to the test for now just to get the AIX bot green.

[ChuanqiXu]

In D140867#4042351, @Jake-Egan wrote:

Would you like to take a double look?

Yes, it still fails on the bot and on my local machine. I've added an XFAIL to the test for now just to get the AIX bot green.

Weird. Maybe you can find a method by adding REQUIRES or NOT-SUPPORTED and test it locally. If it goes well then you can upstream the change.

[Jake-Egan]

In D140867#4042405, @ChuanqiXu wrote:

In D140867#4042351, @Jake-Egan wrote:

Would you like to take a double look?

Yes, it still fails on the bot and on my local machine. I've added an XFAIL to the test for now just to get the AIX bot green.

Weird. Maybe you can find a method by adding REQUIRES or NOT-SUPPORTED and test it locally. If it goes well then you can upstream the change.

It seems that it's because the test fails when running on AIX - not just when it's being targeted.

// REQUIRES: x86-registered-target,aarch64-registered-target

The above doesn't stop the test from being run on AIX. So, I can add:

// UNSUPPORTED: system-aix

But do you want to keep the REQUIRES for other targets?

[ChuanqiXu]

In D140867#4043763, @Jake-Egan wrote:

In D140867#4042405, @ChuanqiXu wrote:

In D140867#4042351, @Jake-Egan wrote:

Would you like to take a double look?

Yes, it still fails on the bot and on my local machine. I've added an XFAIL to the test for now just to get the AIX bot green.

Weird. Maybe you can find a method by adding REQUIRES or NOT-SUPPORTED and test it locally. If it goes well then you can upstream the change.

It seems that it's because the test fails when running on AIX - not just when it's being targeted.

// REQUIRES: x86-registered-target,aarch64-registered-target

The above doesn't stop the test from being run on AIX. So, I can add:

// UNSUPPORTED: system-aix

But do you want to keep the REQUIRES for other targets?

The REQUIRES are added for AIX, so maybe it is ok to abandon them temporarily and add them back once we find other similar failures.

clang/docs/ReleaseNotes.rst

- Fix an issue where -frewrite-includes generated line control directives with
   incorrect line numbers in some cases when a header file used an end of line
   character sequence that differed from the primary source file.
   `Issue 59736 <https://github.com/llvm/llvm-project/issues/59736>`_
 - In C mode, when ``e1`` has ``__attribute__((noreturn))`` but ``e2`` doesn't,
   ``(c ? e1 : e2)`` is no longer considered noreturn.
   `Issue 59792 <https://github.com/llvm/llvm-project/issues/59792>`_
 - Fix an issue that makes Clang crash on lambda template parameters. This fixes
   `Issue 57960 <https://github.com/llvm/llvm-project/issues/57960>`_
+- Fix issue that the standard C++ modules importer will call global
+  constructor/destructor for the global varaibles in the importing modules.
+  This fixes `Issue 59765 <https://github.com/llvm/llvm-project/issues/59765>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
   // If this is OpenMP device, check if it is legal to emit this global
   // normally.
   if (LangOpts.OpenMPIsDevice && OpenMPRuntime &&
       OpenMPRuntime->emitTargetGlobalVariable(D))
     return;
 
   llvm::TrackingVH<llvm::Constant> Init;
   bool NeedsGlobalCtor = false;
+  // Whether the definition of the variable is available externally.
+  // If yes, we shouldn't emit the GloablCtor and GlobalDtor for the variable
+  // since this is the job for its original source.
+  bool IsDefinitionAvailableExternally =
+      getContext().GetGVALinkageForVariable(D) == GVA_AvailableExternally;
   bool NeedsGlobalDtor =
+      !IsDefinitionAvailableExternally &&
       D->needsDestruction(getContext()) == QualType::DK_cxx_destructor;
 
   const VarDecl *InitDecl;
   const Expr *InitExpr = D->getAnyInitializer(InitDecl);
 
   Optional<ConstantEmitter> emitter;
 
   // CUDA E.2.4.1 "__shared__ variables cannot have an initialization
if (!Initializer) {
       QualType T = InitExpr->getType();
       if (D->getType()->isReferenceType())
         T = D->getType();
 
       if (getLangOpts().CPlusPlus) {
         if (InitDecl->hasFlexibleArrayInit(getContext()))
           ErrorUnsupported(D, "flexible array initializer");
         Init = EmitNullConstant(T);
+
+        if (!IsDefinitionAvailableExternally)
           NeedsGlobalCtor = true;
       } else {
         ErrorUnsupported(D, "static initializer");
         Init = llvm::UndefValue::get(getTypes().ConvertType(T));
       }
     } else {
       Init = Initializer;
       // We don't need an initializer, so remove the entry for the delayed
       // initializer position (just in case this entry was delayed) if we

clang/test/CodeGenCXX/pr59765-modules-global-ctor-dtor.cppm

+// https://github.com/llvm/llvm-project/issues/59765
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -triple %itanium_abi_triple -emit-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/M.pcm -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %t/M.cppm
+// RUN: %clang_cc1 -std=c++20 %t/Use.cpp -fprebuilt-module-path=%t -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %t/Use.cpp
+//
+// Check that the behavior of header units is normal as headers.
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple -xc++-user-header %t/foo.h -emit-header-unit -o %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 %t/UseHU.cpp -fmodule-file=%t/foo.pcm -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %t/UseHU.cpp
+
+//--- M.cppm
+export module M;
+export class A {
+public:
+    A();
+    ~A();
+    void use();
+};
+export A a;
+
+// CHECK: @_ZW1M1a = {{.*}}global %class.A zeroinitializer
+// CHECK: define{{.*}}void @__cxx_global_var_init()
+// CHECK-NEXT: entry:
+// CHECK-NEXT: call{{.*}}void @_ZNW1M1AC1Ev({{.*}}@_ZW1M1a
+// CHECK-NEXT: call{{.*}}i32 @__cxa_atexit(ptr @_ZNW1M1AD1Ev, ptr @_ZW1M1a
+
+//--- Use.cpp
+import M;
+void use() {
+    a.use();
+}
+
+// CHECK-NOT: @_ZNW1M1AC1Ev
+// CHECK-NOT: @_ZNW1M1AD1Ev
+
+//--- foo.h
+class A {
+public:
+    A();
+    ~A();
+    void use();
+};
+A a;
+
+//--- UseHU.cpp
+import "foo.h";
+void use() {
+    a.use();
+}
+
+// CHECK: @a = {{.*}}global %class.A zeroinitializer
+// CHECK: define{{.*}}void @__cxx_global_var_init()
+// CHECK-NEXT: entry:
+// CHECK-NEXT: call{{.*}}void @_ZN1AC1Ev({{.*}}@a
+// CHECK-NEXT: call{{.*}}i32 @__cxa_atexit(ptr @_ZN1AD1Ev, ptr @a