Simplify handling of builtin with inline redefinition

[serge-sans-paille]

Summary

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

[kees]

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 ?)

[serge-sans-paille]

@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))

[nickdesaulniers]

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.

clang/lib/CodeGen/CodeGenFunction.cpp

1306

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

clang/test/CodeGen/memcpy-inline-builtin.c

2

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

8

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.

21

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.

clang/test/CodeGen/pr9614.c

44

Can you elaborate on these changes? Surprising.

[nickdesaulniers]

Please amend the bug description to link to:

• https://bugs.llvm.org/show_bug.cgi?id=50322
• https://lore.kernel.org/lkml/[e-mail]/

(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.

[nickdesaulniers]

clang/test/CodeGen/memcpy-inline-builtin.c

2

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?

[serge-sans-paille]

Update formatting / extra comments

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

[serge-sans-paille]

clang/test/CodeGen/memcpy-inline-builtin.c

2

-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.

8

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.

clang/test/CodeGen/pr9614.c

44

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)

[nickdesaulniers]

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

clang/test/CodeGen/memcpy-inline-builtin.c

4

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.

[serge-sans-paille]

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.

[serge-sans-paille]

Set default as suggested by @nickdesaulniers

[kees]

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&id=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.

[kees]

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!

[serge-sans-paille]

@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.

[serge-sans-paille]

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

[kda]

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

[serge-sans-paille]

@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.

[thakis]

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.

[thakis]

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

[serge-sans-paille]

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

[serge-sans-paille]

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

[serge-sans-paille]

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

[kees]

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

[rnk]

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

clang/lib/CodeGen/CodeGenModule.cpp

3177

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

[serge-sans-paille]

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 ;-)

clang/lib/CodeGen/CodeGenModule.cpp

3177

Let me give it a try!

clang/lib/CodeGen/CodeGenFunction.cpp

 void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
                                    const CGFunctionInfo &FnInfo) {
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
   CurGD = GD;
 
   FunctionArgList Args;
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
+  // Give a different name to inline builtin to avoid conflict with actual
+  // builtins.
+  if (FD->isInlineBuiltinDeclaration() && Fn)
+    Fn->setName(Fn->getName() + ".inline");
+
   // Check if we should generate debug info for this function.
   if (FD->hasAttr<NoDebugAttr>()) {
     // Clear non-distinct debug info that was possibly attached to the function
     // due to an earlier declaration without the nodebug attribute
     if (Fn)
       Fn->setSubprogram(nullptr);
     // Disable debug info indefinitely for this function
     DebugInfo = nullptr;

clang/lib/CodeGen/CodeGenModule.cpp

if (const CXXDestructorDecl *Dtor = dyn_cast<CXXDestructorDecl>(F)) {
           if (HasNonDllImportDtor(cast<FieldDecl>(Member)->getType()))
             return false;
       for (const CXXBaseSpecifier &B : Dtor->getParent()->bases())
         if (HasNonDllImportDtor(B.getType()))
           return false;
     }
   }
 
+  // Inline builtins declaration must be emitted. They often are fortified
+  // functions.
+  if (F->isInlineBuiltinDeclaration())
+    return true;
+
   // PR9614. Avoid cases where the source code is lying to us. An available
   // externally function should have an equivalent function somewhere else,
   // but a function that calls itself through asm label/`__builtin_` trickery is
   // clearly not equivalent to the real implementation.
   // This happens in glibc's btowc and in some configure checks.
   return !isTriviallyRecursive(F);
 }
 
 bool CodeGenModule::shouldOpportunisticallyEmitVTables() {

clang/test/CodeGen/memcpy-inline-builtin.c

+// 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 @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 [[TMP3]], i8* align 1 [[TMP4]], i64 [[TMP5]], i1 false) #[[ATTR2]]
+// CHECK-NEXT:    ret void
+//
+void foo(void *a, const void *b, size_t c) {
+  memcpy(a, b, c);
+}

clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c

-// 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

clang/test/CodeGen/memcpy-nobuiltin.c

 // RUN: %clang_cc1 -verify -S -emit-llvm -o- %s -isystem %S -DWITH_DECL | FileCheck --check-prefix=CHECK-WITH-DECL %s
 // RUN: %clang_cc1 -verify -S -emit-llvm -o- %s -isystem %S -UWITH_DECL | FileCheck --check-prefix=CHECK-NO-DECL %s
 // RUN: %clang_cc1 -verify -S -emit-llvm -o- %s -isystem %S -DWITH_SELF_REFERENCE_DECL | FileCheck --check-prefix=CHECK-SELF-REF-DECL %s
 //
 // 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 <memcpy-nobuiltin.inc>
 void test(void *dest, void const *from, size_t n) {
   memcpy(dest, from, n);
 
   static char buffer[1];
   memcpy(buffer, from, 2); // expected-warning {{'memcpy' will always overflow; destination buffer has size 1, but size argument is 2}}
 }

clang/test/CodeGen/pr9614.c

void f(void) {
   abs(0);
   strrchr_foo("", '.');
   prefetch();
   memchr("", '.', 0);
 }
 
 // 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(