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

[serge via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6bfc85c217e4: Fix inline builtin handling in case of 
redefinition (authored by serge-sans-paille).

Changed prior to commit:
  https://reviews.llvm.org/D112059?vs=383241=384001#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112059

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/strlen-inline-builtin-redecl.c
  clang/test/CodeGen/user-func-gnu-inline-redecl.c

Index: clang/test/CodeGen/user-func-gnu-inline-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/user-func-gnu-inline-redecl.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -O1 -o - %s | FileCheck %s
+//
+// Verifies that the gnu_inline version is ignored in favor of the redecl
+
+extern inline __attribute__((gnu_inline)) unsigned long some_size(int c) {
+  return 1;
+}
+unsigned long mycall(int s) {
+  // CHECK-LABEL: i64 @mycall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
+unsigned long some_size(int c) {
+  return 2;
+}
+unsigned long yourcall(int s) {
+  // CHECK-LABEL: i64 @yourcall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,40 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
-std::string FDInlineName = (Fn->getName() + ".inline").str();
-llvm::Module *M = Fn->getParent();
-llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  if (Fn) {
+if (FD->isInlineBuiltinDeclaration()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  llvm::Function *Clone = M->getFunction(FDInlineName);
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// 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.
+else {
+  for (const FunctionDecl *PD = FD->getPreviousDecl(); PD;
+   PD = PD->getPreviousDecl()) {
+if (LLVM_UNLIKELY(PD->isInlineBuiltinDeclaration())) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  if (llvm::Function *Clone = M->getFunction(FDInlineName)) {
+Clone->replaceAllUsesWith(Fn);
+Clone->eraseFromParent();
+  }
+  break;
+}
+  }
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.

thanks again for all of the work that went into this!


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from a formatting nit. I don't think the precommit CI failures are 
related to your patch from what I was seeing, but may be worth keeping an eye 
on once you land just in case.




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1323
+else {
+  for(const FunctionDecl* PD = FD->getPreviousDecl(); PD; PD = 
PD->getPreviousDecl()) {
+if(LLVM_UNLIKELY(PD->isInlineBuiltinDeclaration())) {

Please fix the formatting.


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

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

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 383241.
serge-sans-paille added a comment.

Re-uploading previous version that walks redef, with a slight change in the 
walking algorithm.


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

https://reviews.llvm.org/D112059

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/strlen-inline-builtin-redecl.c
  clang/test/CodeGen/user-func-gnu-inline-redecl.c

Index: clang/test/CodeGen/user-func-gnu-inline-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/user-func-gnu-inline-redecl.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -O1 -o - %s | FileCheck %s
+//
+// Verifies that the gnu_inline version is ignored in favor of the redecl
+
+extern inline __attribute__((gnu_inline)) unsigned long some_size(int c) {
+  return 1;
+}
+unsigned long mycall(int s) {
+  // CHECK-LABEL: i64 @mycall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
+unsigned long some_size(int c) {
+  return 2;
+}
+unsigned long yourcall(int s) {
+  // CHECK-LABEL: i64 @yourcall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,39 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
-std::string FDInlineName = (Fn->getName() + ".inline").str();
-llvm::Module *M = Fn->getParent();
-llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  if (Fn) {
+if (FD->isInlineBuiltinDeclaration()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  llvm::Function *Clone = M->getFunction(FDInlineName);
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// 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.
+else {
+  for(const FunctionDecl* PD = FD->getPreviousDecl(); PD; PD = PD->getPreviousDecl()) {
+if(LLVM_UNLIKELY(PD->isInlineBuiltinDeclaration())) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  if (llvm::Function *Clone = M->getFunction(FDInlineName)) {
+Clone->replaceAllUsesWith(Fn);
+Clone->eraseFromParent();
+  }
+  break;
+}
+  }
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D112059#3094464 , @nickdesaulniers 
wrote:

> Here's a [hastily and poorly written] script to measure the average cycle 
> counts for 30 invocations using linux `perf`: 
> https://gist.github.com/nickdesaulniers/4a20ba10c26ac2ad02cb0425b8b0f826
>
> For Diff 382671 (latest; storing redecl state), builds of the linux kernel 
> x86_64 defconfig+CONFIG_FORTIFY_SOURCE=y:
>
>   $ /tmp/measure_30.sh 'make LLVM=1 -j72' 'make LLVM=1 -j72 clean'
>   ...
>   Average of 30 runs: 10780685107280.83 cycles
>
> For Diff 382344 (earlier; walking redecl chain), builds of the linux kernel 
> x86_64 defconfig+CONFIG_FORTIFY_SOURCE=y:
>
>   $ /tmp/measure_30.sh 'make LLVM=1 -j72' 'make LLVM=1 -j72 clean'
>   ...
>   Average of 30 runs: 10745227016663.00 cycles
>
> Damn, so what I proposed was slower, at least for the major case that I care 
> about. I suspect that perhaps there's more forward declarations than actual 
> functions we end up generating IR for, perhaps...either way, I'm sorry for 
> suggesting the "storing redecl state" approach.  If we want to go back to the 
> earlier version in Diff 382344, at this point, I'd be happy to accept that 
> revision.  Sorry for causing whiplash on this.

Thank you for measuring this as well! And no worries on the whiplash (easy for 
me to say as a reviewer, hah) -- I think it was a reasonable thought to explore 
and measure the performance of. FWIW, I'd be happy accepting the earlier 
revision as well.


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Here's a [hastily and poorly written] script to measure the average cycle 
counts for 30 invocations using linux `perf`: 
https://gist.github.com/nickdesaulniers/4a20ba10c26ac2ad02cb0425b8b0f826

For Diff 382671 (latest; storing redecl state), builds of the linux kernel 
x86_64 defconfig+CONFIG_FORTIFY_SOURCE=y:

  $ /tmp/measure_30.sh 'make LLVM=1 -j72' 'make LLVM=1 -j72 clean'
  ...
  Average of 30 runs: 10780685107280.83 cycles

For Diff 382344 (earlier; walking redecl chain), builds of the linux kernel 
x86_64 defconfig+CONFIG_FORTIFY_SOURCE=y:

  $ /tmp/measure_30.sh 'make LLVM=1 -j72' 'make LLVM=1 -j72 clean'
  ...
  Average of 30 runs: 10745227016663.00 cycles

Damn, so what I proposed was slower, at least for the major case that I care 
about. I suspect that perhaps there's more forward declarations than actual 
functions we end up generating IR for, perhaps...either way, I'm sorry for 
suggesting the "storing redecl state" approach.  If we want to go back to the 
earlier version in Diff 382344, at this point, I'd be happy to accept that 
revision.  Sorry for causing whiplash on this.


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

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

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

I second @aaron.ballman there. I compiled the sqlite3.c amalgamation, -O0, with 
both approach, measuring the number of instructions as gathered by `valgrind 
--tool=callgrind`

- when walking redecls:9001630039 instructions, I changed the 
implementation a bit down to 9001628850
- when storing redecl state: 9000816370 instructions


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D112059#3090466 , 
@serge-sans-paille wrote:

> - Use a FunctionDecl Attribute to store the shadowed inline redecl status

The downside to this approach is that we can now handle less ctor initializers 
because we need to steal a bit from there. We're going from allowing 2,097,152 
ctor inits to 1,048,576, which is still a pretty large number of ctor inits, so 
I think this new approach is defensible. However, this does make it that much 
harder to add any new bits in the future. I agree that walking the redecl chain 
could potentially cause a performance issue, but that's speculative without 
some measurements. Because we don't have those measurements, I'm actually a bit 
more comfortable with walking the redecl chain than the current approach -- if 
we measure performance and find that walking this chain is a bottleneck, then 
it makes it more obvious that the new approach is worthwhile.


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

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

[serge via Phabricator via cfe-commits]

serge-sans-paille added inline comments.



Comment at: clang/test/CodeGen/user-func-gnu-inline-redecl.c:20
+  return some_size(s);
+}

nickdesaulniers wrote:
> this test passes before this patch is applied; I wonder if we have existing 
> coverage in tree for this case?
> 
> Surprisingly, I don't think we do. Perhaps `gnu-inline-redecl.c` might be a 
> more concise test name?
> 
> I can't help but shake the feeling that the builtin id stuff is a degenerate 
> case of how GCC treats redeclarations of extern inline (gnu_inline) functions 
> and that perhaps by solving just that, fixing the case of builtins might just 
> "fall out" from that.
Clang indeed naturally handles gnu_inline in a decent way. The problem we're 
trying to solve now is a side effect of the premature renaming of function call 
site when we think it's a direct call to inline builtin. I've updated the 
implementation to avoid walking redecls.


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

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

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 382671.
serge-sans-paille added a comment.

- Use a FunctionDecl Attribute to store the shadowed inline redecl status


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

https://reviews.llvm.org/D112059

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/gnu-inline-redecl.c
  clang/test/CodeGen/strlen-inline-builtin-redecl.c

Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/test/CodeGen/gnu-inline-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/gnu-inline-redecl.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -O1 -o - %s | FileCheck %s
+//
+// Verifies that the gnu_inline version is ignored in favor of the redecl
+
+extern inline __attribute__((gnu_inline)) unsigned long some_size(int c) {
+  return 1;
+}
+unsigned long mycall(int s) {
+  // CHECK-LABEL: i64 @mycall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
+unsigned long some_size(int c) {
+  return 2;
+}
+unsigned long yourcall(int s) {
+  // CHECK-LABEL: i64 @yourcall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -10927,6 +10927,11 @@
   }
 
   if (Redeclaration) {
+FunctionDecl *OldFDecl = dyn_cast(OldDecl);
+if (OldFDecl && OldFDecl->isInlineBuiltinDeclaration() &&
+!NewFD->isInlineBuiltinDeclaration())
+  NewFD->setShadowsGNUInlineIntrinsic(true);
+
 // NewFD and OldDecl represent declarations that need to be
 // merged.
 if (MergeFunctionDecl(NewFD, OldDecl, S, MergeTypeWithPrevious)) {
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,34 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
-std::string FDInlineName = (Fn->getName() + ".inline").str();
-llvm::Module *M = Fn->getParent();
-llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  if (Fn) {
+if (FD->isInlineBuiltinDeclaration()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  llvm::Function *Clone = M->getFunction(FDInlineName);
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// 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.
+else if (FD->shadowsGNUInlineIntrinsic()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  if (llvm::Function *Clone = M->getFunction(FDInlineName)) {
+Clone->replaceAllUsesWith(Fn);
+Clone->eraseFromParent();
+  }
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/test/CodeGen/user-func-gnu-inline-redecl.c:20
+  return some_size(s);
+}

this test passes before this patch is applied; I wonder if we have existing 
coverage in tree for this case?

Surprisingly, I don't think we do. Perhaps `gnu-inline-redecl.c` might be a 
more concise test name?

I can't help but shake the feeling that the builtin id stuff is a degenerate 
case of how GCC treats redeclarations of extern inline (gnu_inline) functions 
and that perhaps by solving just that, fixing the case of builtins might just 
"fall out" from that.


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Actually, it looks like:

  diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
  index 69d2ef631872..8e77cdef2ed5 100644
  --- a/clang/lib/Sema/SemaDecl.cpp
  +++ b/clang/lib/Sema/SemaDecl.cpp
  @@ -10927,6 +10927,10 @@ bool Sema::CheckFunctionDeclaration(Scope *S, 
FunctionDecl *NewFD,
 }
   
 if (Redeclaration) {
  +if (cast(OldDecl)->isInlineBuiltinDeclaration() && 
!NewFD->isInlineBuiltinDeclaration()) {
  +  // Set a flag on NewFD that it's a shadowed gnu_inline that should be
  +  // emitted, or on OldDecl that it should not be emitted?
  +}
   // NewFD and OldDecl represent declarations that need to be
   // merged.
   if (MergeFunctionDecl(NewFD, OldDecl, S, MergeTypeWithPrevious)) {

might detect what you need.  Can we perhaps use those checks there (when we 
have already detected a redeclaration) to perhaps set new members (to be added) 
on `FunctionDecl` that they shouldn't be emitted or not because they are this 
weird case? Then `CodeGenFunction::GenerateCode` can simply check that flag, 
then erase the existing function (or not generate it in the first place by 
setting a flag on `OldDecl` perhaps?).  No redecl walking for every 
FunctionDecl required.


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.

Thank you for fixing this terrible edge case; LGTM.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, though codegen is not my area of expertise.


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

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

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 382344.
serge-sans-paille added a comment.

Formatting nits


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

https://reviews.llvm.org/D112059

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/strlen-inline-builtin-redecl.c
  clang/test/CodeGen/user-func-gnu-inline-redecl.c

Index: clang/test/CodeGen/user-func-gnu-inline-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/user-func-gnu-inline-redecl.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -O1 -o - %s | FileCheck %s
+//
+// Verifies that the gnu_inline version is ignored in favor of the redecl
+
+extern inline __attribute__((gnu_inline)) unsigned long some_size(int c) {
+  return 1;
+}
+unsigned long mycall(int s) {
+  // CHECK-LABEL: i64 @mycall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
+unsigned long some_size(int c) {
+  return 2;
+}
+unsigned long yourcall(int s) {
+  // CHECK-LABEL: i64 @yourcall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,36 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
-std::string FDInlineName = (Fn->getName() + ".inline").str();
-llvm::Module *M = Fn->getParent();
-llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  if (Fn) {
+if (FD->isInlineBuiltinDeclaration()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  llvm::Function *Clone = M->getFunction(FDInlineName);
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// 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.
+else if (llvm::any_of(FD->redecls(), [](const FunctionDecl *FD) {
+   return FD->isInlineBuiltinDeclaration();
+ })) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  if (llvm::Function *Clone = M->getFunction(FDInlineName)) {
+Clone->replaceAllUsesWith(Fn);
+Clone->eraseFromParent();
+  }
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 382337.
serge-sans-paille added a comment.

- Add a test case to ensure we keep the right behavior for non-intrinsic gnu 
inline
- walk the redecl chain before doing an extra string alloc


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

https://reviews.llvm.org/D112059

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/strlen-inline-builtin-redecl.c
  clang/test/CodeGen/user-func-gnu-inline-redecl.c

Index: clang/test/CodeGen/user-func-gnu-inline-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/user-func-gnu-inline-redecl.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -O1 -o - %s | FileCheck %s
+//
+// Verifies that the gnu_inline version is ignored in favor of the redecl
+
+extern inline __attribute__((gnu_inline)) unsigned long some_size(int c) {
+  return 1;
+}
+unsigned long mycall(int s) {
+  // CHECK-LABEL: i64 @mycall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
+unsigned long some_size(int c) {
+  return 2;
+}
+unsigned long yourcall(int s) {
+  // CHECK-LABEL: i64 @yourcall
+  // CHECK: ret i64 2
+  return some_size(s);
+}
Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,36 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
-std::string FDInlineName = (Fn->getName() + ".inline").str();
-llvm::Module *M = Fn->getParent();
-llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  if (Fn) {
+if (FD->isInlineBuiltinDeclaration()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  llvm::Function *Clone = M->getFunction(FDInlineName);
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// 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.
+else if (llvm::any_of(FD->redecls(), [](const FunctionDecl *FD) {
+   return FD->isInlineBuiltinDeclaration();
+ })) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  if (llvm::Function *Clone = M->getFunction(FDInlineName)) {
+Clone->replaceAllUsesWith(Fn);
+Clone->eraseFromParent();
+  }
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:
> > > > 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?
> > > I think you have a valid concern about the extra allocations, but I'm not 
> > > certain of a better predicate to use. My intuition is that the overhead 
> > > here won't be unacceptable, but it'd be good to know we're not regressing 
> > > compile time performance significantly.
> > > 
> > > Additional test coverage with a comment as to why we're testing it is a 
> > > good idea!
> > Perhaps we can first test whether this FuctionDecl is a redecl, then do the 
> > allocation, then check if the `.inline` suffix?
> > 
> > That way we avoid creating the new string unless we're codgen'ing a redecl, 
> > which should be uncommon in practice.
> That could save us a bit of perf, but I think redecls are actually quite 
> common because a definition is itself a declaration, so having a decl in a 
> header file and defn in a source file will produce a redecl chain.
Ah, ok then.

> Additional test coverage with a comment as to why we're testing it is a good 
> idea!

Yeah, with that and fixes to the other small nits in the test case, then I 
think this is ready to land.


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

[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:
> > > 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?
> > I think you have a valid concern about the extra allocations, but I'm not 
> > certain of a better predicate to use. My intuition is that the overhead 
> > here won't be unacceptable, but it'd be good to know we're not regressing 
> > compile time performance significantly.
> > 
> > Additional test coverage with a comment as to why we're testing it is a 
> > good idea!
> Perhaps we can first test whether this FuctionDecl is a redecl, then do the 
> allocation, then check if the `.inline` suffix?
> 
> That way we avoid creating the new string unless we're codgen'ing a redecl, 
> which should be uncommon in practice.
That could save us a bit of perf, but I think redecls are actually quite common 
because a definition is itself a declaration, so having a decl in a header file 
and defn in a source file will produce a redecl chain.


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

[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:
> > > > 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?
> I think you have a valid concern about the extra allocations, but I'm not 
> certain of a better predicate to use. My intuition is that the overhead here 
> won't be unacceptable, but it'd be good to know we're not regressing compile 
> time performance significantly.
> 
> Additional test coverage with a comment as to why we're testing it is a good 
> idea!
Perhaps we can first test whether this FuctionDecl is a redecl, then do the 
allocation, then check if the `.inline` suffix?

That way we avoid creating the new string unless we're codgen'ing a redecl, 
which should be uncommon in practice.


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

[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:
> > > 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?
I think you have a valid concern about the extra allocations, but I'm not 
certain of a better predicate to use. My intuition is that the overhead here 
won't be unacceptable, but it'd be good to know we're not regressing compile 
time performance significantly.

Additional test coverage with a comment as to why we're testing it is a good 
idea!


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

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 
 Tue Sep 
28 13:24:25 2021 +0200 (breakage)
- bd379915de38a9af3d65e19075a6a64ebbb8d6db 
 Tue Sep 
28 16:07:33 2021 +0200 (attempted fix forward of 3d6f49a56995b845 
)
- 0d76d4833dd2815e0b1c786250f474d222f6a0a1 
 Tue Sep 
28 11:30:37 2021 -0700 (revert of 3d6f49a56995b845 
)
- c3717b6858d32d64514a187ede1a77be8ba4e542 
 Tue Sep 
28 21:00:47 2021 +0200 (reland, introduced kernel breakage)
- 0f0e31cf511def3e92244e615b2646c1fd0df0cd 
 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added subscribers: aaron.ballman, rsmith.
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();

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.



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.

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.



Comment at: clang/test/CodeGen/strlen-inline-builtin-redecl.c:8
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+

unused decl



Comment at: clang/test/CodeGen/strlen-inline-builtin-redecl.c:10
+
+extern inline __attribute__((always_inline)) __attribute__((gnu_inline)) 
unsigned long strlen(const char *p) {
+  return 1;

Do you mind wrapping this to 80 chars wide? I suspect if you put the two 
`__attribute__`s first, then the formatter will do a better job.  You can also 
combine these, a la `__attribute__((always_inline, gnu_inline))` to cut down on 
line length.


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

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

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 381014.
serge-sans-paille added a comment.

Avoid walking redecls


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

https://reviews.llvm.org/D112059

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/strlen-inline-builtin-redecl.c


Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | 
FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an 
external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,30 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
+  if (Fn) {
 std::string FDInlineName = (Fn->getName() + ".inline").str();
 llvm::Module *M = Fn->getParent();
 llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+if (FD->isInlineBuiltinDeclaration()) {
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// 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.
+else if (Clone) {
+  Clone->replaceAllUsesWith(Fn);
+  Clone->eraseFromParent();
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.


Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,30 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
+  if (Fn) {
 std::string FDInlineName = (Fn->getName() + ".inline").str();
 llvm::Module *M = Fn->getParent();
 llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+if (FD->isInlineBuiltinDeclaration()) {
+  if (!Clone) {
+

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

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 381011.
serge-sans-paille added a comment.

Reduce the number of time we would walk redecls.
Simplify test case


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

https://reviews.llvm.org/D112059

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/strlen-inline-builtin-redecl.c


Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | 
FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an 
external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,34 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
+  if (Fn) {
 std::string FDInlineName = (Fn->getName() + ".inline").str();
 llvm::Module *M = Fn->getParent();
 llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+if (FD->isInlineBuiltinDeclaration()) {
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+
+// detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// 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.
+else if (Clone) {
+  for (auto const *Redecl : FD->redecls()) {
+if (Redecl->isInlineBuiltinDeclaration()) {
+  Clone->replaceAllUsesWith(Fn);
+  Clone->eraseFromParent();
+}
+  }
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.


Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+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;
+}
+unsigned long yourstrlen(char const *s) {
+  return strlen(s);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,34 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
+  if (Fn) {
 std::string FDInlineName = (Fn->getName() + ".inline").str();
 llvm::Module *M = Fn->getParent();
 llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a subscriber: nathanchance.
nickdesaulniers added a comment.

Yes; GCC does behave this way.  It does not consider a non-gnu-inline 
redefinition an error, and it does seem to prefer the non-gnu-inline 
redeclaration when both are present, AFAICT.  The test is verifying that 
behavior correctly.  This patch is fixing the test case, the reported reduced 
cases from @manojgupta link , 
@nathanchance link 
, 
and myself link 
, 
and fixing the kernel builds link 
.

Further, I did a build+boot tests of:

1. mainline x86_64 + CONFIG_FORTIFY_SOURCE=y
2. mainline x86_64 + CONFIG_FORTIFY_SOURCE=y + CONFIG_LTO_CLANG_FULL=y
3. mainline x86_64 + CONFIG_FORTIFY_SOURCE=y + CONFIG_LTO_CLANG_THIN=y




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1318
+// external one. That's GCC behavior too.
+else {
+  for (auto const *Redecl : FD->redecls()) {

do we really want to be iterating every redaclaration like this, even for 
non-inline builtin declarations? Is there a way to avoid the below loop for 
most functions? Perhaps we should be doing this when we hit a redeclaration 
instead? Or wrap all this work in a check that there's a builtin ID associated 
with the FD?



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1328
+  }
+}
+  }

If there are multiple redeclarations, do we want to be erasing the clone each 
time, or can we `break` out of this loop?



Comment at: clang/test/CodeGen/strlen-inline-builtin-redecl.c:10-12
+extern inline __attribute__((always_inline))
+__attribute__((gnu_inline)) unsigned long
+strlen(const char *p) {

Has this example been formatted? Does rotating the attributes to the front help?



Comment at: clang/test/CodeGen/strlen-inline-builtin-redecl.c:42
+}
+extern typeof(strlen) strlen;

I think this line can be dropped without affecting the test. Or consider using 
any of the further reduced test cases?


Repository:
  rG LLVM Github Monorepo

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

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

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

thanks, I can verify that it fixes the crash we were seeing.


Repository:
  rG LLVM Github Monorepo

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

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

[serge via Phabricator via cfe-commits]

serge-sans-paille created this revision.
serge-sans-paille added reviewers: nickdesaulniers, manojgupta.
serge-sans-paille requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Basically, inline builtin definition are shadowed by externally visible
redefinition. This matches GCC behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112059

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/strlen-inline-builtin-redecl.c


Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | 
FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an 
external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+extern inline __attribute__((always_inline))
+__attribute__((gnu_inline)) unsigned long
+strlen(const char *p) {
+  unsigned long ret;
+  unsigned long p_size = __builtin_object_size(p, 0);
+
+  if (p_size == (unsigned long)-1 ||
+  (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
+return __builtin_strlen(p);
+  ret = strnlen(p, p_size);
+  if (p_size <= ret)
+fortify_panic(__func__);
+  return ret;
+}
+char *strim(char *s) {
+  unsigned long size;
+  char *end;
+
+  size = strlen(s);
+  if (!size)
+return s;
+
+  end = s + size - 1;
+  return end;
+}
+unsigned long strlen(const char *s) {
+  const char *sc;
+
+  for (sc = s; *sc != '\0'; ++sc)
+;
+  return sc - s;
+}
+extern typeof(strlen) strlen;
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1299,18 +1299,35 @@
   // When generating code for a builtin with an inline declaration, use a
   // mangled name to hold the actual body, while keeping an external definition
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
-std::string FDInlineName = (Fn->getName() + ".inline").str();
-llvm::Module *M = Fn->getParent();
-llvm::Function *Clone = M->getFunction(FDInlineName);
-if (!Clone) {
-  Clone = llvm::Function::Create(Fn->getFunctionType(),
- llvm::GlobalValue::InternalLinkage,
- Fn->getAddressSpace(), FDInlineName, M);
-  Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  if (Fn) {
+if (FD->isInlineBuiltinDeclaration()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  llvm::Function *Clone = M->getFunction(FDInlineName);
+  if (!Clone) {
+Clone = llvm::Function::Create(Fn->getFunctionType(),
+   llvm::GlobalValue::InternalLinkage,
+   Fn->getAddressSpace(), FDInlineName, M);
+Clone->addFnAttr(llvm::Attribute::AlwaysInline);
+  }
+  Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
+  Fn = Clone;
+}
+// If the inline version is shadowed by a non-inline version, pick the
+// external one. That's GCC behavior too.
+else {
+  for (auto const *Redecl : FD->redecls()) {
+if (Redecl->isInlineBuiltinDeclaration()) {
+  std::string FDInlineName = (Fn->getName() + ".inline").str();
+  llvm::Module *M = Fn->getParent();
+  llvm::Function *Clone = M->getFunction(FDInlineName);
+  if (Clone) {
+Clone->replaceAllUsesWith(Fn);
+Clone->eraseFromParent();
+  }
+}
+  }
 }
-Fn->setLinkage(llvm::GlobalValue::ExternalLinkage);
-Fn = Clone;
   }
 
   // Check if we should generate debug info for this function.


Index: clang/test/CodeGen/strlen-inline-builtin-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/strlen-inline-builtin-redecl.c
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an external definition
+
+// CHECK-NOT: strlen.inline
+
+unsigned long strnlen(const char *, unsigned long);
+void fortify_panic(const char *);
+
+extern inline __attribute__((always_inline))
+__attribute__((gnu_inline)) unsigned long
+strlen(const char *p) {
+  unsigned long ret;
+  unsigned long p_size = __builtin_object_size(p, 0);
+
+  if (p_size == (unsigned long)-1 ||
+  (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
+return __builtin_strlen(p);
+  ret = strnlen(p, p_size);
+  if (p_size <= ret)
+