[Clang][Sema] Capturing section type conflicts between #pragma clang section and section attributes

[pratlucas]

Summary

Conflicting types for the same section name defined in clang section
pragmas and GNU-style section attributes were not properly captured by
Clang's Sema. The lack of diagnostics was caused by the fact the section
specification coming from attributes was handled by Sema as implicit,
even though explicitly defined by the user.

This patch enables the diagnostics for section type conflicts between
those specifications by making sure sections defined in section
attributes are correctly handled as explicit.

[pratlucas]

Fixing "mising clang-format" messages.

[pratlucas]

Removing unnecessary function.

[pratlucas]

Rebasing.

[rnk]

clang/include/clang/AST/ASTContext.h

2984

It seems like there is no need for this to be defined inline, since it is presumably cold code.

clang/test/Sema/pragma-clang-section.c

28

Please add a case like:

const int y __attribute__((section("myrodata.6"))) = 11;

There should be no diagnostics in this case, and I expect myrodata.6 to override the pragma, since it is more specific to the declaration.

[pratlucas]

Updateing test.

[pratlucas]

clang/include/clang/AST/ASTContext.h

2984

The inline's purpose here is just to allow the definition in the header file, avoiding multiple definition errors.

clang/test/Sema/pragma-clang-section.c

28

I've added this case to the test.
The overriding behaviour is already checked by clang/test/CodeGen/clang-sections-attribute.c.

[pratlucas]

Ping.

[rnk]

lgtm

clang/include/clang/AST/ASTContext.h

2984

Yes, I am suggesting that the body be moved out of line, but this is optional. Less code in headers --> faster builds.

[pratlucas]

Addressing review comment.

clang/include/clang/AST/ASTContext.h

 //===- ASTContext.h - Context to hold long-lived AST nodes ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
struct SectionInfo {
     int SectionFlags;
 
     SectionInfo() = default;
     SectionInfo(DeclaratorDecl *Decl,
                 SourceLocation PragmaSectionLocation,
                 int SectionFlags)
         : Decl(Decl), PragmaSectionLocation(PragmaSectionLocation),
           SectionFlags(SectionFlags) {}
+    std::string getSectionReference() const {
+      if (Decl)
+        return "'" + Decl->getName().str() + "'";
+      else
+        return "a prior #pragma section";
+    }
   };
 
   llvm::StringMap<SectionInfo> SectionInfos;
 
   /// Return a new OMPTraitInfo object owned by this context.
   OMPTraitInfo &getNewOMPTraitInfo();
 
 private:
   /// All OMPTraitInfo objects live in this collection, one per
   /// `pragma omp [begin] declare variant` directive.
   SmallVector<OMPTraitInfo *, 4> OMPTraitInfoVector;
 };
 
+/// Insertion operator for diagnostics.
+inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+                                           const ASTContext::SectionInfo& Section) {
+  if (Section.Decl)
+    return DB << Section.Decl;
+  return DB << "a prior #pragma section";
+}
+
 /// Utility function for constructing a nullary selector.
 inline Selector GetNullarySelector(StringRef name, ASTContext &Ctx) {
   IdentifierInfo* II = &Ctx.Idents.get(name);
   return Ctx.Selectors.getSelector(0, &II);
 }
 
 /// Utility function for constructing an unary selector.
 inline Selector GetUnarySelector(StringRef name, ASTContext &Ctx) {
   IdentifierInfo* II = &Ctx.Idents.get(name);
   return Ctx.Selectors.getSelector(1, &II);
 }
 
 } // namespace clang
 
 // operator new and delete aren't allowed inside namespaces.
 
 /// Placement new for using the ASTContext's allocator.
 ///
 /// This placement form of operator new uses the ASTContext's allocator for
 /// obtaining memory.
 ///
 /// IMPORTANT: These are also declared in clang/AST/ASTContextAllocate.h!
 /// Any changes here need to also be made there.
 ///
 /// We intentionally avoid using a nothrow specification here so that the calls
 /// to this operator will not perform a null check on the result -- the
 /// underlying allocator never returns null pointers.
 ///
 /// Usage looks like this (assuming there's an ASTContext 'Context' in scope):
 /// @code
 /// // Default alignment (8)
 /// IntegerLiteral *Ex = new (Context) IntegerLiteral(arguments);

