This is an archive of the discontinued LLVM Phabricator instance.

[PR30341] Alias must point to a definition
ClosedPublic

Authored by hiraditya on Sep 16 2016, 1:05 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

hiraditya updated this revision to Diff 71692.Sep 16 2016, 1:05 PM
hiraditya retitled this revision from to [PR30341] Alias must point to a definition.
hiraditya updated this object.
hiraditya added reviewers: rafael, eugenis.
sebpop added inline comments.Sep 16 2016, 1:19 PM
clang/lib/CodeGen/CGCXX.cpp
137 ↗(On Diff #71692)

Please remove the reference to r254170.
You may want to put a reference to a bug, or better, just describe in full why this is disabled.

166 ↗(On Diff #71692)

You may want to adjust this comment, now that the condition does not check for "AvailableExternally".
You can move part of the FIXME in the comment above.

hiraditya updated this revision to Diff 71827.Sep 19 2016, 8:16 AM

Added a test case (contributed by Eric).

hiraditya marked 2 inline comments as done.Sep 19 2016, 8:16 AM

The change looks good to me. Somebody else should approve.

rafael edited reviewers, added: rsmith; removed: rafael.Sep 19 2016, 8:46 AM
rsmith added inline comments.Sep 27 2016, 3:55 PM
clang/lib/CodeGen/CGCXX.cpp
137–138 ↗(On Diff #71827)

Instead of this, I'd just say:

available_externally definitions aren't real definitions, so we cannot create an alias to one.

or similar. It doesn't seem necessary or relevant to reference a particular PR here.

140–142 ↗(On Diff #71827)

It's not clear what this comment about AlwaysInline is referring to, since the code does not mention that.

142–143 ↗(On Diff #71827)

What "corresponding LLVM changes" are you expecting here? It seems to be fundamental to aliases that they can only denote definitions.

170 ↗(On Diff #71827)

Did you mean to change the behavior here? For non-available_externally functions, we never used to care whether they're always_inline. Why do we care now?

sebpop added inline comments.Sep 28 2016, 6:18 AM
clang/lib/CodeGen/CGCXX.cpp
140–142 ↗(On Diff #71827)

I think we should just remove all the FIXME note: Aditya has just moved this comment from below... to here.

162 ↗(On Diff #71827)

... from here ...

170 ↗(On Diff #71827)

I think this change does not modify the current behavior: the check has been moved up and we early return in that case.

hiraditya updated this revision to Diff 72827.Sep 28 2016, 7:58 AM

Addressed Richard's comments.

hiraditya marked 3 inline comments as done.Sep 28 2016, 7:59 AM
rsmith added inline comments.Sep 28 2016, 10:56 AM
clang/lib/CodeGen/CGCXX.cpp
170 ↗(On Diff #72827)

We can now only reach this code for the case where TargetLinkage != llvm::GlobalValue::AvailableExternallyLinkage. Substituting true for that expression here gives

if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) &&
    (true || !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>())) {

which simplifies to

if (llvm::GlobalValue::isDiscardableIfUnused(Linkage)) {

not

if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) &&
    !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>()) {

We used to enter this case for a function that is discardable if unused, not available externally, and always inline (that is, a normal inline+always_inline function). With this change, we don't do so any more.

hiraditya updated this revision to Diff 72898.Sep 28 2016, 2:15 PM

Addressed Richard's comment.

clang/lib/CodeGen/CGCXX.cpp
169 ↗(On Diff #72898)

Thanks for pointing this out.

rsmith accepted this revision.Sep 28 2016, 3:21 PM
rsmith edited edge metadata.
This revision is now accepted and ready to land.Sep 28 2016, 3:21 PM
This revision was automatically updated to reflect the committed changes.

I had to revert the patch because this test case was failing. The problem was that the destructors have different return type for different targets. So I'll modify the testcase and put the patch back in.