PR47663: Warn if an entity becomes weak after its first use.

[rsmith]

Summary

Such code will not work in general; we might have already used the
non-weakness, for example when constant-evaluating an address comparison
Thisor when generating code for the declaration.

Modern versions of GCC reject some such cases, but silently accept
others (based, it appears, on whether the weakness of the declaration
was already inspected). It doesn't seem feasible for us to exactly
follow their behavior, and it might be problematic to start rejecting
cases that GCC accepts and that work in practice, so only warn on this
rather than rejecting, at least for now.

[rjmccall]

This seems like a good idea in the abstract; we'll need to figure out what the practical consequences are.

[rsmith]

In D89212#2324127, @rjmccall wrote:

This seems like a good idea in the abstract; we'll need to figure out what the practical consequences are.

Looks like it triggers warnings in Abseil at least; there, the code looks like this:

// spinlock.h
void AbslInternalSpinLockDelay();
inline void lock(...) {
  AbslInternalSpinLockDelay();
}

// spinlock.cc
__attribute__((weak)) void AbslInternalSpinLockDelay() { ... }

... and adding the weak attribute to the declaration would change the meaning of the program (we want an external reference, not an extern_weak reference).

Perhaps we should only warn if the function is not defined in the same translation unit? If it is defined, then I think its address can never be null, and CodeGen will emit it with the correct linkage. We still have the problem that == comparisons against pointers to the function can incorrectly evaluate to false, but I think that's really a problem with aliases rather than with weakness. (C++ requires that void f(), g(); bool b = f == g; initializes b to false even though the only correct answer is that we don't know; we refuse to evaluate the comparison if either f or g is weak, but I think that's only really addressing the problem that they could both be null, not that they could have the same non-null address, since the latter problem isn't specific to weak declarations. I think the constant evaluator is being overly-conservative, and could evaluate f == g to false if f and g are both weak but at least one of them is defined locally, under the language-guaranteed assumption that distinct functions have different addresses. And perhaps we should have a way of declaring a function as potentially aliasing another function without actually defining the function as an alias, as a way to turn off that assumption independent of weakness?)

[rjmccall]

It does seem necessary to distinguish weak definitions from weak non-definitions, but that's completely reasonable — those two cases essentially act like totally different attributes that happen to be written with the same spelling. If we acknowledge that, I think that gives us a straightforward rule: the problem is adding weak to a non-definition after a previous non-weak non-definition.

I think the only reasonable solution for aliases is to assume they don't happen, given the language rules. We could add an attribute to allow, but I'm not sure anyone would use it: in practice I think it's extremely uncommon to define an external symbol as an alias of another external symbol. Maybe we should just document alongside the alias attribute that the behavior of equality comparisons involving aliased symbols may be inconsistent between translation units.

[rsmith]

• Don't warn if we see a weak definition for a declaration that's already

[rjmccall]

Patch looks good to me. I think we're running some internal tests; do you want to wait for those?

[aaron.ballman]

