Emit section information for extern variables.

[eandrews]

Summary

Currently, if _attribute_((section())) is used for extern variables, section information is not emitted in generated IR when the variables are used. This is expected since sections are not generated for external linkage objects. However NiosII requires this information as it uses special GP-relative accesses for any objects that use attribute section (.sdata). GCC keeps this attribute in middle-end.

This change emits the section information for all targets.

[efriedma]

Please propose a patch for LangRef clarifying what exactly it means to have a section attribute on a declaration.

test/CodeGenCXX/extern-section-attribute.cpp

1 ↗

(On Diff #110263)

Not sure it really adds anything to run this with four different RUN lines.

[efriedma]

lib/CodeGen/CodeGenModule.cpp

2434 ↗

(On Diff #110263)

Why do you specifically check "D->hasExternalStorage() && !D->isThisDeclarationADefinition()", instead of just setting the section unconditionally?

[eandrews]

lib/CodeGen/CodeGenModule.cpp

2434 ↗

(On Diff #110263)

I noticed that you enter GetOrCreateLLVMGlobal( ) whenever the extern variable is declared as well as when it is defined. The flow of the program is different in each case. When the variable is defined, it also enters EmitGlobalVarDefinition( ). There is existing code handling section information here. I added the check in GetOrCreateLLVMGlobal( ) so the block gets skipped for variable definition, since its already handled elsewhere.

[eandrews]

As per review comments, removed extra RUNS in lit test.

[erichkeane]

@efriedma : LangRef changed in this review: https://reviews.llvm.org/D36712

[eandrews]

LangRef has been updated and accepted. You can find it here - https://reviews.llvm.org/rL311459

[eandrews]

*ping*

[erichkeane]

Hey @efriedma : Did you get a chance to take a look at this? She got the langref changes in, so it is just waiting on this.

[efriedma]

lib/CodeGen/CodeGenModule.cpp

2434 ↗

(On Diff #110263)

I would rather just call setSection unconditionally here, as opposed to trying to guess whether the global will eventually be defined.

I'm also sort of concerned the behavior here could be weird if a section attribute is added on a redeclaration (e.g. what happens if you write extern int x; int y = &x; extern int x __attribute((section("foo")));)... maybe we should emit a warning?

[eandrews]

I've updated the patch based on review comments. The changes include setting section unconditionally for extern variables and emitting a warning if section attribute is specified on a redeclaration.

[eandrews]

lib/CodeGen/CodeGenModule.cpp

2434 ↗

(On Diff #110263)

@efriedma I modified the patch to emit a warning in the scenario you mentioned. A warning is also emitted for the following -

extern int x;  int *y=&x; int x __attribute__((section("foo")));

I thought it made sense to emit the warning for the latter as well. Should I restrict the warning to just redeclarations (without definition) instead?

[efriedma]

Should I restrict the warning to just redeclarations (without definition) instead?

I think just redeclarations. The pattern of a declaration without a section followed by a definition with a section is common, and not really harmful.

lib/Sema/SemaDecl.cpp

2613 ↗

(On Diff #116211)

Probably don't want to check isUsed(); that's going to seem inconsistent to users.

[eandrews]

I've modified the patch to emit a warning for re-declarations only. I also removed the isUsed check.

[efriedma]

LGTM

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 
 def err_only_annotate_after_access_spec : Error<
   "access specifier can only have annotation attributes">;
 
 def err_attribute_section_invalid_for_target : Error<
   "argument to 'section' attribute is not valid for this target: %0">;
 def warn_mismatched_section : Warning<
   "section does not match previous declaration">, InGroup<Section>;
+def warn_attribute_section_on_redeclaration : Warning<
+  "section attribute is specified on redeclared variable">, InGroup<Section>;
 
 def err_anonymous_property: Error<
   "anonymous property is not supported">;
 def err_property_is_variably_modified : Error<
   "property %0 has a variably modified type">;
 def err_no_accessor_for_property : Error<
   "no %select{getter|setter}0 defined for property %1">;
 def err_cannot_find_suitable_accessor : Error<

cfe/trunk/lib/CodeGen/CodeGenModule.cpp

if (D) {
     }
 
     // If required by the ABI, treat declarations of static data members with
     // inline initializers as definitions.
     if (getContext().isMSStaticDataMemberInlineDefinition(D)) {
       EmitGlobalVarDefinition(D);
     }
 
+    // Emit section information for extern variables.
+    if (D->hasExternalStorage()) {
+      if (const SectionAttr *SA = D->getAttr<SectionAttr>())
+        GV->setSection(SA->getName());
+    }
+
     // Handle XCore specific ABI requirements.
     if (getTriple().getArch() == llvm::Triple::xcore &&
         D->getLanguageLinkage() == CLanguageLinkage &&
         D->getType().isConstant(Context) &&
         isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
       GV->setSection(".cp.rodata");
 
     // Check if we a have a const declaration with an initializer, we may be

cfe/trunk/lib/Sema/SemaDecl.cpp

if (const auto *OldAbiTagAttr = Old->getAttr<AbiTagAttr>()) {
         }
       }
     } else {
       Diag(NewAbiTagAttr->getLocation(), diag::err_abi_tag_on_redeclaration);
       Diag(Old->getLocation(), diag::note_previous_declaration);
     }
   }
 
+  // This redeclaration adds a section attribute.
+  if (New->hasAttr<SectionAttr>() && !Old->hasAttr<SectionAttr>()) {
+    if (auto *VD = dyn_cast<VarDecl>(New)) {
+      if (VD->isThisDeclarationADefinition() != VarDecl::Definition) {
+        Diag(New->getLocation(), diag::warn_attribute_section_on_redeclaration);
+        Diag(Old->getLocation(), diag::note_previous_declaration);
+      }
+    }
+  }
+
   if (!Old->hasAttrs())
     return;
 
   bool foundAny = New->hasAttrs();
 
   // Ensure that any moving of objects within the allocated map is done before
   // we process them.
   if (!foundAny) New->setAttrs(AttrVec());

cfe/trunk/test/Sema/attr-section.c

void test() {
   __attribute__((section("NEAR,x"))) static int n2; // ok.
 }
 
 // pr9356
 void __attribute__((section("foo,zed"))) test2(void); // expected-note {{previous attribute is here}}
 void __attribute__((section("bar,zed"))) test2(void) {} // expected-warning {{section does not match previous declaration}}
 
 enum __attribute__((section("NEAR,x"))) e { one }; // expected-error {{'section' attribute only applies to functions, methods, properties, and global variables}}
+
+extern int a; // expected-note {{previous declaration is here}}
+int *b = &a;
+extern int a __attribute__((section("foo,zed"))); // expected-warning {{section attribute is specified on redeclared variable}}