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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers 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();
----------------
aaron.ballman wrote:
> 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?
> If this also reorders

It does; https://godbolt.org/z/bbrox7f6e.

> Does that make sense?

Yes, thanks for the additional info. In this case, I guess we can disregard my feedback that started this thread, marking it as done?

Perhaps @serge-sans-paille should add such a non-builtin test case as part of the change?


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

https://reviews.llvm.org/D112059