This is an archive of the discontinued LLVM Phabricator instance.

[CUDA] [AST] Allow isInlineDefinitionExternallyVisible to be called on functions without bodies.
ClosedPublic

Authored by jlebar on Oct 14 2016, 10:31 PM.

Details

Summary

In CUDA compilation, we call isInlineDefinitionExternallyVisible (via
getGVALinkageForFunction) on functions while parsing their definitions.

At the point in time when we call getGVALinkageForFunction, the function
may not have a body, so we trip this assert. But as far as I can tell,
this is harmless.

It is not clear to me whether it's possible to replace this assert with
something slightly weaker.

Event Timeline

jlebar updated this revision to Diff 74766.Oct 14 2016, 10:31 PM
jlebar retitled this revision from to [CUDA] [AST] Allow isInlineDefinitionExternallyVisible to be called on functions without bodies..
jlebar updated this object.
jlebar added reviewers: rsmith, tra.
jlebar added a subscriber: cfe-commits.

Friendly ping. This assert breaks basically all cuda compilation on certain libstdc++ versions. If this change is wrong, I sort of urgently need to figure out the right thing.

tra edited edge metadata.Oct 25 2016, 4:56 PM

I'm OK with the change, but the comments suggest that things may be more complicated.
How about disabling assert for CUDA only?

In D25640#579238, @tra wrote:

I'm OK with the change, but the comments suggest that things may be more complicated.
How about disabling assert for CUDA only?

I don't think that is the right approach. This function has nothing to do with CUDA, so either the assertion is valid and we shouldn't be calling it this way, or it's not valid and we should remove the assertion.

Richard, would appreciate some help with this one.

rsmith edited edge metadata.Oct 27 2016, 12:01 AM

I'm not OK with losing this checking entirely. It seems like the problem case is when we're currently processing the definition of the function in question (so it /does/ have a body, it just doesn't have a /complete/ body yet)? I suppose we could track that directly somehow.

jlebar updated this revision to Diff 76118.Oct 27 2016, 3:46 PM
jlebar edited edge metadata.

Add WillHaveBody flag.

OK, I can add new flags with the best of 'em.

I got rid of a super ugly hack I found that was working around the same problem I was trying to work around here. (And I verified that if I don't call setWillHaveBody, a testcase fails.) I can split this out into two patches if you prefer.

rsmith accepted this revision.Oct 27 2016, 10:20 PM
rsmith edited edge metadata.

Looks great, thanks.

clang/lib/Sema/SemaDecl.cpp
5762–5768 ↗(On Diff #76118)

Wow.

This revision is now accepted and ready to land.Oct 27 2016, 10:20 PM
This revision was automatically updated to reflect the committed changes.