[CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

[george.burgess.iv]

Summary

This suboptimality was encountered in Linux (see some discussion on D71082, and https://github.com/ClangBuiltLinux/linux/issues/979).

While it'd be great to have the kernel's FORTIFY working well with clang, it seems best to preserve old functionality in cases where D71082 can't emit a FORTIFY'ed wrapper of a builtin. With this patch, the memcpy in the test-case here https://reviews.llvm.org/D71082#1953975 compiles back to a mov.

[efriedma]

LGTM

[george.burgess.iv]

thanks!

[mstorsjo]

This triggers failed asserts:

$ cat dcpdecrypt.cpp
extern "C" __inline__ __attribute__((__gnu_inline__)) void *
memcpy(void *, const void *, unsigned) {}
void *memcpy(void *, const void *, unsigned);
void a() { memcpy; }
$ clang++ -c dcpdecrypt.cpp -target i686-linux-gnu
clang++: ../tools/clang/lib/AST/Decl.cpp:3436: bool clang::FunctionDecl::isInlineDefinitionExternallyVisible() const: Assertion `(doesThisDeclarationHaveABody() || willHaveBody() || hasAttr<AliasAttr>()) && "Must be
a function definition"' failed.

[george.burgess.iv]

Thanks for the report! Looking now.

[nickdesaulniers]

The same failing assertion is tripping up Linaro's TCWG Linux kernel builds:

• https://ci.linaro.org/job/tcwg_kernel-bisect-llvm-master-arm-mainline-allmodconfig/33/artifact/artifacts/build-2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a/03-build_linux/console.log
• https://ci.linaro.org/job/tcwg_kernel-bisect-llvm-master-aarch64-lts-allyesconfig/30/artifact/artifacts/build-2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a/03-build_linux/console.log

Funny enough, I think I touched that assertion last as one of the final bug fixes to the clang-9 release.

[melver]

This seems to be causing problems for the Linux kernel with ASan instrumentation enabled: https://lkml.kernel.org/r/[e-mail]

Still investigating if it's the kernel, KASAN, or Clang that's doing something wrong. Any pointers welcome.

Many thanks!

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::SetFunctionAttributes(GlobalDecl GD, llvm::Function *F,
   if (!IsIncompleteFunction && F->isDeclaration())
     getTargetCodeGenInfo().setTargetAttributes(FD, F, *this);
 
   if (const auto *CSA = FD->getAttr<CodeSegAttr>())
     F->setSection(CSA->getName());
   else if (const auto *SA = FD->getAttr<SectionAttr>())
      F->setSection(SA->getName());
 
-  if (FD->isInlineBuiltinDeclaration()) {
+  // If we plan on emitting this inline builtin, we can't treat it as a builtin.
+  if (FD->isInlineBuiltinDeclaration() && shouldEmitFunction(FD)) {
     F->addAttribute(llvm::AttributeList::FunctionIndex,
                     llvm::Attribute::NoBuiltin);
   }
 
   if (FD->isReplaceableGlobalAllocationFunction()) {
     // A replaceable global allocation function does not act like a builtin by
     // default, only if it is invoked by a new-expression or delete-expression.
     F->addAttribute(llvm::AttributeList::FunctionIndex,

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