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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6348-6349
+    } else {
+      // If neither of these things, we have a user we don't know how to handle,
+      // so default to previous behavior of emitting a terrible error message.
+      return false;
----------------
This comment can only be understood in the context of this commit with the FIXME comment noted in the relevant test case. I suggest:
  // This user is one we don't know how to handle, so fail redirection. This
  // will result in an ifunc retaining a resolver name that will ultimately fail
  // to be resolved to a defined function.


================
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());
+
----------------
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?


================
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));
----------------
This comment seems inconsistent with the code. If there are no ifuncs involved, then why call `CheckAndReplaceExternCIFuncs()`?


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1573-1575
+  /// Helper function for EmitStaticExternCAliases that clears the uses of
+  /// 'Elem' if it is used exclusively by ifunc resolvers. Returns 'true' if it
+  /// was successful erases Elem.
----------------
Grammar is off in the last sentence.

The comment doesn't really explain this function's purpose. I suggest:
  /// Helper function for EmitStaticExternCAliases() to redirect ifuncs that have a resolver
  /// name that matches 'Elem' to instead resolve to the name of 'CppFunc'. This
  /// redirection is necessary in cases where 'Elem' has a name that will be emitted as
  /// an alias of the name bound to 'CppFunc'; ifuncs may not reference aliases. Redirection
  /// is only performed if 'Elem' is only used by ifuncs in which case, 'Elem' is destroyed..
  /// 'true' is returned If redirection is successful, and 'false' is returned otherwise.


================
Comment at: clang/test/SemaCXX/externc-ifunc-resolver.cpp:9-13
+// FIXME: This diagnostic is pretty confusing, the issue is that the existence
+// of the two functions suppresses the 'alias' creation, and thus the ifunc
+// resolution via the alias as well. In the future we should probably find
+// some way to improve this diagnostic (likely by diagnosing when we decide
+// this case suppresses alias creation).
----------------
Thanks for adding this comment; it is helpful.


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

https://reviews.llvm.org/D122608