LGTM aside from some very small nits (feel free to ignore any that don't make sense to you).

clang/lib/AST/DeclBase.cpp

622

This is just novel enough that I'd appreciate a comment about the declaration being in Attr.td so it's more obvious where to find this.

clang/lib/Sema/SemaDecl.cpp

6431

Can the pointers here all be marked const or does that cause issues?

6435–6436

Ah, it's too bad that Decl::specific_attrs() doesn't accept a pack of attributes...

18288

Same request for const here as above.

[rsmith]

In D89212#2326771, @rjmccall wrote:

Patch looks good to me. I think we're running some internal tests; do you want to wait for those?

More testing and validation would be nice; I don't think this patch is urgent so I'm happy to wait.

In my internal testing, I found one more false positive: the protocol buffer compiler's support for weak imports results in code like this:

// generated .h file
extern T foo;
inline T *get() { return &foo; }

// generated .cc file
__attribute__((weak)) extern T foo;
// more uses of foo

This is intentional: the idea is that if someone includes the .h file and invokes the inline function, they get a strong link-time dependency on the symbol foo and need to link against that proto library. But if not, then there is no such dependency, and the library is optional. However, this functionality of the protocol buffer compiler is deprecated and being phased out (and suppressing the warning in this one case should be straightforward) so I'm not worried about it.

[rsmith]

• Addressing review feedback from @aaron.ballman.

[rsmith]

clang/lib/Sema/SemaDecl.cpp

6435–6436

I took a brief look at fixing that, but it's not a completely natural extension because we would want to use a different value_type for the iterator if there's more than one kind under consideration. Probably not worth it for only one user.

18288

Can't make this const; this function modifies the declaration by adding an attribute :) But I can make VD below const.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

6435–6436

Agreed, thank you for looking into it!

18288

Thanks!

clang/include/clang/Basic/Attr.td

return llvm::StringSwitch<llvm::StringRef>(Platform)
              .Case("macOS", "macos")
              .Case("tvOS", "tvos")
              .Case("watchOS", "watchos")
              .Case("iOSApplicationExtension", "ios_app_extension")
              .Case("macOSApplicationExtension", "macos_app_extension")
              .Case("tvOSApplicationExtension", "tvos_app_extension")
              .Case("watchOSApplicationExtension", "watchos_app_extension")
              .Default(Platform);
-} }];
+}
+// Defined in lib/AST/DeclBase.cpp.
+AvailabilityResult getAvailability(ASTContext &C) const;
+}];
   let HasCustomParsing = 1;
   let InheritEvenIfAlreadyPresent = 1;
   let Subjects = SubjectList<[Named]>;
   let Documentation = [AvailabilityDocs];
 }
 
 def ExternalSourceSymbol : InheritableAttr {
   let Spellings = [Clang<"external_source_symbol">];

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_dll_member_of_dll_class : Error<
   "attribute %q0 cannot be applied to member of %q1 class">;
 def warn_attribute_dll_instantiated_base_class : Warning<
   "propagating dll attribute to %select{already instantiated|explicitly specialized}0 "
   "base class template without dll attribute is not supported">,
   InGroup<DiagGroup<"unsupported-dll-base-class-template">>, DefaultIgnore;
 def err_attribute_dll_ambiguous_default_ctor : Error<
   "'__declspec(dllexport)' cannot be applied to more than one default constructor in %0">;
+def warn_attribute_weak_after_use : Warning<
+  "%0 declared weak after its first use; weakness may not be respected in all contexts">,
+  InGroup<DiagGroup<"weak-after-use">>;
+def note_declared_weak_here : Note<
+  "declared weak by %select{'%1' attribute|pragma '%1'}0 here">;
 def err_attribute_weakref_not_static : Error<
   "weakref declaration must have internal linkage">;
 def err_attribute_weakref_not_global_context : Error<
   "weakref declaration of %0 must be in a global context">;
 def err_attribute_weakref_without_alias : Error<
   "weakref declaration of %0 must also have an alias attribute">;
 def err_alias_not_supported_on_darwin : Error <
   "aliases are not supported on darwin">;

clang/lib/AST/DeclBase.cpp

if (!A->getDeprecated().empty() && EnclosingVersion >= A->getDeprecated()) {
     }
 
     return AR_Deprecated;
   }
 
   return AR_Available;
 }
 
