[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:
> 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.
>From the test cases below:
```
extern inline __attribute__((always_inline)) __attribute__((gnu_inline)) unsigned long strlen(const char *p) {
  return 1;
}
unsigned long mystrlen(char const *s) {
  return strlen(s);
}
unsigned long strlen(const char *s) {
  return 2;
}
```
These redeclarations resolve a particular way by GCC and this patch is intending to match that behavior. My question ultimately boils down to whether that also happens for this code, where the function is not a builtin but looks like one due to the attributes:
```
extern inline __attribute__((always_inline)) __attribute__((gnu_inline)) unsigned long wahoo(const char *p) {
  return 1;
}
unsigned long mywahoo(char const *s) {
  return wahoo(s);
}
unsigned long wahoo(const char *s) {
  return 2;
}
```
If this also reorders, then I don't think we can look at whether `FD->getBuiltinID() != 0` to decide whether to do the reordering dance because arbitrary user functions aren't Clang builtins and so they'd not get the correct behavior. Does that make sense?


================
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:
> 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?
I don't think we should modify the redecl chain from `CheckFunctionDeclaration()` -- this case would create a redeclaration chain whose previous link was not temporally the previous declaration. There might be another approach so we can avoid `replaceAllUsesWith()`. One possibility (no idea how feasible or what explodes) is to modify `FunctionDecl::getDefinition()` to look through the chain to return the best definition when there are multiple definitions to pick from.


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

https://reviews.llvm.org/D112059