Page MenuHomePhabricator

Fix for merging decls in pragma weak
Needs ReviewPublic

Authored by amusman on Feb 26 2016, 2:41 AM.

Details

Summary

This change fixes assertion failure in this example:

extern void f();
void g() { }
#pragma weak f = g
extern void externmain() { return f(); }

(clang-3.9: /export/users/amusman/opensource/ctrunka/llvm/tools/clang/lib/Sema/SemaOverload.cpp:5697: 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.)

It seems to also fix the error message on the following example (from bug 15695):

extern int PFoo(int);
#pragma weak PFoo = Foo
int Foo(int a) { return a; }
int main() {

return PFoo(1);

}

It turns out my previous change (http://reviews.llvm.org/D13048) broke pragma weak in some cases, this is updated patch for this issue.

Diff Detail

Event Timeline

amusman updated this revision to Diff 49168.Feb 26 2016, 2:41 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.
aaron.ballman added inline comments.Mar 7 2016, 6:54 AM
lib/Sema/SemaDeclAttr.cpp
5715

Please do not use auto here since the type is not spelled out in the initializer.

5729

Shouldn't the function declaration we create have the same inline and constexpr specifiers as the previous function declaration? I realize that this isn't new to your changes, but I'm wondering if the previous functionality was correct.

5770–5772

Has this been diagnosed elsewhere already?

amusman added inline comments.Mar 9 2016, 12:46 AM
lib/Sema/SemaDeclAttr.cpp
5715

OK

5729

Yes, it seems OK to take them from previous declaration.

5770–5772

No, I added this assertion just for additional comment purpose.

amusman updated this revision to Diff 50111.Mar 9 2016, 12:49 AM

Fixes for the review comments by Aaron.

aaron.ballman added inline comments.Mar 9 2016, 7:00 AM
lib/Sema/SemaDeclAttr.cpp
5770–5774

I am still confused then. If you can get there in extern "C" scope, this should not be an assert but should instead be a diagnostic with a test case. Or are you saying that this should be impossible to get into this state in the first place (at which point the comment is incorrect)?

amusman added inline comments.Mar 10 2016, 4:19 AM
lib/Sema/SemaDeclAttr.cpp
5770–5774

It is possible for example when we get here on a pragma weak without weak alias:

#pragma weak foo
extern "C" void foo() {}

But, if we move this assertion under the subsequent condition 'if (W.getAilas()) ...', then it would be probably OK to just assert(S==TUScope).

aaron.ballman added inline comments.Mar 14 2016, 10:43 AM
lib/Sema/SemaDeclAttr.cpp
5770–5774

I guess my point was that if we can get into this situation, it should not be an assert, but a diagnostic with test cases.

amusman updated this revision to Diff 50944.Mar 17 2016, 9:19 AM
amusman added inline comments.Mar 17 2016, 9:25 AM
lib/Sema/SemaDeclAttr.cpp
5770–5774

OK. I found an example with weak alias and added diagnostic for this case.
For the other case above, do you think it needs error diagnostic too? It seems OK to me, as it just marks the extern "C" function as weak.

rsmith edited edge metadata.Mar 18 2016, 7:26 AM

It seems like the heart of the problem is that we hacked in support for #pragma weak by injecting a declaration into the AST. That's not how it's supposed to work at all -- it does *not* introduce a source-level name. We should probably be handing it straight to IR generation rather than trying to pretend there's an actual function with attributes. (Note that doing so would also let us support the other kinds of #pragma weak which take a mangled name as a string literal.)

lib/Sema/SemaDeclAttr.cpp
5710–5711

You should be doing redeclaration lookup here.

5715–5716

This seems wrong; you might find an overload set as the previous declarations, and in that case you need to pick the right one out of the set.

5721–5730

This doesn't make sense to me. The point of doing all this work is to make sure that the aliased declaration has the same type as its target. Giving them the same type defeats that check.

test/CodeGen/pragma-weak.c
200

This should be rejected, according to all the documentation I can find on other compilers' support of this pragma.

amusman added inline comments.Mar 24 2016, 2:48 AM
lib/Sema/SemaDeclAttr.cpp
5721–5730

There is another motivation for this work -- to make sure we don't several declarations for one pragma (CheckFunctionDeclaration merges them). Defeating the type check is made specifically to allow an example below (please see my next comment).

test/CodeGen/pragma-weak.c
200

Well, on the other hand, such form seems to be used in practice and allowed by other compilers (and clang without this patch).

Here is an example from FreeBSD kernel:
https://svnweb.freebsd.org/base/head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c?view=markup#l1580

I'm not sure we should emit error in this case, maybe it is better to allow it (and maybe add a separate type check with a warning)?