[PATCH] D112059: Fix inline builtin handling in case of redefinition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1302
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
+  if (Fn) {
     std::string FDInlineName = (Fn->getName() + ".inline").str();
----------------
nickdesaulniers wrote:
> I don't think we want to do all this work if just `Fn`; ie. create a new `std::string` with `.inline` suffix for every function we're going to generate code (IR) for.  How about we add an additional unlikely guard:
> 
> `if (FD->getBuiltinID() && FN) {`
> 
> Because in the usual case, `FD` both has a builtin ID and is an inline builtin declaration, while in the exceptional case that this patch addresses, `FD` has a builtin ID but is not an inline builtin declaration.
Is it correct to gate this on whether it's a builtin or not? I thought that builtin-like (e.g., the usual pile of attributes) user code should also have the same effect, shouldn't it?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1319-1321
+    // everywhere. That's GCC behavior too. Unfortunately, I cannot find a way
+    // to detect that situation before we reach codegen, so do some late
+    // replacement.
----------------
nickdesaulniers wrote:
> Perhaps in `Sema::CheckFunctionDeclaration`?  I see there is where we detect redeclarations. The calls from there to `FunctionDecl::setPreviousDeclaration()` seem to set up the redecl chain.  Perhaps this exceptional case (or both cases, even) would be handled better there?
> 
> cc @rsmith @aaron.ballman in case they have feedback/tips/cycles.
I don't know that it's a good idea to modify the redeclaration chain in this case. The comments on the chain are pretty clear that it's a temporal chain where "previous" means previously declared in relation to the current declaration. @rsmith may feel differently, however.


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

https://reviews.llvm.org/D112059