Implement __attribute__((internal_linkage))

[eugenis]

Summary

The attrubite is applicable to functions and variables and changes the linkage of the subject to internal.

Following the proposal in http://lists.llvm.org/pipermail/cfe-dev/2015-October/045580.html

[majnemer]

include/clang/Basic/Attr.td

2114

Space between Function and Var.

lib/Sema/SemaDeclAttr.cpp

1580–1585

Why is this here? You've already got logic for this in handleInternalLinkageAttr

4563–4564

I'd expect a more serious diagnostic for a mismatch (an error) due to the nature of the attribute (namely, it's ABI implications).

[eugenis]

lib/Sema/SemaDeclAttr.cpp

1580–1585

I think because the attributes may appear in different order. See for example the cross-checks between AlwaysInlineAttr and OptimizeNoneAttr. I did not test this.

[eugenis]

This new version supports attribute((internal_linkage)) on classes and even namespaces!

lib/Sema/SemaDeclAttr.cpp

1580–1585

Confirmed, we need both. See the test.

[majnemer]

No diagnostic is issued for the following C test case:

int x __attribute__((internal_linkage));
int x __attribute__((common));
int *f() { return &x; }

include/clang/Basic/Attr.td

2112

Why doesn't this use Subjects ?

[eugenis]

In D13925#276626, @majnemer wrote:

No diagnostic is issued for the following C test case:

int x __attribute__((internal_linkage));
int x __attribute__((common));
int *f() { return &x; }

Thanks for noticing! Added missing attribute merging logic in SemaAttr.cpp.

include/clang/Basic/Attr.td

2112

Because the list of subjects is long and unusual, and Subjects field does not support arbitrary lists - all combinations must have a specific diagnostic line elsewhere in the code.

This is checked in handleInternalLinkageAttr instead.

[eugenis]

Added a [[clang::internal_linkage]] spelling to the attribute.
Added tests for namespace re-declarations with and without the attribute.

[rnk]

I'm happy with this. Richard, what do you think?

[rsmith]

include/clang/Basic/Attr.td

2112

InheritableAttr doesn't seem right for the case when this is applied to a namespace.

lib/AST/Decl.cpp

1358–1363

We shouldn't allow the linkage to be changed after the first declaration. What cases cause problems here?

lib/Sema/Sema.cpp

539

Hmm, what should isExternallyVisible return for a declaration with this attribute? Presumably it should be false...

[aaron.ballman]

I would like to hold off on adding the namespace attribute. There were persuasive reasons to not have attributes on namespaces that was discussed in EWG in Kona, and this is a feature we could add on later if there's sufficient design consensus.

include/clang/Basic/Attr.td

2112

Long and unusual is not a problem; that's why you can manually specify the diagnostic enum. This should be using Subjects unless it's reallllly onerous (or impossible).

include/clang/Basic/AttrDocs.td

1619

Some examples would be nice, as well as an explanation as to why someone might want to do this.

lib/Sema/SemaDeclAttr.cpp

3312

Please use checkAttrMutualExclusion instead (and augment it with the note_conflicting_attribute).

3326

Please use checkAttrMutualExclusion instead.

test/SemaCXX/internal_linkage.cpp

7

This does not seem like a useful diagnostic as it doesn't tell the user *why* it was ignored.

26

Missing tests for internal_linkage accepting an argument (which should diagnose).
Missing tests for what happens with forward declarations and redeclarations that do not have matching attribute specifiers.
Missing tests for using the C++ spelling of the attribute.

[eugenis]

Disabled the new attribute on namespaces.

include/clang/Basic/Attr.td

2112

This attribute no longer applies to namespaces.

include/clang/Basic/AttrDocs.td

1619

Extended the explanation.

lib/AST/Decl.cpp

1358–1363

Isn't it the same case as in the comment above with static following an extern in microsoft extensions?

lib/Sema/Sema.cpp

539

Fixed by adding an attribute check to getLVForDecl (in addition to computeLVForDecl).

[eugenis]

One more test for forward declarations.

[rsmith]

include/clang/Basic/AttrDocs.td

1620

Missing period at end of sentence.

1620

Please also say what it means if the attribute is applied to a class.

lib/AST/Decl.cpp

635–643

Dead code?

1358–1363

We should not introduce another case where the linkage of an entity can change after its first declaration. It seems reasonable to require this attribute to be on the first declaration of the function.

lib/Sema/SemaDeclAttr.cpp

3312

Aaron, could we move the mutual exclusion functionality to TableGen? (Separately from this patch, of course.) It looks like there are now 6 attributes that could use the "simple attribute" codepath if we did so.

