[Clang] Work with multiple pragmas weak before definition

Update `WeakUndeclaredIdentifiers` to hold a collection of weak
aliases per identifier instead of only one.

This also allows the "used" state to be removed from `WeakInfo`
because it is really only there as an alternative to removing
processed map entries, and we can represent that using an empty set
now. The serialization code is updated for the removal of the field.
Additionally, a PCH test is added for the new functionality.

The records are grouped by the "target" identifier, which was already
being used as a key for lookup purposes. We also store only one record
per alias name; combined, this means that diagnostics are grouped by
the "target" and limited to one per alias (which should be acceptable).

Fixes PR28611.
Fixes #28985.

Reviewed By: aaron.ballman, cebowleratibm

Differential Revision: https://reviews.llvm.org/D121927

Co-authored-by: Rachel Craik <[email protected]>
Co-authored-by: Jamie Schmeiser <[email protected]>

Author: 3 people

clang/docs/ReleaseNotes.rst

@@ -78,6 +78,10 @@ Bug Fixes
 - No longer crash when specifying a variably-modified parameter type in a
   function with the ``naked`` attribute. This fixes
   `Issue 50541 <https://github.com/llvm/llvm-project/issues/50541>`_.
+- Allow multiple ``#pragma weak`` directives to name the same undeclared (if an
+  alias, target) identifier instead of only processing one such ``#pragma weak``
+  per identifier.
+  Fixes `Issue 28985 <https://github.com/llvm/llvm-project/issues/28985>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

clang/include/clang/Sema/Sema.h

@@ -1072,10 +1072,20 @@ class Sema final {
     }
   };
 
-  /// WeakUndeclaredIdentifiers - Identifiers contained in
-  /// \#pragma weak before declared. rare. may alias another
-  /// identifier, declared or undeclared
-  llvm::MapVector<IdentifierInfo *, WeakInfo> WeakUndeclaredIdentifiers;
+  /// WeakUndeclaredIdentifiers - Identifiers contained in \#pragma weak before
+  /// declared. Rare. May alias another identifier, declared or undeclared.
+  ///
+  /// For aliases, the target identifier is used as a key for eventual
+  /// processing when the target is declared. For the single-identifier form,
+  /// the sole identifier is used as the key. Each entry is a `SetVector`
+  /// (ordered by parse order) of aliases (identified by the alias name) in case
+  /// of multiple aliases to the same undeclared identifier.
+  llvm::MapVector<
+      IdentifierInfo *,
+      llvm::SetVector<
+          WeakInfo, llvm::SmallVector<WeakInfo, 1u>,
+          llvm::SmallDenseSet<WeakInfo, 2u, WeakInfo::DenseMapInfoByAliasOnly>>>
+      WeakUndeclaredIdentifiers;
 
   /// ExtnameUndeclaredIdentifiers - Identifiers contained in
   /// \#pragma redefine_extname before declared.  Used in Solaris system headers
@@ -10175,7 +10185,7 @@ class Sema final {
 
   NamedDecl *DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
                                  SourceLocation Loc);
-  void DeclApplyPragmaWeak(Scope *S, NamedDecl *ND, WeakInfo &W);
+  void DeclApplyPragmaWeak(Scope *S, NamedDecl *ND, const WeakInfo &W);
 
   /// ActOnPragmaWeakID - Called on well formed \#pragma weak ident.
   void ActOnPragmaWeakID(IdentifierInfo* WeakName,

clang/include/clang/Sema/Weak.h

@@ -15,29 +15,40 @@
 #define LLVM_CLANG_SEMA_WEAK_H
 
 #include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/DenseMapInfo.h"
 
 namespace clang {
 
 class IdentifierInfo;
 
 /// Captures information about a \#pragma weak directive.
 class WeakInfo {
-  IdentifierInfo *alias;  // alias (optional)
-  SourceLocation loc;     // for diagnostics
-  bool used;              // identifier later declared?
+  const IdentifierInfo *alias = nullptr; // alias (optional)
+  SourceLocation loc;                    // for diagnostics
 public:
-  WeakInfo()
-    : alias(nullptr), loc(SourceLocation()), used(false) {}
-  WeakInfo(IdentifierInfo *Alias, SourceLocation Loc)
-    : alias(Alias), loc(Loc), used(false) {}
-  inline IdentifierInfo * getAlias() const { return alias; }
+  WeakInfo() = default;
+  WeakInfo(const IdentifierInfo *Alias, SourceLocation Loc)
+      : alias(Alias), loc(Loc) {}
+  inline const IdentifierInfo *getAlias() const { return alias; }
   inline SourceLocation getLocation() const { return loc; }
-  void setUsed(bool Used=true) { used = Used; }
-  inline bool getUsed() { return used; }
-  bool operator==(WeakInfo RHS) const {
-    return alias == RHS.getAlias() && loc == RHS.getLocation();
-  }
-  bool operator!=(WeakInfo RHS) const { return !(*this == RHS); }
+  bool operator==(WeakInfo RHS) const = delete;
+  bool operator!=(WeakInfo RHS) const = delete;
+
+  struct DenseMapInfoByAliasOnly
+      : private llvm::DenseMapInfo<const IdentifierInfo *> {
+    static inline WeakInfo getEmptyKey() {
+      return WeakInfo(DenseMapInfo::getEmptyKey(), SourceLocation());
+    }
+    static inline WeakInfo getTombstoneKey() {
+      return WeakInfo(DenseMapInfo::getTombstoneKey(), SourceLocation());
+    }
+    static unsigned getHashValue(const WeakInfo &W) {
+      return DenseMapInfo::getHashValue(W.getAlias());
+    }
+    static bool isEqual(const WeakInfo &LHS, const WeakInfo &RHS) {
+      return DenseMapInfo::isEqual(LHS.getAlias(), RHS.getAlias());
+    }
+  };
 };
 
 } // end namespace clang

clang/lib/Sema/Sema.cpp

@@ -929,7 +929,7 @@ void Sema::LoadExternalWeakUndeclaredIdentifiers() {
   SmallVector<std::pair<IdentifierInfo *, WeakInfo>, 4> WeakIDs;
   ExternalSource->ReadWeakUndeclaredIdentifiers(WeakIDs);
   for (auto &WeakID : WeakIDs)
-    WeakUndeclaredIdentifiers.insert(WeakID);
+    (void)WeakUndeclaredIdentifiers[WeakID.first].insert(WeakID.second);
 }
 
 
@@ -1180,19 +1180,21 @@ void Sema::ActOnEndOfTranslationUnit() {
 
   // Check for #pragma weak identifiers that were never declared
   LoadExternalWeakUndeclaredIdentifiers();
-  for (auto WeakID : WeakUndeclaredIdentifiers) {
-    if (WeakID.second.getUsed())
+  for (const auto &WeakIDs : WeakUndeclaredIdentifiers) {
+    if (WeakIDs.second.empty())
       continue;
 
-    Decl *PrevDecl = LookupSingleName(TUScope, WeakID.first, SourceLocation(),
+    Decl *PrevDecl = LookupSingleName(TUScope, WeakIDs.first, SourceLocation(),
                                       LookupOrdinaryName);
     if (PrevDecl != nullptr &&
         !(isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl)))
-      Diag(WeakID.second.getLocation(), diag::warn_attribute_wrong_decl_type)
-          << "'weak'" << ExpectedVariableOrFunction;
+      for (const auto &WI : WeakIDs.second)
+        Diag(WI.getLocation(), diag::warn_attribute_wrong_decl_type)
+            << "'weak'" << ExpectedVariableOrFunction;
     else
-      Diag(WeakID.second.getLocation(), diag::warn_weak_identifier_undeclared)
-          << WeakID.first;
+      for (const auto &WI : WeakIDs.second)
+        Diag(WI.getLocation(), diag::warn_weak_identifier_undeclared)
+            << WeakIDs.first;
   }
 
   if (LangOpts.CPlusPlus11 &&

clang/lib/Sema/SemaDecl.cpp

@@ -18707,9 +18707,7 @@ void Sema::ActOnPragmaWeakID(IdentifierInfo* Name,
   if (PrevDecl) {
     PrevDecl->addAttr(WeakAttr::CreateImplicit(Context, PragmaLoc, AttributeCommonInfo::AS_Pragma));
   } else {
-    (void)WeakUndeclaredIdentifiers.insert(
-      std::pair<IdentifierInfo*,WeakInfo>
-        (Name, WeakInfo((IdentifierInfo*)nullptr, NameLoc)));
+    (void)WeakUndeclaredIdentifiers[Name].insert(WeakInfo(nullptr, NameLoc));
   }
 }
 
@@ -18727,8 +18725,7 @@ void Sema::ActOnPragmaWeakAlias(IdentifierInfo* Name,
       if (NamedDecl *ND = dyn_cast<NamedDecl>(PrevDecl))
         DeclApplyPragmaWeak(TUScope, ND, W);
   } else {
-    (void)WeakUndeclaredIdentifiers.insert(
-      std::pair<IdentifierInfo*,WeakInfo>(AliasName, W));
+    (void)WeakUndeclaredIdentifiers[AliasName].insert(W);
   }
 }
 

clang/lib/Sema/SemaDeclAttr.cpp

@@ -9070,9 +9070,7 @@ NamedDecl *Sema::DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
 
 /// 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);
+void Sema::DeclApplyPragmaWeak(Scope *S, NamedDecl *ND, const WeakInfo &W) {
   if (W.getAlias()) { // clone decl, impersonate __attribute(weak,alias(...))
     IdentifierInfo *NDId = ND->getIdentifier();
     NamedDecl *NewD = DeclClonePragmaWeak(ND, W.getAlias(), W.getLocation());
@@ -9099,23 +9097,25 @@ void Sema::ProcessPragmaWeak(Scope *S, Decl *D) {
   // It's valid to "forward-declare" #pragma weak, in which case we
   // have to do this.
   LoadExternalWeakUndeclaredIdentifiers();
-  if (!WeakUndeclaredIdentifiers.empty()) {
-    NamedDecl *ND = nullptr;
-    if (auto *VD = dyn_cast<VarDecl>(D))
-      if (VD->isExternC())
-        ND = VD;
-    if (auto *FD = dyn_cast<FunctionDecl>(D))
-      if (FD->isExternC())
-        ND = FD;
-    if (ND) {
-      if (IdentifierInfo *Id = ND->getIdentifier()) {
-        auto I = WeakUndeclaredIdentifiers.find(Id);
-        if (I != WeakUndeclaredIdentifiers.end()) {
-          WeakInfo W = I->second;
-          DeclApplyPragmaWeak(S, ND, W);
-          WeakUndeclaredIdentifiers[Id] = W;
-        }
-      }
+  if (WeakUndeclaredIdentifiers.empty())
+    return;
+  NamedDecl *ND = nullptr;
+  if (auto *VD = dyn_cast<VarDecl>(D))
+    if (VD->isExternC())
+      ND = VD;
+  if (auto *FD = dyn_cast<FunctionDecl>(D))
+    if (FD->isExternC())
+      ND = FD;
+  if (!ND)
+    return;
+  if (IdentifierInfo *Id = ND->getIdentifier()) {
+    auto I = WeakUndeclaredIdentifiers.find(Id);
+    if (I != WeakUndeclaredIdentifiers.end()) {
+      auto &WeakInfos = I->second;
+      for (const auto &W : WeakInfos)
+        DeclApplyPragmaWeak(S, ND, W);
+      std::remove_reference_t<decltype(WeakInfos)> EmptyWeakInfos;
+      WeakInfos.swap(EmptyWeakInfos);
     }
   }
 }

clang/lib/Serialization/ASTReader.cpp

@@ -3307,7 +3307,7 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
       break;
 
     case WEAK_UNDECLARED_IDENTIFIERS:
-      if (Record.size() % 4 != 0)
+      if (Record.size() % 3 != 0)
         return llvm::createStringError(std::errc::illegal_byte_sequence,
                                        "invalid weak identifiers record");
 
@@ -3322,8 +3322,7 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
         WeakUndeclaredIdentifiers.push_back(
           getGlobalIdentifierID(F, Record[I++]));
         WeakUndeclaredIdentifiers.push_back(
-          ReadSourceLocation(F, Record, I).getRawEncoding());
-        WeakUndeclaredIdentifiers.push_back(Record[I++]);
+            ReadSourceLocation(F, Record, I).getRawEncoding());
       }
       break;
 
@@ -8396,11 +8395,9 @@ void ASTReader::ReadWeakUndeclaredIdentifiers(
       = DecodeIdentifierInfo(WeakUndeclaredIdentifiers[I++]);
     IdentifierInfo *AliasId
       = DecodeIdentifierInfo(WeakUndeclaredIdentifiers[I++]);
-    SourceLocation Loc
-      = SourceLocation::getFromRawEncoding(WeakUndeclaredIdentifiers[I++]);
-    bool Used = WeakUndeclaredIdentifiers[I++];
+    SourceLocation Loc =
+        SourceLocation::getFromRawEncoding(WeakUndeclaredIdentifiers[I++]);
     WeakInfo WI(AliasId, Loc);
-    WI.setUsed(Used);
     WeakIDs.push_back(std::make_pair(WeakId, WI));
   }
   WeakUndeclaredIdentifiers.clear();

clang/lib/Serialization/ASTWriter.cpp

@@ -4559,13 +4559,14 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
   // entire table, since later PCH files in a PCH chain are only interested in
   // the results at the end of the chain.
   RecordData WeakUndeclaredIdentifiers;
-  for (auto &WeakUndeclaredIdentifier : SemaRef.WeakUndeclaredIdentifiers) {
-    IdentifierInfo *II = WeakUndeclaredIdentifier.first;
-    WeakInfo &WI = WeakUndeclaredIdentifier.second;
-    AddIdentifierRef(II, WeakUndeclaredIdentifiers);
-    AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
-    AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
-    WeakUndeclaredIdentifiers.push_back(WI.getUsed());
+  for (const auto &WeakUndeclaredIdentifierList :
+       SemaRef.WeakUndeclaredIdentifiers) {
+    const IdentifierInfo *const II = WeakUndeclaredIdentifierList.first;
+    for (const auto &WI : WeakUndeclaredIdentifierList.second) {
+      AddIdentifierRef(II, WeakUndeclaredIdentifiers);
+      AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
+      AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
+    }
   }
 
   // Build a record containing all of the ext_vector declarations.

clang/test/CodeGen/pragma-weak.c

@@ -17,6 +17,10 @@
 // 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: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
 
 
 
@@ -136,6 +140,15 @@ void __a1(void) {}
 __attribute((pure,noinline,const)) void __xxx(void) { }
 // CHECK: void @__xxx() [[RN:#[0-9]+]]
 
+///////////// PR28611: Try multiple aliases of same undeclared symbol or alias
+#pragma weak undecfunc_alias1 = undecfunc
+#pragma weak undecfunc_alias1 = undecfunc // Try specifying same alias/target pair a second time.
+#pragma weak undecfunc_alias3 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias4 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias2 = undecfunc
+void undecfunc_alias2(void);
+void undecfunc(void) { }
+
 ///////////// PR10878: Make sure we can call a weak alias
 void SHA512Pad(void *context) {}
 #pragma weak SHA384Pad = SHA512Pad

clang/test/PCH/pragma-weak-functional.c

@@ -0,0 +1,17 @@
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/pragma-weak-functional.h %s -verify -emit-llvm -o - | FileCheck %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c-header -emit-pch -o %t %S/pragma-weak-functional.h
+// RUN: %clang_cc1 -include-pch %t %s -verify -emit-llvm -o - | FileCheck %s
+
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
+
+///////////// PR28611: Try multiple aliases of same undeclared symbol or alias
+void undecfunc_alias1(void);
+void undecfunc(void) { }
+// [email protected]:4 {{alias will always resolve to undecfunc}}
+// [email protected]:5 {{alias will always resolve to undecfunc}}

clang/test/PCH/pragma-weak-functional.h

@@ -0,0 +1,6 @@
+// Header for PCH test pragma-weak-functional.c
+
+#pragma weak undecfunc_alias2 = undecfunc
+#pragma weak undecfunc_alias4 = undecfunc_alias1
+#pragma weak undecfunc_alias3 = undecfunc_alias1
+#pragma weak undecfunc_alias1 = undecfunc