[Sema] Don't drop weak_import from a declaration that follows a declaration directly contained in a linkage-specification

[ahatanak]

Only drop it if the declaration follows a definition. I believe this is what 33e0226 was trying to do.

rdar://61865848

[llvmbot]

@llvm/pr-subscribers-clang

Author: Akira Hatanaka (ahatanak)

Changes

Only drop it if the declaration follows a definition. I believe this is what 33e0226 was trying to do.

rdar://61865848

---

Full diff: https://github.com/llvm/llvm-project/pull/85886.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+1-2)
• (modified) clang/test/SemaCXX/attr-weak.cpp (+7)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 5850cd0ab6b9aa..2e45f1191273a4 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -4613,8 +4613,7 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
   mergeDeclAttributes(New, Old);
   // Warn if an already-declared variable is made a weak_import in a subsequent
   // declaration
-  if (New->hasAttr<WeakImportAttr>() &&
-      Old->getStorageClass() == SC_None &&
+  if (New->hasAttr<WeakImportAttr>() && Old->isThisDeclarationADefinition() &&
       !Old->hasAttr<WeakImportAttr>()) {
     Diag(New->getLocation(), diag::warn_weak_import) << New->getDeclName();
     Diag(Old->getLocation(), diag::note_previous_declaration);
diff --git a/clang/test/SemaCXX/attr-weak.cpp b/clang/test/SemaCXX/attr-weak.cpp
index f065bfd9483f8a..a2c5fd4abd35f6 100644
--- a/clang/test/SemaCXX/attr-weak.cpp
+++ b/clang/test/SemaCXX/attr-weak.cpp
@@ -55,3 +55,10 @@ constexpr bool weak_method_is_non_null = &WithWeakMember::weak_method != nullptr
 // virtual member function is present.
 constexpr bool virtual_weak_method_is_non_null = &WithWeakMember::virtual_weak_method != nullptr; // expected-error {{must be initialized by a constant expression}}
 // expected-note@-1 {{comparison against pointer to weak member 'WithWeakMember::virtual_weak_method' can only be performed at runtime}}
+
+// Check that no warnings are emitted.
+extern "C" int g0;
+extern int g0 __attribute__((weak_import));
+
+extern "C" int g1 = 0; // expected-note {{previous definition is here}}
+extern int g1 __attribute__((weak_import)); // expected-warning {{attribute declaration must precede definition}}

[rjmccall]

I think you're right about the intended logic being to check for a definition, especially given the wording of the warning. IIRC, we didn't have high-level functions for checking some of these conditions at the time, so looking directly at the storage-class specifier would have been common.

With that said, I think you need to check if a definition exists at all and not just whether the last declaration is that definition.

[ahatanak]

With that said, I think you need to check if a definition exists at all and not just whether the last declaration is that definition.

Note that checkNewAttributesAfterDef, which is called right before the check, removes attributes on New if there was a full definition. The updated patch just looks for any previously declared tentative definition.

[ahatanak]

The updated patch checks that the decl isn't DeclarationOnly to make it clear that we are looking for all definitions although we know we won't find any full definitions.

[rjmccall]

LGTM. The (existing) diagnostic wording does seem a little awkward, so if you want to put effort into cleaning that up, it'd be welcome, but I won't hold up this fix.