+AvailabilityResult AvailabilityAttr::getAvailability(ASTContext &C) const {
+  return CheckAvailability(C, this, nullptr, VersionTuple());
+}
+
 AvailabilityResult Decl::getAvailability(std::string *Message,
                                          VersionTuple EnclosingVersion,
                                          StringRef *RealizedPlatform) const {
   if (auto *FTD = dyn_cast<FunctionTemplateDecl>(this))
     return FTD->getTemplatedDecl()->getAvailability(Message, EnclosingVersion,
                                                     RealizedPlatform);
 
   AvailabilityResult Result = AR_Available;
bool Decl::isWeakImported() const {
   if (!canBeWeakImported(IsDefinition))
     return false;
 
   for (const auto *A : getMostRecentDecl()->attrs()) {
     if (isa<WeakImportAttr>(A))
       return true;
 
     if (const auto *Availability = dyn_cast<AvailabilityAttr>(A)) {
-      if (CheckAvailability(getASTContext(), Availability, nullptr,
-                            VersionTuple()) == AR_NotYetIntroduced)
+      if (Availability->getAvailability(getASTContext()) == AR_NotYetIntroduced)
         return true;
     }
   }
 
   return false;
 }
 
 unsigned Decl::getIdentifierNamespaceForKind(Kind DeclKind) {

clang/lib/Sema/SemaDecl.cpp

if (VarDecl *Var = dyn_cast<VarDecl>(Decl)) {
     // an array type includes any type qualifiers, the element type is so
     // qualified, not the array type."
     if (Type->isArrayType())
       Type = QualType(Context.getAsArrayType(Type), 0);
     Decl->setType(Type);
   }
 }
 
-static void checkAttributesAfterMerging(Sema &S, NamedDecl &ND) {
+static void checkAttributesAfterMerging(Sema &S, NamedDecl &ND,
+                                        bool WillBeDefinition = false) {
   // Ensure that an auto decl is deduced otherwise the checks below might cache
   // the wrong linkage.
   assert(S.ParsingInitForAutoVars.count(&ND) == 0);
 
   // 'weak' only applies to declarations with external linkage.
   if (WeakAttr *Attr = ND.getAttr<WeakAttr>()) {
     if (!ND.isExternallyVisible()) {
       S.Diag(Attr->getLocation(), diag::err_attribute_weak_static);
for (TypeLoc TL = FD->getTypeSourceInfo()->getTypeLoc();
               << !MD << A->getRange();
         } else if (isa<CXXConstructorDecl>(MD) || isa<CXXDestructorDecl>(MD)) {
           S.Diag(A->getLocation(), diag::err_lifetimebound_ctor_dtor)
               << isa<CXXDestructorDecl>(MD) << A->getRange();
         }
       }
     }
   }
+
+  // If the new declaration is weak and the old was already used, we might have
+  // used the non-weakness of the old declaration (specifically, the implied
+  // non-nullness of a non-weak declaration) in one of various ways (constant
+  // evaluation, code generation, diagnostic emission). But don't warn if we're
+  // introducing a weak (non-alias) definition, because a weak definition can't
+  // be null.
+  auto *VD = dyn_cast<ValueDecl>(&ND);
+  if (VD && VD->isUsed(false) && VD->isWeak() &&
+      (VD->hasDefiningAttr() || !(getDefinition(VD) || WillBeDefinition))) {
+    Attr *WeakA = nullptr;
+    for (Attr *A : VD->getAttrs()) {
+      if (!isa<WeakAttr, WeakRefAttr, WeakImportAttr, AvailabilityAttr>(A))
+        continue;
+
+      if (auto *Avail = dyn_cast<AvailabilityAttr>(A))
+        if (Avail->getAvailability(S.Context) != AR_NotYetIntroduced)
+          continue;
+
+      if (A->isInherited()) {
+        WeakA = nullptr;
+        break;
+      }
+      if (!WeakA)
+        WeakA = A;
+    }
+
+    if (WeakA) {
+      S.Diag(VD->getLocation(), diag::warn_attribute_weak_after_use) << VD;
+      S.Diag(WeakA->getLocation(), diag::note_declared_weak_here)
+          << (WeakA->getSyntax() == AttributeCommonInfo::AS_Pragma)
+          << WeakA->getSpelling();
+    }
+  }
 }
 
 static void checkDLLAttributeRedeclaration(Sema &S, NamedDecl *OldDecl,
                                            NamedDecl *NewDecl,
                                            bool IsSpecialization,
                                            bool IsDefinition) {
   if (OldDecl->isInvalidDecl() || NewDecl->isInvalidDecl())
     return;
if (IdentifierInfo *II = Previous.getLookupName().getAsIdentifierInfo()) {
           // FIXME: We should consider this a builtin only in the std namespace.
           NewFD->addAttr(BuiltinAttr::CreateImplicit(Context, BuiltinID));
         }
       }
     }
   }
 
   ProcessPragmaWeak(S, NewFD);
-  checkAttributesAfterMerging(*this, *NewFD);
+  checkAttributesAfterMerging(*this, *NewFD, D.isFunctionDefinition());
 
   AddKnownFunctionAttributes(NewFD);
 
   if (NewFD->hasAttr<OverloadableAttr>() &&
       !NewFD->getType()->getAs<FunctionProtoType>()) {
     Diag(NewFD->getLocation(),
          diag::err_attribute_overloadable_no_prototype)
       << NewFD;
if (PrevDecl && (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl))) {
     else
       Diag(PrevDecl->getLocation(), diag::warn_redefine_extname_not_applied)
           << /*Variable*/(isa<FunctionDecl>(PrevDecl) ? 0 : 1) << PrevDecl;
   // Otherwise, add a label atttibute to ExtnameUndeclaredIdentifiers.
   } else
     (void)ExtnameUndeclaredIdentifiers.insert(std::make_pair(Name, Attr));
 }
 
