[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

In D109967#3044464 , @rnk wrote:

> Thanks for doing this, this approach looks like it incorporated my feedback 
> on the previous approach.

Yeah, listened to the wise men... and struggled a lot doing so ;-)




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3177
+
   // PR9614. Avoid cases where the source code is lying to us. An available
   // externally function should have an equivalent function somewhere else,

rnk wrote:
> Can this trivially recursive builtin detection stuff be removed now? That 
> would be great.
Let me give it a try!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Thanks for doing this, this approach looks like it incorporated my feedback on 
the previous approach.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3177
+
   // PR9614. Avoid cases where the source code is lying to us. An available
   // externally function should have an equivalent function somewhere else,

Can this trivially recursive builtin detection stuff be removed now? That would 
be great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Is https://bugs.llvm.org/show_bug.cgi?id=50322 and dup of 
https://bugs.llvm.org/show_bug.cgi?id=23280 ? (Can both be closed?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

And on Windows: http://45.33.8.238/win/46077/summary.html ! Thanks for pointing 
those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

@thakis: fine on OSX: http://45.33.8.238/macm1/18808/summary.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

@thakis if rG1ecb1bc3e214 
 doesn't 
work, I'll revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

For macOS, see eg http://45.33.8.238/macm1/18806/step_7.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks tests on windows and macOS  eg 
http://45.33.8.238/win/46075/step_7.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

@kda : thanks for the builbot reference. The issue you're pointing at occurs 
*before*  bd379915de38a9af3d65e19075a6a64ebbb8d6db 
  which 
specifically fixes the issue spotted by the buildbot. As a consequnece I'm 
relanding the patch on top of that fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kevin Athey via Phabricator via cfe-commits]

kda added a comment.

Broke fast buildbot: https://lab.llvm.org/buildbot/#/builders/5/builds/12360

reverting...

A guide to reproducing sanitizer bots can be found here: 
https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

for the record: I had to apply that extra patch: 
bd379915de38a9af3d65e19075a6a64ebbb8d6db 
 which 
enforces the `always_inline` attribute


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

@kees great, thanks for confirming - I just landed the patch. Can you share a 
pointer to the missing pieces? I'm interested in implemented the missing parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline 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 rG3d6f49a56995: Simplify handling of builtin with inline 
redefinition (authored by serge-sans-paille).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/memcpy-inline-builtin.c
  clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
  clang/test/CodeGen/memcpy-nobuiltin.c
  clang/test/CodeGen/pr9614.c

Index: clang/test/CodeGen/pr9614.c
===
--- clang/test/CodeGen/pr9614.c
+++ clang/test/CodeGen/pr9614.c
@@ -32,14 +32,14 @@
 
 // CHECK-LABEL: define{{.*}} void @f()
 // CHECK: call void @foo()
-// CHECK: call i32 @abs(i32 0)
+// CHECK: call i32 @abs(i32 %0)
 // CHECK: call i8* @strrchr(
 // CHECK: call void @llvm.prefetch.p0i8(
 // CHECK: call i8* @memchr(
 // CHECK: ret void
 
 // CHECK: declare void @foo()
