Do not create LLVM IR `constant`s for objects with dynamic initialisation

[chill]

Summary

When a const-qualified object has a section attribute, that
section is set to read-only and clang outputs a LLVM IR constant
for that object. This is incorrect for dynamically initialised
objects.

For example:

int init() { return 15; }
    
__attribute__((section("SA")))
const int a = init();

a is allocated to a read-only section and is left
unintialised (zero-initialised).

This patch adds checks if an initialiser is a constant expression
and allocates objects to sections as follows:

• const-qualified objects
  • no initialiser or constant initialiser: .rodata
  • dynamic initializer: .bss
• non const-qualified objects
  • no initialiser or dynamic initialiser: .bss
  • constant initializer: .data

(".rodata", ".data", and ".bss" names used just for explanatory
purpose)

[chill]

Once test accidentally copied to the wrong directory, I'm going to fix that.

[rnk]

@hans, do you mind reviewing this?

[hans]

Looks great to me!

[chill]

Moved a test from here to there, and did a few test tweaks, NFC.

[thakis]

This breaks tests on macOS: http://45.33.8.238/macm1/10125/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[haowei]

This CL breaks our mac builders as well. Could you either fix the test or revert the change please?

Error message:

******************** TEST 'Clang :: CodeGenCXX/const-dynamic-init.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /opt/staging/llvm_build/bin/clang -cc1 -internal-isystem /opt/staging/llvm_build/lib/clang/13.0.0/include -nostdsysteminc -triple x86_64-apple-darwin19.6.0 -emit-llvm -o - /opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp | /opt/staging/llvm_build/bin/FileCheck /opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp
--
Exit Code: 2

Command Output (stderr):
--
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:3:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("A")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:9:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("B")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:15:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("C")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:19:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("D")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:23:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("E")))
                       ^
5 errors generated.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /opt/staging/llvm_build/bin/FileCheck /opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp

--

********************

[thakis]

reverted in d881319cc5606baa7668405a296d0960a83a1e4c for now

[chill]

Thanks, I'll have a look.

[chill]

Updated a test to run for x86_64-linux instead of %itanium_abi_triple, to avoid having invalid
syntax for MACH-O sections. The patch itself does not care about section attribute syntax and x86 target
does not even need to be compiled.

[mstorsjo]

I ran into a regression caused by this change, see https://bugs.llvm.org/show_bug.cgi?id=51442. This breaks building Qt tests with LLVM 13.

[ssarda]

This patch is causing issue mentioned in https://discourse.llvm.org/t/sema-section-type-conflict/66000

@chill can you check once?

clang/lib/Sema/SemaDecl.cpp

if (var->getType().isDestructedType()) {
         Diag(CacheCulprit->getExprLoc(), diag::err_thread_dynamic_init)
           << CacheCulprit->getSourceRange();
         if (getLangOpts().CPlusPlus11)
           Diag(var->getLocation(), diag::note_use_thread_local);
       }
     }
   }
 
-  // Apply section attributes and pragmas to global variables.
-  bool GlobalStorage = var->hasGlobalStorage();
-  if (GlobalStorage && var->isThisDeclarationADefinition() &&
-      !inTemplateInstantiation()) {
-    PragmaStack<StringLiteral *> *Stack = nullptr;
-    int SectionFlags = ASTContext::PSF_Read;
-    if (var->getType().isConstQualified())
-      Stack = &ConstSegStack;
-    else if (!var->getInit()) {
-      Stack = &BSSSegStack;
-      SectionFlags |= ASTContext::PSF_Write;
-    } else {
-      Stack = &DataSegStack;
-      SectionFlags |= ASTContext::PSF_Write;
-    }
-    if (const SectionAttr *SA = var->getAttr<SectionAttr>()) {
-      if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
-        SectionFlags |= ASTContext::PSF_Implicit;
-      UnifySection(SA->getName(), SectionFlags, var);
-    } else if (Stack->CurrentValue) {
-      SectionFlags |= ASTContext::PSF_Implicit;
-      auto SectionName = Stack->CurrentValue->getString();
-      var->addAttr(SectionAttr::CreateImplicit(
-          Context, SectionName, Stack->CurrentPragmaLocation,
-          AttributeCommonInfo::AS_Pragma, SectionAttr::Declspec_allocate));
-      if (UnifySection(SectionName, SectionFlags, var))
-        var->dropAttr<SectionAttr>();
-    }
-
-    // Apply the init_seg attribute if this has an initializer.  If the
-    // initializer turns out to not be dynamic, we'll end up ignoring this
-    // attribute.
-    if (CurInitSeg && var->getInit())
-      var->addAttr(InitSegAttr::CreateImplicit(Context, CurInitSeg->getString(),
-                                               CurInitSegLoc,
-                                               AttributeCommonInfo::AS_Pragma));
-  }
 
   if (!var->getType()->isStructureType() && var->hasInit() &&
       isa<InitListExpr>(var->getInit())) {
     const auto *ILE = cast<InitListExpr>(var->getInit());
     unsigned NumInits = ILE->getNumInits();
     if (NumInits > 2)
       for (unsigned I = 0; I < NumInits; ++I) {
         const auto *Init = ILE->getInit(I);
if (NumInits > 2)
                  diag::note_concatenated_string_literal_silence);
           }
           // In any case, stop now.
           break;
         }
       }
   }
 
