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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Bumping for an update here.  We can tolerate a build breakage for our older kernels over the weekend, but we should really try to get this resolved by EOW, otherwise we need to look into reverting:

- 3d6f49a56995b845c40be5827ded5d1e3f692cec <https://reviews.llvm.org/rG3d6f49a56995b845c40be5827ded5d1e3f692cec> Tue Sep 28 13:24:25 2021 +0200 (breakage)
- bd379915de38a9af3d65e19075a6a64ebbb8d6db <https://reviews.llvm.org/rGbd379915de38a9af3d65e19075a6a64ebbb8d6db> Tue Sep 28 16:07:33 2021 +0200 (attempted fix forward of 3d6f49a56995b845 <https://reviews.llvm.org/rG3d6f49a56995b845c40be5827ded5d1e3f692cec>)
- 0d76d4833dd2815e0b1c786250f474d222f6a0a1 <https://reviews.llvm.org/rG0d76d4833dd2815e0b1c786250f474d222f6a0a1> Tue Sep 28 11:30:37 2021 -0700 (revert of 3d6f49a56995b845 <https://reviews.llvm.org/rG3d6f49a56995b845c40be5827ded5d1e3f692cec>)
- c3717b6858d32d64514a187ede1a77be8ba4e542 <https://reviews.llvm.org/rGc3717b6858d32d64514a187ede1a77be8ba4e542> Tue Sep 28 21:00:47 2021 +0200 (reland, introduced kernel breakage)
- 0f0e31cf511def3e92244e615b2646c1fd0df0cd <https://reviews.llvm.org/rG0f0e31cf511def3e92244e615b2646c1fd0df0cd> Mon Oct 4 22:26:25 2021 +0200 (fix forward)



================
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();
----------------
aaron.ballman wrote:
> 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?
What do you mean? I'm sorry, I don't quite follow.


================
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.
----------------
aaron.ballman wrote:
> 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.
Sorry, I don't quite follow whether your approving of the current approach or dismissive?


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

https://reviews.llvm.org/D112059