-void Sema::ActOnPragmaWeakID(IdentifierInfo* Name,
+void Sema::ActOnPragmaWeakID(IdentifierInfo *Name,
                              SourceLocation PragmaLoc,
                              SourceLocation NameLoc) {
-  Decl *PrevDecl = LookupSingleName(TUScope, Name, NameLoc, LookupOrdinaryName);
+  WeakInfo W((IdentifierInfo*)nullptr, NameLoc);
 
-  if (PrevDecl) {
-    PrevDecl->addAttr(WeakAttr::CreateImplicit(Context, PragmaLoc, AttributeCommonInfo::AS_Pragma));
+  if (NamedDecl *PrevDecl =
+          LookupSingleName(TUScope, Name, NameLoc, LookupOrdinaryName)) {
+    // Warn if making this declaration weak might invalidate assumptions we've
+    // already made about it.
+    if (auto *VD = dyn_cast<ValueDecl>(PrevDecl)) {
+      if (VD->isUsed(false) && !VD->isWeak() &&
+          (VD->hasDefiningAttr() || !getDefinition(VD))) {
+        Diag(VD->getLocation(), diag::warn_attribute_weak_after_use) << VD;
+        Diag(W.getLocation(), diag::note_declared_weak_here)
+            << /*IsPragma*/ true << "weak";
+      }
+    }
+
+    // FIXME: We should complain if the declaration has already been defined.
+    DeclApplyPragmaWeak(TUScope, PrevDecl, W);
   } else {
     (void)WeakUndeclaredIdentifiers.insert(
-      std::pair<IdentifierInfo*,WeakInfo>
-        (Name, WeakInfo((IdentifierInfo*)nullptr, NameLoc)));
+        std::pair<IdentifierInfo *, WeakInfo>(Name, W));
   }
 }
 
-void Sema::ActOnPragmaWeakAlias(IdentifierInfo* Name,
-                                IdentifierInfo* AliasName,
+void Sema::ActOnPragmaWeakAlias(IdentifierInfo *Name,
+                                IdentifierInfo *AliasName,
                                 SourceLocation PragmaLoc,
                                 SourceLocation NameLoc,
                                 SourceLocation AliasNameLoc) {
-  Decl *PrevDecl = LookupSingleName(TUScope, AliasName, AliasNameLoc,
-                                    LookupOrdinaryName);
+  NamedDecl *PrevDecl =
+      LookupSingleName(TUScope, AliasName, AliasNameLoc, LookupOrdinaryName);
   WeakInfo W = WeakInfo(Name, NameLoc);
 
   if (PrevDecl && (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl))) {
     if (!PrevDecl->hasAttr<AliasAttr>())
-      if (NamedDecl *ND = dyn_cast<NamedDecl>(PrevDecl))
-        DeclApplyPragmaWeak(TUScope, ND, W);
+      DeclApplyPragmaWeak(TUScope, PrevDecl, W);
   } else {
     (void)WeakUndeclaredIdentifiers.insert(
-      std::pair<IdentifierInfo*,WeakInfo>(AliasName, W));
+        std::pair<IdentifierInfo *, WeakInfo>(AliasName, W));
   }
 }
 
 Decl *Sema::getObjCDeclContext() const {
   return (dyn_cast_or_null<ObjCContainerDecl>(CurContext));
 }
 
 Sema::FunctionEmissionStatus Sema::getEmissionStatus(FunctionDecl *FD,

clang/lib/Sema/SemaDeclAttr.cpp

 
 /// 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);
   if (W.getAlias()) { // clone decl, impersonate __attribute(weak,alias(...))
     IdentifierInfo *NDId = ND->getIdentifier();
+    // FIXME (PR47796): Check for a previous declaration with the target name
+    // here, and build a redeclaration of it. Check whether the previous
+    // declaration is used and warn if so.
     NamedDecl *NewD = DeclClonePragmaWeak(ND, W.getAlias(), W.getLocation());
     NewD->addAttr(
         AliasAttr::CreateImplicit(Context, NDId->getName(), W.getLocation()));
     NewD->addAttr(WeakAttr::CreateImplicit(Context, W.getLocation(),
                                            AttributeCommonInfo::AS_Pragma));
     WeakTopLevelDecl.push_back(NewD);
     // FIXME: "hideous" code from Sema::LazilyCreateBuiltin
     // to insert Decl at TU scope, sorry.

clang/test/Sema/attr-weak.c

-// RUN: %clang_cc1 -verify -fsyntax-only %s
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-unknown-macosx10.3.0 -DMACOS
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-linux-gnu -DLINUX
 
 extern int f0() __attribute__((weak));
 extern int g0 __attribute__((weak));
 extern int g1 __attribute__((weak_import));
 int f2() __attribute__((weak));
 int g2 __attribute__((weak));
 int g3 __attribute__((weak_import)); // expected-warning {{'weak_import' attribute cannot be specified on a definition}}
 int __attribute__((weak_import)) g4(void);

 int C; // expected-note {{previous definition is here}}
 extern int C __attribute__((weak_import)); // expected-warning {{an already-declared variable is made a weak_import declaration}}
 
 static int pr14946_x;
 extern int pr14946_x  __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
 
 static void pr14946_f();
 void pr14946_f() __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
+
+void some_function();
+
+void pr47663_a();
+void pr47663_b();
+static void pr47663_c();
+void pr47663_d();
+void pr47663_e(); // expected-warning {{declared weak after its first use}}
+void pr47663_f(); // expected-note {{possible target}}
+void pr47663_g();
+int pr47663_h;
+void pr47663_z() __attribute__((weak));
+void use() {
+  int arr_a[&pr47663_a ? 1 : -1];
+  int arr_b[&pr47663_b ? 1 : -1];
+  int arr_c[&pr47663_c ? 1 : -1];
+  int arr_d[&pr47663_d ? 1 : -1];
+  int arr_e[&pr47663_e ? 1 : -1];
+  int arr_f[&pr47663_f ? 1 : -1];
+  int arr_g[&pr47663_g ? 1 : -1];
+  int arr_h[&pr47663_h ? 1 : -1]; // expected-warning {{will always evaluate to 'true'}}
+  int arr_z[&pr47663_z ? -1 : 1];
+}
+void pr47663_a() __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_b() __attribute__((weak_import)); // expected-warning {{declared weak after its first use}} expected-note {{'weak_import' attribute here}}
+#ifdef LINUX
+static void pr47663_c() __attribute__((weakref, alias("might_not_exist"))); // expected-warning {{declared weak after its first use}} expected-note {{'weakref' attribute here}}
+#endif
+#ifdef MACOS
+void pr47663_d() __attribute__((availability(macos, introduced=10.4))); // expected-warning {{declared weak after its first use}} expected-note {{'availability' attribute here}}
+#endif
+#pragma weak pr47663_e // expected-note {{pragma 'weak' here}}
+
+// FIXME: This should warn; see PR47796. But it actually creates a bogus
+// overload set. When this is fixed, ensure we produce the 'declared weak after
+// its first use' warning.
+#pragma weak pr47663_f = some_function // expected-note {{possible target}}
+void q() { pr47663_f; } // expected-error {{overloaded}}
+
+#pragma clang attribute push (__attribute__((weak)), apply_to = function) // expected-note {{'weak' attribute here}}
+void pr47663_g(); // expected-warning {{declared weak after its first use}}
+#pragma clang attribute pop
+extern int pr47663_h __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_z() __attribute__((weak_import));
+
+// 'weak' on a definition means something different from 'weak' on a
+// declaration. Don't warn in that case.
+void weak_def_after_use();
+extern int weak_def_after_use_v;
+void use_wdau() { weak_def_after_use(); weak_def_after_use_v = 0; }
+__attribute__((weak)) void weak_def_after_use() {}
+__attribute__((weak)) int weak_def_after_use_v;

clang/test/Sema/init.c

 // day.
 int PR4386_bar();
 int PR4386_foo() __attribute((weak));
 int PR4386_zed();
 
 int PR4386_a = ((void *) PR4386_bar) != 0;
 int PR4386_b = ((void *) PR4386_foo) != 0; // expected-error{{initializer element is not a compile-time constant}}
 int PR4386_c = ((void *) PR4386_zed) != 0;
-int PR4386_zed() __attribute((weak));
+int PR4386_zed() __attribute((weak)); // expected-warning{{'PR4386_zed' declared weak after its first use}} expected-note {{attribute}}
 
 // <rdar://problem/10185490> (derived from SPEC vortex benchmark)
 typedef char strty[10];
 struct vortexstruct { strty s; };
 struct vortexstruct vortexvar = { "asdf" };
 
 typedef struct { uintptr_t x : 2; } StructWithBitfield;
 StructWithBitfield bitfieldvar = { (uintptr_t)&bitfieldvar }; // expected-error {{initializer element is not a compile-time constant}}