clang/lib/Sema/SemaAttr.cpp

 //===--- SemaAttr.cpp - Semantic Analysis for Attributes ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (Action & PSK_Set) {
     CurrentValue = Value;
     CurrentPragmaLocation = PragmaLocation;
   }
 }
 
 bool Sema::UnifySection(StringRef SectionName,
                         int SectionFlags,
                         DeclaratorDecl *Decl) {
-  auto Section = Context.SectionInfos.find(SectionName);
-  if (Section == Context.SectionInfos.end()) {
+  SourceLocation PragmaLocation;
+  if (auto A = Decl->getAttr<SectionAttr>())
+    if (A->isImplicit())
+      PragmaLocation = A->getLocation();
+  auto SectionIt = Context.SectionInfos.find(SectionName);
+  if (SectionIt == Context.SectionInfos.end()) {
     Context.SectionInfos[SectionName] =
-        ASTContext::SectionInfo(Decl, SourceLocation(), SectionFlags);
+        ASTContext::SectionInfo(Decl, PragmaLocation, SectionFlags);
     return false;
   }
   // A pre-declared section takes precedence w/o diagnostic.
-  if (Section->second.SectionFlags == SectionFlags ||
-      !(Section->second.SectionFlags & ASTContext::PSF_Implicit))
+  const auto &Section = SectionIt->second;
+  if (Section.SectionFlags == SectionFlags ||
+      ((SectionFlags & ASTContext::PSF_Implicit) &&
+       !(Section.SectionFlags & ASTContext::PSF_Implicit)))
     return false;
-  auto OtherDecl = Section->second.Decl;
   Diag(Decl->getLocation(), diag::err_section_conflict)
-      << Decl << OtherDecl;
-  Diag(OtherDecl->getLocation(), diag::note_declared_at)
-      << OtherDecl->getName();
-  if (auto A = Decl->getAttr<SectionAttr>())
-    if (A->isImplicit())
-      Diag(A->getLocation(), diag::note_pragma_entered_here);
-  if (auto A = OtherDecl->getAttr<SectionAttr>())
-    if (A->isImplicit())
-      Diag(A->getLocation(), diag::note_pragma_entered_here);
+      << Decl << Section;
+  if (Section.Decl)
+    Diag(Section.Decl->getLocation(), diag::note_declared_at)
+        << Section.Decl->getName();
+  if (PragmaLocation.isValid())
+      Diag(PragmaLocation, diag::note_pragma_entered_here);
+  if (Section.PragmaSectionLocation.isValid())
+      Diag(Section.PragmaSectionLocation, diag::note_pragma_entered_here);
   return true;
 }
 
 bool Sema::UnifySection(StringRef SectionName,
                         int SectionFlags,
                         SourceLocation PragmaSectionLocation) {
-  auto Section = Context.SectionInfos.find(SectionName);
-  if (Section != Context.SectionInfos.end()) {
-    if (Section->second.SectionFlags == SectionFlags)
+  auto SectionIt = Context.SectionInfos.find(SectionName);
+  if (SectionIt != Context.SectionInfos.end()) {
+    const auto &Section = SectionIt->second;
+    if (Section.SectionFlags == SectionFlags)
       return false;
-    if (!(Section->second.SectionFlags & ASTContext::PSF_Implicit)) {
+    if (!(Section.SectionFlags & ASTContext::PSF_Implicit)) {
       Diag(PragmaSectionLocation, diag::err_section_conflict)
-          << "this" << "a prior #pragma section";
-      Diag(Section->second.PragmaSectionLocation,
-           diag::note_pragma_entered_here);
+          << "this" << Section.getSectionReference();
+      if (Section.Decl)
+        Diag(Section.Decl->getLocation(), diag::note_declared_at)
+            << Section.Decl->getName();
+      if (Section.PragmaSectionLocation.isValid())
+        Diag(Section.PragmaSectionLocation, diag::note_pragma_entered_here);
       return true;
     }
   }
   Context.SectionInfos[SectionName] =
       ASTContext::SectionInfo(nullptr, PragmaSectionLocation, SectionFlags);
   return false;
 }
 

clang/lib/Sema/SemaDecl.cpp

 //===--- SemaDecl.cpp - Semantic Analysis for Declarations ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (var->getTLSKind() == VarDecl::TLS_Static) {
     }
   }
 
   // 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_Implicit | ASTContext::PSF_Read;
+    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 (Stack->CurrentValue && !var->hasAttr<SectionAttr>())
+    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, Stack->CurrentValue->getString(),
-          Stack->CurrentPragmaLocation, AttributeCommonInfo::AS_Pragma,
-          SectionAttr::Declspec_allocate));
-    if (const SectionAttr *SA = var->getAttr<SectionAttr>())
-      if (UnifySection(SA->getName(), SectionFlags, var))
+          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));

clang/test/Sema/pragma-clang-section.c

 #pragma clang section relro "relro.2"   // expected-error {{expected '=' following '#pragma clang section relro'}}
 #pragma clang section bss="" data="" rodata="" text="" more //expected-error {{expected one of [bss|data|rodata|text|relro] section kind in '#pragma clang section'}}
 
 #pragma clang section bss="mybss.3" data="mybss.3" // expected-error {{this causes a section type conflict with a prior #pragma section}} expected-note {{#pragma entered here}} expected-note {{#pragma entered here}}
 #pragma clang section rodata="mydata.1" // expected-error {{this causes a section type conflict with a prior #pragma section}}
 #pragma clang section bss="myrodata.1" // expected-error {{this causes a section type conflict with a prior #pragma section}}
 #pragma clang section text="mybss.3" // expected-error {{this causes a section type conflict with a prior #pragma section}}
 
+#pragma clang section rodata="myrodata.4" // expected-note {{#pragma entered here}}
+int x __attribute__((section("myrodata.4"))); // expected-error {{'x' causes a section type conflict with a prior #pragma section}}
+const int y __attribute__((section("myrodata.5"))) = 10; // expected-note {{declared here}}
+#pragma clang section data="myrodata.5" // expected-error {{this causes a section type conflict with 'y'}}
+
 int a;