This is an archive of the discontinued LLVM Phabricator instance.

Fix for merging decls in pragma weak
ClosedPublic

Authored by amusman on Sep 22 2015, 1:05 AM.

Details

Summary

In the following example, we have a declaration of weakfoo before #pragma weak.

extern void weakfoo();
void localfoo() { }
#pragma weak weakfoo=localfoo

There are two decls for weakfoo, this leads to assertion failure later (during search a decl for a call to weakfoo):
lib/Sema/SemaOverload.cpp:5583: void clang::Sema::AddOverloadCandidate(clang::FunctionDecl*, clang::DeclAccessPair, llvm::ArrayRef<clang::Expr*>, clang::OverloadCandidateSet&, bool, bool, bool): Assertion `Proto && "Functions without a prototype cannot be overloaded"' failed.
I suggest to call CheckFunctionDeclaration so that 2 decls for the weakfoo are merged.

Diff Detail

Repository
rL LLVM

Event Timeline

amusman updated this revision to Diff 35348.Sep 22 2015, 1:05 AM
amusman retitled this revision from to Fix for merging decls in pragma weak.
amusman updated this object.
amusman added reviewers: rsmith, aaron.ballman, ABataev.
amusman added a subscriber: cfe-commits.
aaron.ballman added inline comments.Sep 22 2015, 6:01 AM
lib/Sema/SemaDeclAttr.cpp
5201 ↗(On Diff #35348)

If we're in this code, any chance you can also look at the other FIXME statements? If not, that's fine -- this is still positive movement. But it's usually best to tackle all of these at once while it's fresh in someone's mind what's happening here.

amusman added inline comments.Sep 22 2015, 8:45 AM
lib/Sema/SemaDeclAttr.cpp
5201 ↗(On Diff #35348)

I've got here through an existing test case that failed on assertion, not sure I can quickly write or find examples for the other fixme's... So, I'd prefer not to promise that :)

aaron.ballman edited edge metadata.Sep 22 2015, 9:01 AM

Generally LGTM, but I would wait for Richard to review in this case.

~Aaron

lib/Sema/SemaDeclAttr.cpp
5201 ↗(On Diff #35348)

Ah, I hadn't noticed that this was Eli's code originally. I thought the original author was on the review thread for context. I wouldn't worry about it. If it's not problems in the last four years, we can live with it for a bit longer. ;-)

aaron.ballman accepted this revision.Nov 6 2015, 7:04 AM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Nov 6 2015, 7:04 AM
This revision was automatically updated to reflect the committed changes.