-  // All the following checks are C++ only.
-  if (!getLangOpts().CPlusPlus) {
-    // If this variable must be emitted, add it as an initializer for the
-    // current module.
-    if (Context.DeclMustBeEmitted(var) && !ModuleScopes.empty())
-      Context.addModuleInitializer(ModuleScopes.back().Module, var);
-    return;
-  }
 
   QualType type = var->getType();
 
   if (var->hasAttr<BlocksAttr>())
     getCurFunction()->addByrefBlockVar(var);
 
   Expr *Init = var->getInit();
+  bool GlobalStorage = var->hasGlobalStorage();
   bool IsGlobal = GlobalStorage && !var->isStaticLocal();
   QualType baseType = Context.getBaseElementType(type);
+  bool HasConstInit = true;
 
   // Check whether the initializer is sufficiently constant.
-  if (!type->isDependentType() && Init && !Init->isValueDependent() &&
+  if (getLangOpts().CPlusPlus && !type->isDependentType() && Init &&
+      !Init->isValueDependent() &&
       (GlobalStorage || var->isConstexpr() ||
        var->mightBeUsableInConstantExpressions(Context))) {
     // If this variable might have a constant initializer or might be usable in
     // constant expressions, check whether or not it actually is now.  We can't
     // do this lazily, because the result might depend on things that change
     // later, such as which constexpr functions happen to be defined.
     SmallVector<PartialDiagnosticAt, 8> Notes;
-    bool HasConstInit;
     if (!getLangOpts().CPlusPlus11) {
       // Prior to C++11, in contexts where a constant initializer is required,
       // the set of valid constant initializers is described by syntactic rules
       // in [expr.const]p2-6.
       // FIXME: Stricter checking for these rules would be useful for constinit /
       // -Wglobal-constructors.
       HasConstInit = checkConstInit();
 
if (HasConstInit) {
         // but nonetheless doesn't require a global constructor.
         if (!checkConstInit())
           Diag(var->getLocation(), diag::warn_global_constructor)
               << Init->getSourceRange();
       }
     }
   }
 
+  // Apply section attributes and pragmas to global variables.
+  if (GlobalStorage && var->isThisDeclarationADefinition() &&
+      !inTemplateInstantiation()) {
+    PragmaStack<StringLiteral *> *Stack = nullptr;
+    int SectionFlags = ASTContext::PSF_Read;
+    if (var->getType().isConstQualified()) {
+      if (HasConstInit)
+        Stack = &ConstSegStack;
+      else {
+        Stack = &BSSSegStack;
+        SectionFlags |= ASTContext::PSF_Write;
+      }
+    } else if (var->hasInit() && HasConstInit) {
+      Stack = &DataSegStack;
+      SectionFlags |= ASTContext::PSF_Write;
+    } else {
+      Stack = &BSSSegStack;
+      SectionFlags |= ASTContext::PSF_Write;
+    }
+    if (const SectionAttr *SA = var->getAttr<SectionAttr>()) {
+      if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
+        SectionFlags |= ASTContext::PSF_Implicit;
+      UnifySection(SA->getName(), SectionFlags, var);
+    } else if (Stack->CurrentValue) {
+      SectionFlags |= ASTContext::PSF_Implicit;
+      auto SectionName = Stack->CurrentValue->getString();
+      var->addAttr(SectionAttr::CreateImplicit(
+          Context, SectionName, Stack->CurrentPragmaLocation,
+          AttributeCommonInfo::AS_Pragma, SectionAttr::Declspec_allocate));
+      if (UnifySection(SectionName, SectionFlags, var))
+        var->dropAttr<SectionAttr>();
+    }
+
+    // Apply the init_seg attribute if this has an initializer.  If the
+    // initializer turns out to not be dynamic, we'll end up ignoring this
+    // attribute.
+    if (CurInitSeg && var->getInit())
+      var->addAttr(InitSegAttr::CreateImplicit(Context, CurInitSeg->getString(),
+                                               CurInitSegLoc,
+                                               AttributeCommonInfo::AS_Pragma));
+  }
+
+  // All the following checks are C++ only.
+  if (!getLangOpts().CPlusPlus) {
+    // If this variable must be emitted, add it as an initializer for the
+    // current module.
+    if (Context.DeclMustBeEmitted(var) && !ModuleScopes.empty())
+      Context.addModuleInitializer(ModuleScopes.back().Module, var);
+    return;
+  }
+
   // Require the destructor.
   if (!type->isDependentType())
     if (const RecordType *recordType = baseType->getAs<RecordType>())
       FinalizeVarWithDestructor(var, recordType);
 
   // If this variable must be emitted, add it as an initializer for the current
   // module.
   if (Context.DeclMustBeEmitted(var) && !ModuleScopes.empty())

clang/test/CodeGenCXX/clang-sections-1.cpp

+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=LLVM
+// RUN: %clang_cc1 -triple x86_64-linux         -S -o - %s | FileCheck %s --check-prefix=ASM
+// Actually, any ELF target would do
+// REQUIRES: x86_64-linux
+
+#pragma clang section bss = "B$$" data = "d@t@" rodata = "r0d@t@"
+
+const int a = 1;
+const int *f() { return &a; }
+
+int init();
+const int b = init();
+
+int c = 2;
+
+int d = init();
+
+int e;
+
+// LLVM: @_ZL1a = internal constant i32 1, align 4 #[[#A:]]
+// LLVM: @_ZL1b = internal global i32 0, align 4 #[[#A]]
+// LLVM: @c = {{.*}}global i32 2, align 4 #[[#A]]
+// LLVM: @d = {{.*}}global i32 0, align 4 #[[#A]]
+// LLVM: @e = {{.*}}global i32 0, align 4 #[[#A]]
+
+// LLVM: attributes #[[#A]] = { "bss-section"="B$$" "data-section"="d@t@" "rodata-section"="r0d@t@" }
+
+// ASM:       .section "r0d@t@","a",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1a:
+// ASM-NEXT:  .long 1
+
+// ASM:       .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1b:
+// ASM-NEXT: .long 0
+
+// ASM:       .section "d@t@","aw",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: c:
+// ASM:       .long 2
+
+// ASM:       .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: d:
+// ASM:       .long 0
+
+// ASM-NOT:   .section
+// ASM-LABEL: e:
+// ASM        .long 0

clang/test/CodeGenCXX/const-dynamic-init.cpp

+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+
+__attribute__((section("A")))
+const int a = 1;
+const int *f() { return &a; }
+// CHECK: @_ZL1a = internal constant i32 1, section "A"
+
+int init();
+__attribute__((section("B")))
+const int b = init();
+// Even if it's const-qualified, it must not be LLVM IR `constant` since it's
+// dynamically initialised.
+// CHECK: @_ZL1b = internal global i32 0, section "B"
+
+__attribute__((section("C")))
+int c = 2;
+// CHECK: @c = {{.*}}global i32 2, section "C"
+
+__attribute__((section("D")))
+int d = init();
+// CHECK: @d = {{.*}}global i32 0, section "D"
+
+__attribute__((section("E")))
+int e;
+// CHECK: @e = {{.*}}global i32 0, section "E", align 4