[eugenis]

lib/AST/Decl.cpp

635–643

Right. Removed.

1358–1363

This is strange, I can no longer trigger this code path.
I wonder if the change that added an attribute check to isExternallyVisible made this special case unnecessary?
Reverting this chunk.

[aaron.ballman]

I would like to see one more test, just to make sure that a Var subject doesn't also allow it on a parameter:

void f(int a [[clang::internal_linkage]]);

Aside from that, LGTM!

[rsmith]

test/CodeGenCXX/attribute_internal_linkage.cpp

35–36

We should reject this. We're just causing problems for ourselves if we allow the linkage to change on a redeclaration.

[eugenis]

Hm, the current implementation allows all of the following:

void f(int a [[clang::internal_linkage]]) { // 1

int b [[clang::internal_linkage]];   // 2
static int c [[clang::internal_linkage]];  // 3

}

I'll fix (1). Is it OK to allow (2) and (3)? The attribute has no effect because the declarations already have internal linkage, so I'd say it behaves as documented.

[rnk]

This is an interesting test case, though:

inline int foo() {
  static int __attribute__((internal_linkage)) x;
  return x++;
}

If foo gets inlined, those call sites will use and update 'x'. If foo is not inlined, one definition of foo will win, and every caller will use its version of 'x'.

We could emit a warning, but I kind of don't care. If you're using internal_linkage, you are operating outside the rules of C++. You're expecting multiple copies of these things to be emitted.

[eugenis]

How do I check if a Var is a local variable?

test/CodeGenCXX/attribute_internal_linkage.cpp

36–37

OK, done. Still allowing forward declaration of a class w/o the attribute though, it looks harmless.

[eugenis]

In D13925#281349, @eugenis wrote:

Hm, the current implementation allows all of the following:

void f(int a [[clang::internal_linkage]]) { // 1

int b [[clang::internal_linkage]];   // 2
static int c [[clang::internal_linkage]];  // 3

}

I'll fix (1). Is it OK to allow (2) and (3)? The attribute has no effect because the declarations already have internal linkage, so I'd say it behaves as documented.

Done. Warning on (1) and (2), silently accepting (3).

[eugenis]

test/SemaCXX/internal_linkage.cpp

24

Btw, this triggers on libc++ (if macro-replacing always_inline with internal_linkage) 400 times. No problem, should be easy to clean up.

[aaron.ballman]

lib/Sema/SemaDecl.cpp

6609 ↗

(On Diff #39249)

Is there a reason this change cannot go into SemaDeclAttr.cpp instead? That way we don't bother to attach the attribute in the first place.

lib/Sema/SemaDeclAttr.cpp

3328

Why is this dropping AlwaysInlineAttr instead of returning a nullptr?

[eugenis]

Added the new warning to a -W group.

lib/Sema/SemaDeclAttr.cpp

3328

A copy/paste error. Fixed.

[aaron.ballman]

LGTM, thank you!

include/clang/Basic/DiagnosticSemaKinds.td

4090 ↗

(On Diff #39389)

Good catch!

[eugenis]

Richard, are you OK with this?

[eugenis]

Rebase.

[eugenis]

r252648.
Thanks everyone for the very detailed review!

include/clang/Basic/Attr.td

 }
 
 def OMPThreadPrivateDecl : InheritableAttr {
   // This attribute has no spellings as it is only ever created implicitly.
   let Spellings = [];
   let SemaHandler = 0;
   let Documentation = [Undocumented];
 }
+
+def InternalLinkage : InheritableAttr {
+  let Spellings = [GCC<"internal_linkage">];
+  let Documentation = [InternalLinkageDocs];
+}

include/clang/Basic/AttrDocs.td

 def NoAliasDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``noalias`` attribute indicates that the only memory accesses inside
 function are loads and stores from objects pointed to by its pointer-typed
 arguments, with arbitrary offsets.
   }];
 }
+
+def InternalLinkageDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``internal_linkage`` attribute changes the linkage type of the declaration to internal.
+  }];
+}

lib/AST/Decl.cpp

if (Function->getCanonicalDecl()->getStorageClass() == SC_Static)
       return LinkageInfo(InternalLinkage, DefaultVisibility, false);
   } else if (const auto *IFD = dyn_cast<IndirectFieldDecl>(D)) {
     //   - a data member of an anonymous union.
     const VarDecl *VD = IFD->getVarDecl();
     assert(VD && "Expected a VarDecl in this IndirectFieldDecl!");
     return getLVForNamespaceScopeDecl(VD, computation);
   }
   assert(!isa<FieldDecl>(D) && "Didn't expect a FieldDecl!");
 
