Page MenuHomePhabricator

Implement __attribute__((internal_linkage))
ClosedPublic

Authored by eugenis on Oct 20 2015, 6:01 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 37955.Oct 20 2015, 6:01 PM
eugenis retitled this revision from to Implement __attribute__((internal_linkage)).
eugenis updated this object.
eugenis added a reviewer: rsmith.
eugenis set the repository for this revision to rL LLVM.
eugenis added subscribers: cfe-commits, EricWF, rnk.
majnemer added inline comments.
include/clang/Basic/Attr.td
2138

Space between Function and Var.

lib/Sema/SemaDeclAttr.cpp
1581–1586

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

4694–4695

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 added inline comments.Oct 20 2015, 9:44 PM
lib/Sema/SemaDeclAttr.cpp
1581–1586

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 updated this revision to Diff 38071.Oct 21 2015, 6:26 PM
eugenis marked 2 inline comments as done.

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

lib/Sema/SemaDeclAttr.cpp
1581–1586

Confirmed, we need both. See the test.

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
2136

Why doesn't this use Subjects ?

eugenis updated this revision to Diff 38679.Oct 28 2015, 12:04 PM

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
2136

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 updated this revision to Diff 38979.Nov 2 2015, 1:40 PM

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

rnk accepted this revision.Nov 2 2015, 1:46 PM
rnk added a reviewer: rnk.

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

This revision is now accepted and ready to land.Nov 2 2015, 1:46 PM
rsmith added inline comments.Nov 2 2015, 1:52 PM
include/clang/Basic/Attr.td
2136

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

lib/AST/Decl.cpp
1348–1349

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

lib/Sema/Sema.cpp
539 ↗(On Diff #38979)

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

aaron.ballman requested changes to this revision.Nov 2 2015, 3:57 PM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a subscriber: 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
2136

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
1627

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

lib/Sema/SemaDeclAttr.cpp
3401

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

3415

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.

This revision now requires changes to proceed.Nov 2 2015, 3:57 PM
eugenis updated this revision to Diff 39124.Nov 3 2015, 3:26 PM
eugenis edited edge metadata.
eugenis marked 5 inline comments as done.

Disabled the new attribute on namespaces.

include/clang/Basic/Attr.td
2136

This attribute no longer applies to namespaces.

include/clang/Basic/AttrDocs.td
1627

Extended the explanation.

lib/AST/Decl.cpp
1348–1349

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

lib/Sema/Sema.cpp
539 ↗(On Diff #38979)

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

eugenis updated this revision to Diff 39128.Nov 3 2015, 3:50 PM
eugenis edited edge metadata.

One more test for forward declarations.

rsmith added inline comments.Nov 3 2015, 4:23 PM
include/clang/Basic/AttrDocs.td
1628

Missing period at end of sentence.

1628

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

lib/AST/Decl.cpp
634–635

Dead code?

1348–1349

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
3401

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 updated this revision to Diff 39135.Nov 3 2015, 4:59 PM
eugenis marked 2 inline comments as done.
eugenis added inline comments.
lib/AST/Decl.cpp
634–635

Right. Removed.

1348–1349

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 accepted this revision.Nov 4 2015, 6:38 AM
aaron.ballman edited edge metadata.

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!

This revision is now accepted and ready to land.Nov 4 2015, 6:38 AM
rsmith added inline comments.Nov 4 2015, 11:19 AM
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.

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 added a comment.Nov 4 2015, 11:40 AM

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.

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 updated this revision to Diff 39245.Nov 4 2015, 12:55 PM
eugenis edited edge metadata.
eugenis updated this revision to Diff 39249.Nov 4 2015, 1:22 PM

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 added inline comments.Nov 4 2015, 6:11 PM
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 added inline comments.Nov 5 2015, 6:18 AM
lib/Sema/SemaDecl.cpp
6622

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
3417

Why is this dropping AlwaysInlineAttr instead of returning a nullptr?

eugenis updated this revision to Diff 39389.Nov 5 2015, 11:20 AM
eugenis marked an inline comment as done.

Added the new warning to a -W group.

lib/Sema/SemaDeclAttr.cpp
3417

A copy/paste error. Fixed.

LGTM, thank you!

include/clang/Basic/DiagnosticSemaKinds.td
4092

Good catch!

Richard, are you OK with this?

eugenis updated this revision to Diff 39751.Nov 9 2015, 2:06 PM

Rebase.

eugenis closed this revision.Nov 10 2015, 1:31 PM

r252648.
Thanks everyone for the very detailed review!