Fix for merging decls in pragma weak

[amusman]

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.

[aaron.ballman]

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–5774

Has this been diagnosed elsewhere already?

[amusman]

lib/Sema/SemaDeclAttr.cpp

5715

OK

5729

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

5770–5774

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

[amusman]

Fixes for the review comments by Aaron.

[aaron.ballman]

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]

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]

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]

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]

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]

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)?

[amusman]

Ping.

lib/Sema/SemaDeclAttr.cpp

void Sema::checkUnusedDeclAttributes(Declarator &D) {
   ::checkUnusedDeclAttributes(*this, D.getDeclSpec().getAttributes().getList());
   ::checkUnusedDeclAttributes(*this, D.getAttributes());
   for (unsigned i = 0, e = D.getNumTypeObjects(); i != e; ++i)
     ::checkUnusedDeclAttributes(*this, D.getTypeObject(i).getAttrs());
 }
 
 /// DeclClonePragmaWeak - clone existing decl (maybe definition),
 /// \#pragma weak needs a non-definition decl and source may not have one.
-NamedDecl * Sema::DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
-                                      SourceLocation Loc) {
+NamedDecl *Sema::DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
   assert(isa<FunctionDecl>(ND) || isa<VarDecl>(ND));
   NamedDecl *NewD = nullptr;
+  LookupResult Previous(*this, II, Loc, LookupOrdinaryName);
+  LookupParsedName(Previous, TUScope, nullptr, true);
+
+  // If there is some previous declaration, prefer to use its type.
+  // Otherwise, use the type from the existing decl (weak default).
+  NamedDecl *PrevD =
+      Previous.isSingleResult() ? Previous.getRepresentativeDecl() : nullptr;
   if (FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
-    FunctionDecl *NewFD;
-    // FIXME: Missing call to CheckFunctionDeclaration().
     // FIXME: Mangling?
     // FIXME: Is the qualifier info correct?
     // FIXME: Is the DeclContext correct?
-    NewFD = FunctionDecl::Create(FD->getASTContext(), FD->getDeclContext(),
-                                 Loc, Loc, DeclarationName(II),
-                                 FD->getType(), FD->getTypeSourceInfo(),
-                                 SC_None, false/*isInlineSpecified*/,
-                                 FD->hasPrototype(),
-                                 false/*isConstexprSpecified*/);
+    auto PrevFD = dyn_cast_or_null<FunctionDecl>(PrevD);
+    if (!PrevFD)
+      PrevFD = FD;
+
+    auto NewFD = FunctionDecl::Create(
+        PrevFD->getASTContext(), PrevFD->getDeclContext(), Loc, Loc,
+        DeclarationName(II), PrevFD->getType(), PrevFD->getTypeSourceInfo(),
+        SC_None, PrevFD->isInlineSpecified(), PrevFD->hasPrototype(),
+        PrevFD->isConstexpr());
     NewD = NewFD;
-
-    if (FD->getQualifier())
-      NewFD->setQualifierInfo(FD->getQualifierLoc());
+    if (PrevFD->getQualifier())
+      NewFD->setQualifierInfo(PrevFD->getQualifierLoc());
 
     // Fake up parameter variables; they are declared as if this were
     // a typedef.
-    QualType FDTy = FD->getType();
+    QualType FDTy = PrevFD->getType();
     if (const FunctionProtoType *FT = FDTy->getAs<FunctionProtoType>()) {
-      SmallVector<ParmVarDecl*, 16> Params;
+      SmallVector<ParmVarDecl *, 16> Params;
       for (const auto &AI : FT->param_types()) {
         ParmVarDecl *Param = BuildParmVarDeclForTypedef(NewFD, Loc, AI);
         Param->setScopeInfo(0, Params.size());
         Params.push_back(Param);
       }
       NewFD->setParams(Params);
     }
+    CheckFunctionDeclaration(TUScope, NewFD, Previous,
+                             false /*IsExplicitSpecialization*/);
   } else if (VarDecl *VD = dyn_cast<VarDecl>(ND)) {
-    NewD = VarDecl::Create(VD->getASTContext(), VD->getDeclContext(),
-                           VD->getInnerLocStart(), VD->getLocation(), II,
-                           VD->getType(), VD->getTypeSourceInfo(),
-                           VD->getStorageClass());
-    if (VD->getQualifier()) {
-      VarDecl *NewVD = cast<VarDecl>(NewD);
-      NewVD->setQualifierInfo(VD->getQualifierLoc());
-    }
+    auto PrevVD = dyn_cast_or_null<VarDecl>(PrevD);
+    if (!PrevVD)
+      PrevVD = VD;
+
+    auto NewVD =
+        VarDecl::Create(PrevVD->getASTContext(), PrevVD->getDeclContext(), Loc,
+                        Loc, II, PrevVD->getType(), PrevVD->getTypeSourceInfo(),
+                        PrevVD->getStorageClass());
+    NewD = NewVD;
+    if (PrevVD->getQualifier())
+      NewVD->setQualifierInfo(PrevVD->getQualifierLoc());
+
+    CheckVariableDeclaration(NewVD, Previous);
   }
   return NewD;
 }
 
 /// DeclApplyPragmaWeak - A declaration (maybe definition) needs \#pragma weak
 /// applied to it, possibly with an alias.
 void Sema::DeclApplyPragmaWeak(Scope *S, NamedDecl *ND, WeakInfo &W) {
   if (W.getUsed()) return; // only do this once
   W.setUsed(true);
+  // We can get here in TU or extern "C" scope.
+  assert((S == TUScope || (LangOpts.CPlusPlus && S->getEntity() != nullptr &&
+          S->getEntity()->isExternCContext())) && "Unexpected scope");
   if (W.getAlias()) { // clone decl, impersonate __attribute(weak,alias(...))
     IdentifierInfo *NDId = ND->getIdentifier();
     NamedDecl *NewD = DeclClonePragmaWeak(ND, W.getAlias(), W.getLocation());
     NewD->addAttr(AliasAttr::CreateImplicit(Context, NDId->getName(),
                                             W.getLocation()));
     NewD->addAttr(WeakAttr::CreateImplicit(Context, W.getLocation()));
     WeakTopLevelDecl.push_back(NewD);
     // FIXME: "hideous" code from Sema::LazilyCreateBuiltin
     // to insert Decl at TU scope, sorry.

test/CodeGen/pragma-weak.c

 // CHECK-DAG: @foo2 = weak alias void (), void ()* @__foo2
 // CHECK-DAG: @stutter = weak alias void (), void ()* @__stutter
 // CHECK-DAG: @stutter2 = weak alias void (), void ()* @__stutter2
 // CHECK-DAG: @declfirst = weak alias void (), void ()* @__declfirst
 // CHECK-DAG: @declfirstattr = weak alias void (), void ()* @__declfirstattr
 // CHECK-DAG: @mix2 = weak alias void (), void ()* @__mix2
 // CHECK-DAG: @a1 = weak alias void (), void ()* @__a1
 // CHECK-DAG: @xxx = weak alias void (), void ()* @__xxx
+// CHECK-DAG: @weakfoo = weak alias void {{.*}} @localfoo
+// CHECK-DAG: @f1 = weak alias {{.*}} @g1
+// CHECK-DAG: @f2 = weak alias {{.*}} @g2
+// CHECK-DAG: @zzzz = weak alias {{.*}} @global_z
+// CHECK-DAG: @yyyy = weak alias {{.*}} @global_y
+// CHECK-DAG: @wwww = weak alias {{.*}} @global_w
 
 
 
 // CHECK-LABEL: define weak void @weakdef()
 
 
 #pragma weak weakvar
 int weakvar;
label:
   #pragma weak PR16705c
   PR16705c();
 }
 
 // CHECK: declare extern_weak i32 @PR16705a()
 // CHECK: declare extern_weak i32 @PR16705b()
 // CHECK: declare extern_weak i32 @PR16705c()
 
+// In this test case, we have a declaration of weakfoo before #pragma weak.
+// Test that 2 decls for the weakfoo are merged.
+extern void weakfoo();
+void localfoo() { }
+#pragma weak weakfoo=localfoo
+extern void externmain() { return weakfoo(); }
+// CHECK-LABEL: define void @externmain()
+// CHECK: call{{.*}}@weakfoo
+
+int f1(char *);
+int g1(char *);
+#pragma weak f1 = g1
+int g1(char *p) {return *p;}
+
+struct s { int a; int b; };
+struct s_impl { char a[4]; double b; };
+int f2(struct s *, int);
+int g2(struct s_impl *, int);
+#pragma weak f2 = g2
+int g2(struct s_impl * p, int d) {return p->a[0] + d;}
+
+// Pragma weak alias for a variable, test that the variable decls are merged
+int zzzz;
+int global_z = 123;
+#pragma weak zzzz=global_z
+int global_y = 123;
+#pragma weak yyyy=global_y
+int yyyy;
+int global_w = 123;
+#pragma weak wwww=global_w
+int get_zzzz() { return zzzz + yyyy + wwww; }
+// CHECK-LABEL: define {{.*}}@get_zzzz
+// CHECK: load {{.*}}@zzzz
+// CHECK: load {{.*}}@yyyy
+// CHECK: load {{.*}}@wwww
 
 ///////////// TODO: stuff that still doesn't work
 
 // due to the fact that disparate TopLevelDecls cannot affect each other
 // (due to clang's Parser and ASTConsumer behavior, and quite reasonable)
 // #pragma weak must appear before or within the same TopLevelDecl as it
 // references.
 void yyy(void){}

test/CodeGenCXX/pragma-weak.cpp

 // RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm %s -o - | FileCheck %s
 
 #pragma weak zex
 int zex;
 // GCC produces a weak symbol for this because it matches mangled names.
 // Different c++ ABIs may or may not mangle this, so we produce a strong
 // symbol.
 // CHECK: @zex = global i32
+//
+// CHECK-DAG: @zzzz = weak alias {{.*}} @global_x
 
 #pragma weak foo
 struct S {  void foo(); };
 void S::foo() {}
 // CHECK-LABEL: define {{.*}}void @_ZN1S3fooEv(
 
 #pragma weak zed
 namespace bar {  void zed() {} }
 // CHECK-LABEL: define {{.*}}void @_ZN3bar3zedEv(
 
 #pragma weak bah
 void bah() {}
 // CHECK-LABEL: define {{.*}}void @_Z3bahv(
 
 #pragma weak baz
 extern "C" void baz() {}
 // CHECK-LABEL: define weak {{.*}}void @baz(
 
 #pragma weak _Z3baxv
 void bax() {}
 // GCC produces a weak symbol for this one, but it doesn't look like a good
 // idea to expose the mangling to the pragma unless we really have to.
 // CHECK-LABEL: define {{.*}}void @_Z3baxv(
+
+extern "C" int zzzz;
+int global_x = 123;
+#pragma weak zzzz=global_x
+int get_zzzz() { return zzzz; }
+// CHECK-LABEL: define {{.*}}@_Z8get_zzzzv
+// CHECK: load {{.*}}@zzzz
+

test/Sema/pragma-weak.c

 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -verify %s
 
 void __both3(void);
 #pragma weak both3 = __both3 // expected-note {{previous definition}}
 void both3(void) __attribute((alias("__both3"))); // expected-error {{redefinition of 'both3'}}
 void __both3(void) {}
 
 void __a3(void) __attribute((noinline));
 #pragma weak a3 = __a3 // expected-note {{previous definition}}
 void a3(void) __attribute((alias("__a3"))); // expected-error {{redefinition of 'a3'}}
 void __a3(void) {}
+
+extern void weak2foo(int);
+void local2foo(double d1, double d2) { }
+#pragma weak weak2foo=local2foo // expected-note {{'weak2foo' declared here}}
+#pragma weak weak3foo=local2foo // expected-note {{'weak3foo' declared here}}
+extern void extern2main() {
+  return weak2foo()  // expected-error {{too few arguments to function call, expected 1, have 0}}
+       + weak3foo(); // expected-error {{too few arguments to function call, expected 2, have 0}}
+}
+