+  for (const DeclContext *DC = D->getDeclContext();
+       !isa<TranslationUnitDecl>(DC); DC = DC->getParent()) {
+    const NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC);
+    if (ND && ND->getAttr<InternalLinkageAttr>())
+      return LinkageInfo::internal();
+  }
+
   if (D->isInAnonymousNamespace()) {
     const VarDecl *Var = dyn_cast<VarDecl>(D);
     const FunctionDecl *Func = dyn_cast<FunctionDecl>(D);
     if ((!Var || !isFirstInExternCContext(Var)) &&
         (!Func || !isFirstInExternCContext(Func)))
       return LinkageInfo::uniqueExternal();
   }
 
   // Set up the defaults.
while (Record && Record->isLambda()) {
     Record = dyn_cast_or_null<CXXRecordDecl>(
       Record->getParent()->getParent());
   }
   return Ret;
 }
 
 static LinkageInfo computeLVForDecl(const NamedDecl *D,
                                     LVComputationKind computation) {
+  // Internal_linkage attribute overrides other considerations.
+  if (D->hasAttr<InternalLinkageAttr>())
+    return LinkageInfo::internal();
+
   // Objective-C: treat all Objective-C declarations as having external
   // linkage.
   switch (D->getKind()) {
     default:
       break;
     case Decl::ParmVar:
       return LinkageInfo::none();
     case Decl::TemplateTemplateParm: // count these as external
for (auto I : D->redecls()) {
       NamedDecl *T = cast<NamedDecl>(I);
       if (T == D)
         continue;
       if (!T->isInvalidDecl() && T->hasCachedLinkage()) {
         Old = T;
         break;
       }
     }
-    assert(!Old || Old->getCachedLinkage() == D->getCachedLinkage());
+    // Linkages may also differ if one of the declarations has
+    // InternalLinkageAttr.
+    assert(!Old || Old->getCachedLinkage() == D->getCachedLinkage() ||
+           (Old->hasAttr<InternalLinkageAttr>() !=
+            D->hasAttr<InternalLinkageAttr>()));
 #endif
 
     return LV;
   }
 };
 }
 
 static LinkageInfo getLVForDecl(const NamedDecl *D,
                                 LVComputationKind computation) {

lib/Sema/Sema.cpp

for (SmallVectorImpl<std::pair<NamedDecl *, SourceLocation> >::iterator
 
     if (ND->hasAttr<DLLImportAttr>() || ND->hasAttr<DLLExportAttr>()) {
       // An exported function will always be emitted when defined, so even if
       // the function is inline, it doesn't have to be emitted in this TU. An
       // imported function implies that it has been exported somewhere else.
       continue;
     }
 
-    if (!ND->isExternallyVisible()) {
+    if (!ND->isExternallyVisible() || ND->hasAttr<InternalLinkageAttr>()) {
       S.Diag(ND->getLocation(), diag::warn_undefined_internal)
         << isa<VarDecl>(ND) << ND;
     } else {
       assert(cast<FunctionDecl>(ND)->getMostRecentDecl()->isInlined() &&
              "used object requires definition but isn't inline or internal?");
       S.Diag(ND->getLocation(), diag::warn_undefined_inline) << ND;
     }
     if (I->second.isValid())

lib/Sema/SemaDeclAttr.cpp

static void handleRestrictAttr(Sema &S, Decl *D, const AttributeList &Attr) {
 
   S.Diag(Attr.getLoc(), diag::warn_attribute_return_pointers_only)
       << Attr.getName() << getFunctionOrMethodResultSourceRange(D);
 }
 
 static void handleCommonAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   if (S.LangOpts.CPlusPlus) {
     S.Diag(Attr.getLoc(), diag::err_attribute_not_supported_in_lang)
         << Attr.getName() << AttributeLangSupport::Cpp;
     return;
   }
 
+  if (InternalLinkageAttr *Internal = D->getAttr<InternalLinkageAttr>()) {
+    S.Diag(Attr.getRange().getBegin(), diag::err_attributes_are_not_compatible)
+        << Attr.getName() << Internal;
+    S.Diag(Internal->getLocation(), diag::note_conflicting_attribute);
+    return;
+  }
+
   D->addAttr(::new (S.Context) CommonAttr(Attr.getRange(), S.Context,
                                         Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleNoReturnAttr(Sema &S, Decl *D, const AttributeList &attr) {
   if (hasDeclarator(D)) return;
 
   if (S.CheckNoReturnAttr(attr)) return;
AlwaysInlineAttr *Sema::mergeAlwaysInlineAttr(Decl *D, SourceRange Range,
 
   return ::new (Context) AlwaysInlineAttr(Range, Context,
                                           AttrSpellingListIndex);
 }
 
 MinSizeAttr *Sema::mergeMinSizeAttr(Decl *D, SourceRange Range,
                                     unsigned AttrSpellingListIndex) {
   if (OptimizeNoneAttr *Optnone = D->getAttr<OptimizeNoneAttr>()) {
     Diag(Range.getBegin(), diag::warn_attribute_ignored) << "'minsize'";
     Diag(Optnone->getLocation(), diag::note_conflicting_attribute);
     return nullptr;
   }
 
   if (D->hasAttr<MinSizeAttr>())
     return nullptr;
 
   return ::new (Context) MinSizeAttr(Range, Context, AttrSpellingListIndex);
 }
 
 OptimizeNoneAttr *Sema::mergeOptimizeNoneAttr(Decl *D, SourceRange Range,
                                               unsigned AttrSpellingListIndex) {
   if (AlwaysInlineAttr *Inline = D->getAttr<AlwaysInlineAttr>()) {
     Diag(Inline->getLocation(), diag::warn_attribute_ignored) << Inline;
     Diag(Range.getBegin(), diag::note_conflicting_attribute);
     D->dropAttr<AlwaysInlineAttr>();
   }
   if (MinSizeAttr *MinSize = D->getAttr<MinSizeAttr>()) {
     Diag(MinSize->getLocation(), diag::warn_attribute_ignored) << MinSize;
     Diag(Range.getBegin(), diag::note_conflicting_attribute);
     D->dropAttr<MinSizeAttr>();
   }
 
   if (D->hasAttr<OptimizeNoneAttr>())
std::string SanitizerName =
           .Case("no_sanitize_address", "address")
           .Case("no_sanitize_thread", "thread")
           .Case("no_sanitize_memory", "memory");
   D->addAttr(::new (S.Context)
                  NoSanitizeAttr(Attr.getRange(), S.Context, &SanitizerName, 1,
                                 Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleInternalLinkageAttr(Sema &S, Decl *D,
+                                      const AttributeList &Attr) {
+  if (!(isa<FunctionDecl>(D) || isa<VarDecl>(D) || isa<TagDecl>(D) ||
+        isa<NamespaceDecl>(D))) {
+    S.Diag(Attr.getRange().getBegin(), diag::warn_attribute_ignored)
+      << Attr.getName();
+    return;
+  }
+
+  if (CommonAttr *Common = D->getAttr<CommonAttr>()) {
+    S.Diag(Attr.getRange().getBegin(), diag::err_attributes_are_not_compatible)
+        << Attr.getName() << Common;
+    S.Diag(Common->getLocation(), diag::note_conflicting_attribute);
+    return;
+  }
+
+  if (!D->hasAttr<InternalLinkageAttr>())
+    D->addAttr(::new (S.Context) InternalLinkageAttr(
+        Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
+}
+
 /// Handles semantic checking for features that are common to all attributes,
 /// such as checking whether a parameter was properly specified, or the correct
 /// number of arguments were passed, etc.
 static bool handleCommonAttributeFeatures(Sema &S, Scope *scope, Decl *D,
                                           const AttributeList &Attr) {
   // Several attributes carry different semantics than the parsing requires, so
   // those are opted out of the common handling.
   //
case AttributeList::AT_IntelOclBicc:
     handleCallConvAttr(S, D, Attr);
     break;
   case AttributeList::AT_OpenCLKernel:
     handleSimpleAttribute<OpenCLKernelAttr>(S, D, Attr);
     break;
   case AttributeList::AT_OpenCLImageAccess:
     handleSimpleAttribute<OpenCLImageAccessAttr>(S, D, Attr);
     break;
+  case AttributeList::AT_InternalLinkage:
+    handleInternalLinkageAttr(S, D, Attr);
+    break;
 
   // Microsoft attributes:
   case AttributeList::AT_MSNoVTable:
     handleSimpleAttribute<MSNoVTableAttr>(S, D, Attr);
     break;
   case AttributeList::AT_MSStruct:
     handleSimpleAttribute<MSStructAttr>(S, D, Attr);
     break;

test/CodeGenCXX/attribute_internal_linkage.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+__attribute__((internal_linkage)) static void f() {}
+// CHECK-DAG: define internal void @_ZL1fv
+
+class A {
+public:
+  static int y __attribute__((internal_linkage));
+// CHECK-DAG: @_ZN1A1yE = internal global
+  void f1() __attribute__((internal_linkage));
+// CHECK-DAG: define internal void @_ZN1A2f1Ev
+  void f2() __attribute__((internal_linkage)) {}
+// CHECK-DAG: define internal void @_ZN1A2f2Ev
+  void f3();
+// CHECK-DAG: define internal void @_ZN1A2f3Ev
+  static void f4() __attribute__((internal_linkage)) {}
+// CHECK-DAG: define internal void @_ZN1A2f4Ev
+  A() __attribute__((internal_linkage)) {}
+// CHECK-DAG: define internal void @_ZN1AC1Ev
+// CHECK-DAG: define internal void @_ZN1AC2Ev
+  ~A() __attribute__((internal_linkage)) {}
+// CHECK-DAG: define internal void @_ZN1AD1Ev
+// CHECK-DAG: define internal void @_ZN1AD2Ev
+};
+
+int A::y;
+
+void A::f1() {
+}
+
+__attribute__((internal_linkage)) void A::f3() {
+}
+
+// Internal_linkage on a namespace affects everything within.
+namespace ZZZ __attribute__((internal_linkage)) {
+int x;
+// CHECK-DAG: @_ZNL3ZZZL1xE = internal global
+void f() {}
+// CHECK-DAG: define internal void @_ZNL3ZZZL1fEv
+class A {
+public:
+  A() {}
+// CHECK-DAG: define internal void @_ZNL3ZZZL1AC1Ev
+// CHECK-DAG: define internal void @_ZNL3ZZZL1AC2Ev
+  ~A() {}
+// CHECK-DAG: define internal void @_ZNL3ZZZL1AD1Ev
+// CHECK-DAG: define internal void @_ZNL3ZZZL1AD2Ev
+  void g() {};
+// CHECK-DAG: define internal void @_ZNL3ZZZL1A1gEv
+};
+}
+
+// Internal_linkage on a class affects all its members.
+class __attribute__((internal_linkage)) B {
+public:
+  B() {}
+  // CHECK-DAG: define internal void @_ZNL1BC1Ev
+  // CHECK-DAG: define internal void @_ZNL1BC2Ev
+  ~B() {}
+  // CHECK-DAG: define internal void @_ZNL1BD1Ev
+  // CHECK-DAG: define internal void @_ZNL1BD2Ev
+  void f() {};
+  // CHECK-DAG: define internal void @_ZNL1B1fEv
+  static int x;
+  // CHECK-DAG: @_ZNL1B1xE = internal global
+};
+
+int B::x;
+
+void use() {
+  A a;
+  a.f1();
+  a.f2();
+  a.f3();
+  A::f4();
+  f();
+  int &Y = A::y;
+  ZZZ::A za;
+  za.g();
+  ZZZ::f();
+  int &X = ZZZ::x;
+  B b;
+  b.f();
+  int &X2 = B::x;
+}

test/Sema/internal_linkage.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int var __attribute__((internal_linkage));
+int var2 __attribute__((internal_linkage,common)); // expected-error{{'internal_linkage' and 'common' attributes are not compatible}} \
+                                                   // expected-note{{conflicting attribute is here}}
+int var3 __attribute__((common,internal_linkage)); // expected-error{{'common' and 'internal_linkage' attributes are not compatible}} \
+                                                   // expected-note{{conflicting attribute is here}}
+
+ __attribute__((internal_linkage)) int f() {}
+struct __attribute__((internal_linkage)) S {
+};

test/SemaCXX/internal_linkage.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int f() __attribute__((internal_linkage));
+class __attribute__((internal_linkage)) A {
+public:
+  int x __attribute__((internal_linkage)); // expected-warning{{'internal_linkage' attribute ignored}}
+  static int y __attribute__((internal_linkage));
+  void f1() __attribute__((internal_linkage));
+  void f2() __attribute__((internal_linkage)) {}
+  static void f3() __attribute__((internal_linkage)) {}
+  A() __attribute__((internal_linkage)) {}
+  ~A() __attribute__((internal_linkage)) {}
+  A& operator=(const A&) __attribute__((internal_linkage)) { return *this; }
+  struct {
+    int z  __attribute__((internal_linkage)); // expected-warning{{'internal_linkage' attribute ignored}}
+  };
+};
+
+namespace Z __attribute__((internal_linkage)) {
+}
+
+int A::y;
+
+void A::f1() {
+}