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

Fri Apr 1 12:53:25 PDT 2022

erichkeane marked 2 inline comments as done.
erichkeane added inline comments.

================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6393
+    // If Val is null, that implies there were multiple declarations that each
+    // had a claim to the unmangled name. In this case, generation of hte alias
+    // is suppressed. See CodeGenModule::MaybeHandleStaticInExterC.
----------------
tahonermann wrote:
> You retyped my suggested comment instead of copy/paste? Or is the "hte" just intended to test my attention to detail? 😜 
Ugh, even worse.  I read your suggestion and was like, "I don't like that, it reads funny, let me try my own version".  THEN, I typed in MY version of it just to find out I'd basically typed the exact wording(I put the 'See...' line earlier), so I just copy/pasted the change.  For the worse... yay me.

================
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.
----------------
tahonermann wrote:
> tahonermann wrote:
> > 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.
> The parent comment is marked as done, but no change appears to have been applied.
Woops, forgot to git-add the file!  Done again!

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

https://reviews.llvm.org/D122608