[PATCH] D122608: Fix behavior of ifuncs with 'used' extern "C" static functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6390-6391
     llvm::GlobalValue *Val = I.second;
-    if (Val && !getModule().getNamedValue(Name->getName()))
+    llvm::GlobalValue *ExistingElem =
+        getModule().getNamedValue(Name->getName());
+
----------------
tahonermann wrote:
> I suggest moving the declaration of `ExistingElem` after the break on `!Val` given that the former is not relevant if `Val` is null.
> 
> I think the `!Val` check is worth a comment here. I suggest:
>   // If Val is null, that implies that there were multiple declarations that each
>   // had a claim to the unmangled name. In this case, generation of the alias
>   // is suppressed. See CodeGenModule::MaybeHandleStaticInExternC().
> 
> The lack of checking for an existing element with the desired alias name was a pre-existing oversight, yes?
SGTM, thanks!


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6396-6397
+
+    // Simple case, where there are no ifuncs involved.
+    if (!ExistingElem || CheckAndReplaceExternCIFuncs(ExistingElem, Val))
       addCompilerUsedGlobal(llvm::GlobalAlias::create(Name->getName(), Val));
----------------
tahonermann wrote:
> This comment seems inconsistent with the code. If there are no ifuncs involved, then why call `CheckAndReplaceExternCIFuncs()`?
Woops!  Artifact of a previous implementation of this. Replaced the comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122608/new/

https://reviews.llvm.org/D122608