-// CHECK: declare i32 @abs(i32
 // CHECK: declare i8* @strrchr(i8*, i32)
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(
Index: clang/test/CodeGen/memcpy-nobuiltin.c
===
--- clang/test/CodeGen/memcpy-nobuiltin.c
+++ clang/test/CodeGen/memcpy-nobuiltin.c
@@ -4,7 +4,8 @@
 //
 // CHECK-WITH-DECL-NOT: @llvm.memcpy
 // CHECK-NO-DECL: @llvm.memcpy
-// CHECK-SELF-REF-DECL: @llvm.memcpy
+// CHECK-SELF-REF-DECL-LABEL: define dso_local i8* @memcpy.inline
+// CHECK-SELF-REF-DECL:   @memcpy(
 //
 #include 
 void test(void *dest, void const *from, size_t n) {
Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
+++ /dev/null
@@ -1,25 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-//
-// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
-// if the builtin isn't emittable.
-
-typedef unsigned long size_t;
-
-// always_inline is used so clang will emit this body. Otherwise, we need >=
-// -O1.
-#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
-__attribute__((gnu_inline))
-
-AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
-  return __builtin_memcpy(a, b, c);
-}
-
-// CHECK-LABEL: define{{.*}} void @foo
-void foo(void *a, const void *b, size_t c) {
-  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
-  // later on if optimizations are enabled.
-  // CHECK: call i8* @memcpy
-  memcpy(a, b, c);
-}
-
-// CHECK-NOT: nobuiltin
Index: clang/test/CodeGen/memcpy-inline-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-inline-builtin.c
@@ -0,0 +1,44 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -o - %s | FileCheck %s
+//
+// Verifies that clang detects memcpy inline version and uses it instead of the builtin.
+
+typedef unsigned long size_t;
+
+// Clang requires these attributes for a function to be redefined.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) __attribute__((gnu_inline))
+
+// Clang recognizes an inline builtin and renames it to prevent conflict with builtins.
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  asm("# memcpy.inline marker");
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[A_ADDR_I:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[B_ADDR_I:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[C_ADDR_I:%.*]] = alloca i64, align 8
+// CHECK-NEXT:[[A_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[B_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[C_ADDR:%.*]] = alloca i64, align 8
+// CHECK-NEXT:store i8* [[A:%.*]], i8** [[A_ADDR]], align 8
+// CHECK-NEXT:store i8* [[B:%.*]], i8** [[B_ADDR]], align 8
+// CHECK-NEXT:store i64 [[C:%.*]], i64* [[C_ADDR]], align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8*, i8** [[A_ADDR]], align 8
+// CHECK-NEXT:[[TMP1:%.*]] = load i8*, i8** [[B_ADDR]], align 8
+// CHECK-NEXT:[[TMP2:%.*]] = load i64, i64* [[C_ADDR]], align 8
+// CHECK-NEXT:store i8* [[TMP0]], i8** [[A_ADDR_I]], align 8
+// CHECK-NEXT:store i8* [[TMP1]], i8** [[B_ADDR_I]], align 8
+// CHECK-NEXT:store i64 [[TMP2]], i64* [[C_ADDR_I]], align 8
+// CHECK-NEXT:call void asm sideeffect "# memcpy.inline marker", "~{dirflag},~{fpsr},~{flags}"() #[[ATTR2:[0-9]+]], !srcloc !2
+// CHECK-NEXT:[[TMP3:%.*]] = load i8*, i8** [[A_ADDR_I]], align 8
+// CHECK-NEXT:[[TMP4:%.*]] = load 

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Yeah, I can confirm that many cases are improved with this patch (others are 
more sensitive and depend on the __bos behavior I mentioned). With the 17 
kernel FORTIFY self-tests, all 17 fail without this patch. With this patch, 9 
start passing. Nice!


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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

I'm setting up to test this patch (thank you!) using my current kernel FORTIFY 
improvements. Right now I have a bunch of compile-time behavior selftests 
written:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/overflow=3c5221f3f4fd865a780f544b72c68f4209bd2e76

It should be possible to do an A/B test against those from the kernel's view of 
its FORTIFY functions. However, due to other bugs with __builtin_object_size(), 
the kernel still can't use name-matched inlines:
https://github.com/ClangBuiltLinux/linux/issues/1401
i.e. D109967  will fix half of what is 
needed, but the plan in the kernel right now is to work around the problem 
entirely by using macros instead.

I'll report back on the results of my testing, though, since it should give a 
good sense of how much coverage the kernel can gain back with this bug fixed.


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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

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

Set default as suggested by @nickdesaulniers


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

https://reviews.llvm.org/D109967

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/memcpy-inline-builtin.c
  clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
  clang/test/CodeGen/memcpy-nobuiltin.c
  clang/test/CodeGen/pr9614.c

Index: clang/test/CodeGen/pr9614.c
===
--- clang/test/CodeGen/pr9614.c
+++ clang/test/CodeGen/pr9614.c
@@ -32,14 +32,14 @@
 
 // CHECK-LABEL: define{{.*}} void @f()
 // CHECK: call void @foo()
-// CHECK: call i32 @abs(i32 0)
+// CHECK: call i32 @abs(i32 %0)
 // CHECK: call i8* @strrchr(
 // CHECK: call void @llvm.prefetch.p0i8(
 // CHECK: call i8* @memchr(
 // CHECK: ret void
 
 // CHECK: declare void @foo()
-// CHECK: declare i32 @abs(i32
 // CHECK: declare i8* @strrchr(i8*, i32)
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(
Index: clang/test/CodeGen/memcpy-nobuiltin.c
===
--- clang/test/CodeGen/memcpy-nobuiltin.c
+++ clang/test/CodeGen/memcpy-nobuiltin.c
@@ -4,7 +4,8 @@
 //
 // CHECK-WITH-DECL-NOT: @llvm.memcpy
 // CHECK-NO-DECL: @llvm.memcpy
-// CHECK-SELF-REF-DECL: @llvm.memcpy
+// CHECK-SELF-REF-DECL-LABEL: define dso_local i8* @memcpy.inline
+// CHECK-SELF-REF-DECL:   @memcpy(
 //
 #include 
 void test(void *dest, void const *from, size_t n) {
Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
+++ /dev/null
@@ -1,25 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-//
-// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
-// if the builtin isn't emittable.
-
-typedef unsigned long size_t;
-
-// always_inline is used so clang will emit this body. Otherwise, we need >=
-// -O1.
-#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
-__attribute__((gnu_inline))
-
-AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
-  return __builtin_memcpy(a, b, c);
-}
-
-// CHECK-LABEL: define{{.*}} void @foo
-void foo(void *a, const void *b, size_t c) {
-  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
-  // later on if optimizations are enabled.
-  // CHECK: call i8* @memcpy
-  memcpy(a, b, c);
-}
-
-// CHECK-NOT: nobuiltin
Index: clang/test/CodeGen/memcpy-inline-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-inline-builtin.c
@@ -0,0 +1,44 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -o - %s | FileCheck %s
+//
+// Verifies that clang detects memcpy inline version and uses it instead of the builtin.
+
+typedef unsigned long size_t;
+
+// Clang requires these attributes for a function to be redefined.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) __attribute__((gnu_inline))
+
+// Clang recognizes an inline builtin and renames it to prevent conflict with builtins.
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  asm("# memcpy.inline marker");
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[A_ADDR_I:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[B_ADDR_I:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[C_ADDR_I:%.*]] = alloca i64, align 8
+// CHECK-NEXT:[[A_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[B_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[C_ADDR:%.*]] = alloca i64, align 8
+// CHECK-NEXT:store i8* [[A:%.*]], i8** [[A_ADDR]], align 8
+// CHECK-NEXT:store i8* [[B:%.*]], i8** [[B_ADDR]], align 8
+// CHECK-NEXT:store i64 [[C:%.*]], i64* [[C_ADDR]], align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8*, i8** [[A_ADDR]], align 8
+// CHECK-NEXT:[[TMP1:%.*]] = load i8*, i8** [[B_ADDR]], align 8
+// CHECK-NEXT:[[TMP2:%.*]] = load i64, i64* [[C_ADDR]], align 8
+// CHECK-NEXT:store i8* [[TMP0]], i8** [[A_ADDR_I]], align 8
+// CHECK-NEXT:store i8* [[TMP1]], i8** [[B_ADDR_I]], align 8
+// CHECK-NEXT:store i64 [[TMP2]], i64* [[C_ADDR_I]], align 8
+// CHECK-NEXT:call void asm sideeffect "# memcpy.inline marker", "~{dirflag},~{fpsr},~{flags}"() #[[ATTR2:[0-9]+]], !srcloc !2
+// CHECK-NEXT:[[TMP3:%.*]] = load i8*, i8** [[A_ADDR_I]], align 8
+// CHECK-NEXT:[[TMP4:%.*]] = load i8*, i8** [[B_ADDR_I]], align 8
+// CHECK-NEXT:[[TMP5:%.*]] = load i64, i64* [[C_ADDR_I]], align 8
+// CHECK-NEXT:call void 

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille marked 3 inline comments as done.
serge-sans-paille added a comment.

In D109967#3013552 , @nickdesaulniers 
wrote:

> Looks reasonable. Can you give us some time to test this on the Linux kernel?

Sure, who can refuse some extra testing? Please ping the thread once you're 
done with the extra testing.


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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Nick Desaulniers via Phabricator via cfe-commits]

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

Looks reasonable. Can you give us some time to test this on the Linux kernel?




Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:3
+
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | 
FileCheck %s
+//

are you able to leave off the `-unknown-unknown` from the triple? I assume 
those are the defaults; I know you can with `llc` but not sure about `clang`.


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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille marked 6 inline comments as done.
serge-sans-paille added inline comments.



Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s 
-disable-llvm-passes | FileCheck %s
+//

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > Would `-emit-codegen-only` be more appropriate here than 
> > `-disable-llvm-passes`?
> No; but I don't think we need `-disable-llvm-passes` since we haven't 
> explicitly enabled additional optimization modes via `-O` flags? ie. I 
> suspect if we drop `-disable-llvm-passes` then nothing changes?
`-disable-llvm-passes` was tehre to disable the always inliner. I used another 
approach to keep merker from the inlined function and keep the test easy to 
review.



Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:7
+
+// always_inline is used so clang will emit this body. Otherwise, we need >= 
-O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \

nickdesaulniers wrote:
> I'm not sure what `this` refers to in the comment?
> 
> Also, this whole bit about `always_inline` smells fishy to me.  The semantics 
> of `gnu_inline` is that no body is emitted.  If you //don't// want a body 
> //don't// use `gnu_inline`.  Or was this set of qualifiers+fn attrs taken 
> directly from the kernel, or glibc?  
> `FunctionDecl::isInlineBuiltinDeclaration` only checks for builtin ID, has 
> body, and whether inline was specified.
Comment updated. Basically these attributes are needed for a function to be 
considered by clang as redfinable, which is the case here. these attributes are 
extensively used in glibc headers for that purpose.



Comment at: clang/test/CodeGen/pr9614.c:44
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(

nickdesaulniers wrote:
> Can you elaborate on these changes? Surprising.
Before the patch, the redefined version of bas was ignored as the function is a 
trivially recursive redefinition. Thanks to current patch, the redefinition is 
correctly detected, renamed and inlined (the always inliner still runs here)


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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 373801.
serge-sans-paille added a comment.
Herald added a subscriber: whisperity.

Update formatting / extra comments

Update test to be more explicit about their intent / run them through 
update_cc_test_checks


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

https://reviews.llvm.org/D109967

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/memcpy-inline-builtin.c
  clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
  clang/test/CodeGen/memcpy-nobuiltin.c
  clang/test/CodeGen/pr9614.c
  clang/tools/scan-build-py/lib/libscanbuild/analyze.py

Index: clang/tools/scan-build-py/lib/libscanbuild/analyze.py
===
--- clang/tools/scan-build-py/lib/libscanbuild/analyze.py
+++ clang/tools/scan-build-py/lib/libscanbuild/analyze.py
@@ -39,7 +39,7 @@
 
 __all__ = ['scan_build', 'analyze_build', 'analyze_compiler_wrapper']
 
-scanbuild_dir = os.path.dirname(__import__('sys').argv[0])
+scanbuild_dir = os.path.dirname(os.path.realpath(__import__('sys').argv[0]))
 
 COMPILER_WRAPPER_CC = os.path.join(scanbuild_dir, '..', 'libexec', 'analyze-cc')
 COMPILER_WRAPPER_CXX = os.path.join(scanbuild_dir, '..', 'libexec', 'analyze-c++')
Index: clang/test/CodeGen/pr9614.c
===
--- clang/test/CodeGen/pr9614.c
+++ clang/test/CodeGen/pr9614.c
@@ -32,14 +32,14 @@
 
 // CHECK-LABEL: define{{.*}} void @f()
 // CHECK: call void @foo()
-// CHECK: call i32 @abs(i32 0)
+// CHECK: call i32 @abs(i32 %0)
 // CHECK: call i8* @strrchr(
 // CHECK: call void @llvm.prefetch.p0i8(
 // CHECK: call i8* @memchr(
 // CHECK: ret void
 
 // CHECK: declare void @foo()
-// CHECK: declare i32 @abs(i32
 // CHECK: declare i8* @strrchr(i8*, i32)
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(
Index: clang/test/CodeGen/memcpy-nobuiltin.c
===
--- clang/test/CodeGen/memcpy-nobuiltin.c
+++ clang/test/CodeGen/memcpy-nobuiltin.c
@@ -4,7 +4,8 @@
 //
 // CHECK-WITH-DECL-NOT: @llvm.memcpy
 // CHECK-NO-DECL: @llvm.memcpy
-// CHECK-SELF-REF-DECL: @llvm.memcpy
+// CHECK-SELF-REF-DECL-LABEL: define dso_local i8* @memcpy.inline
+// CHECK-SELF-REF-DECL:   @memcpy(
 //
 #include 
 void test(void *dest, void const *from, size_t n) {
Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
+++ /dev/null
@@ -1,25 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-//
-// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
-// if the builtin isn't emittable.
-
-typedef unsigned long size_t;
-
-// always_inline is used so clang will emit this body. Otherwise, we need >=
-// -O1.
-#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
-__attribute__((gnu_inline))
-
-AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
-  return __builtin_memcpy(a, b, c);
-}
-
-// CHECK-LABEL: define{{.*}} void @foo
-void foo(void *a, const void *b, size_t c) {
-  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
-  // later on if optimizations are enabled.
-  // CHECK: call i8* @memcpy
-  memcpy(a, b, c);
-}
-
-// CHECK-NOT: nobuiltin
Index: clang/test/CodeGen/memcpy-inline-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-inline-builtin.c
@@ -0,0 +1,44 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+//
+// Verifies that clang detects memcpy inline version and uses it instead of the builtin.
+
+typedef unsigned long size_t;
+
+// Clang requires these attributes for a function to be redefined.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) __attribute__((gnu_inline))
+
+// Clang recognizes an inline builtin and renames it to prevent conflict with builtins.
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  asm("# memcpy.inline marker");
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[A_ADDR_I:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[B_ADDR_I:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[C_ADDR_I:%.*]] = alloca i64, align 8
+// CHECK-NEXT:[[A_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[B_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:[[C_ADDR:%.*]] = alloca i64, align 8
+// CHECK-NEXT:store i8* [[A:%.*]], i8** [[A_ADDR]], align 8
+// CHECK-NEXT:store i8* [[B:%.*]], i8** [[B_ADDR]], align 8
+// CHECK-NEXT:store 

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s 
-disable-llvm-passes | FileCheck %s
+//

nickdesaulniers wrote:
> Would `-emit-codegen-only` be more appropriate here than 
> `-disable-llvm-passes`?
No; but I don't think we need `-disable-llvm-passes` since we haven't 
explicitly enabled additional optimization modes via `-O` flags? ie. I suspect 
if we drop `-disable-llvm-passes` then nothing changes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Please amend the bug description to link to:

- https://bugs.llvm.org/show_bug.cgi?id=50322
- https://lore.kernel.org/lkml/20210822075122.864511-17-keesc...@chromium.org/

(You can do `git commit --amend; arc diff --verbatim` to have `arc` update the 
patch description in phab and keep it in sync with local changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Thanks for the patch!

Please fix the lint checks.  `git-clang-format HEAD~` should help, and IIRC 
there is a git hook when using `arc diff` (though maybe that requires one time 
setup?  I seem to have an `.arclint` file in my `llvm-projects` checkout.




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1306
+Fn->setName(Fn->getName() + ".inline");
+  }
+

avoid `{}` for single statement conditionals.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements



Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s 
-disable-llvm-passes | FileCheck %s
+//

Would `-emit-codegen-only` be more appropriate here than `-disable-llvm-passes`?



Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:7
+
+// always_inline is used so clang will emit this body. Otherwise, we need >= 
-O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \

I'm not sure what `this` refers to in the comment?

Also, this whole bit about `always_inline` smells fishy to me.  The semantics 
of `gnu_inline` is that no body is emitted.  If you //don't// want a body 
//don't// use `gnu_inline`.  Or was this set of qualifiers+fn attrs taken 
directly from the kernel, or glibc?  `FunctionDecl::isInlineBuiltinDeclaration` 
only checks for builtin ID, has body, and whether inline was specified.



Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:20
+void foo(void *a, const void *b, size_t c) {
+  // CHECK: call i8* @memcpy.inline
+  memcpy(a, b, c);

Can you use `llvm/utils/update_cc_test_checks.py` to autogen full checks for 
this file? I think it would be helpful to reviewers to be able to see 
everything that's generated here, even if we only really care that the `call` 
is to the `.inline` suffixed version.



Comment at: clang/test/CodeGen/pr9614.c:44
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(

Can you elaborate on these changes? Surprising.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

@kees this does fix https://bugs.llvm.org/show_bug.cgi?id=50322  , but only if 
the memcpy "inline definition" is flagged as a `__attribute__((always_inline))`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

Does this address https://bugs.llvm.org/show_bug.cgi?id=50322 ? (I assume this 
is a new version of https://reviews.llvm.org/D92657 ?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967

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

[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

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

It is a common practice in glibc header to provide an inline redefinition of an
existing function. It is especially the case for fortified function.

Clang currently has an imperfect approach to the problem, using a combination of
trivially recursive function detection and noinline attribute.

Simplify the logic by suffixing these functions by `.inline` during codegen, so
that they are not recognized as builtin by llvm.

After that patch, clang passes all tests from:

  https://github.com/serge-sans-paille/fortify-test-suite


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109967

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/memcpy-inline-builtin.c
  clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
  clang/test/CodeGen/memcpy-nobuiltin.c
  clang/test/CodeGen/pr9614.c

Index: clang/test/CodeGen/pr9614.c
===
--- clang/test/CodeGen/pr9614.c
+++ clang/test/CodeGen/pr9614.c
@@ -32,14 +32,14 @@
 
 // CHECK-LABEL: define{{.*}} void @f()
 // CHECK: call void @foo()
-// CHECK: call i32 @abs(i32 0)
+// CHECK: call i32 @abs(i32 %0)
 // CHECK: call i8* @strrchr(
 // CHECK: call void @llvm.prefetch.p0i8(
 // CHECK: call i8* @memchr(
 // CHECK: ret void
 
 // CHECK: declare void @foo()
-// CHECK: declare i32 @abs(i32
 // CHECK: declare i8* @strrchr(i8*, i32)
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(
Index: clang/test/CodeGen/memcpy-nobuiltin.c
===
--- clang/test/CodeGen/memcpy-nobuiltin.c
+++ clang/test/CodeGen/memcpy-nobuiltin.c
@@ -4,7 +4,8 @@
 //
 // CHECK-WITH-DECL-NOT: @llvm.memcpy
 // CHECK-NO-DECL: @llvm.memcpy
-// CHECK-SELF-REF-DECL: @llvm.memcpy
+// CHECK-SELF-REF-DECL-LABEL: define dso_local i8* @memcpy.inline
+// CHECK-SELF-REF-DECL:   @memcpy(
 //
 #include 
 void test(void *dest, void const *from, size_t n) {
Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
+++ /dev/null
@@ -1,25 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-//
-// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
-// if the builtin isn't emittable.
-
-typedef unsigned long size_t;
-
-// always_inline is used so clang will emit this body. Otherwise, we need >=
-// -O1.
-#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
-__attribute__((gnu_inline))
-
-AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
-  return __builtin_memcpy(a, b, c);
-}
-
-// CHECK-LABEL: define{{.*}} void @foo
-void foo(void *a, const void *b, size_t c) {
-  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
-  // later on if optimizations are enabled.
-  // CHECK: call i8* @memcpy
-  memcpy(a, b, c);
-}
-
-// CHECK-NOT: nobuiltin
Index: clang/test/CodeGen/memcpy-inline-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-inline-builtin.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s -disable-llvm-passes | FileCheck %s
+//
+// Verifies that clang detects memcpy inline version and use it correctly.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit this body. Otherwise, we need >= -O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+__attribute__((gnu_inline))
+
+// Clang recognizes an inline builtin and renames it to prevent conflict with
+// builtins.
+
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: define{{.*}} void @foo
+void foo(void *a, const void *b, size_t c) {
+  // CHECK: call i8* @memcpy.inline
+  memcpy(a, b, c);
+}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1293,8 +1293,10 @@
   case MultiVersionKind::None:
 llvm_unreachable("None multiversion type isn't valid here");
   }
+
 }
 
+
   // Make unique name for device side static file-scope variable for HIP.
   if (CGM.getContext().shouldExternalizeStaticVar(ND) &&
   CGM.getLangOpts().GPURelocatableDeviceCode &&
@@ -3146,6 +3148,7 @@
   if (getFunctionLinkage(GD) != llvm::Function::AvailableExternallyLinkage)
 return true;
   const auto *F = cast(GD.getDecl());
+
   if