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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
