[clang] Honor __attribute__((no_builtin("foo"))) on functions

[steplong]

Summary

Support for __attribute__((no_builtin("foo"))) was added in https://reviews.llvm.org/D68028,
but builtins were still being used even when the attribute was placed on a function.

[hans]

If I understand correctly, D68028 made it so that LLVM doesn't add any builtin calls (e.g. turning for-loops into memcpy), but Clang could still turn calls into builtins. Maybe the patch description could be expanded to explain this?

clang/lib/CodeGen/CGExpr.cpp

5068

What if CurFn has the "wildcard" no-builtins attribute?

[aaron.ballman]

FWIW, it looks like precommit CI found issues in the newly added test which should be addressed.

clang/test/CodeGen/no-builtin-2.c

4

You shouldn't include system headers -- that will pull from whatever is installed on the test system. If you included this for size_t, you can do something like: typedef __typeof_(sizeof(0)) size_t; to work around it.

[steplong]

In D124701#3485317, @hans wrote:

If I understand correctly, D68028 made it so that LLVM doesn't add any builtin calls (e.g. turning for-loops into memcpy), but Clang could still turn calls into builtins. Maybe the patch description could be expanded to explain this?

I'm not too sure, but I think D68028 just adds the attribute nobuiltin, but it gets ignored. It doesn't seem to affect the calls in the function. They're still being converted into builtins even with the attribute on the function.

[steplong]

clang/lib/CodeGen/CGExpr.cpp

5068

Hmmm, good question. Currently, "*" generates a warning. I was able to get "*" to add the attribute no-builtins to the function, but if I try making it affect the calls in the function, I see a compiler crash when compiling compiler-rt. It looks like some builtins must be generated even if -fno-builtins is called such as __sync_lock_test_and_set_1. I'm not sure how to check for that.

[steplong]

Update patch to accept attribute((no_builtin("*")). It adds no-builtins to the function attributes, but it still doesn't affect the calls. The calls in the function still become builtin calls.

[aaron.ballman]

In D124701#3485647, @steplong wrote:

In D124701#3485317, @hans wrote:

If I understand correctly, D68028 made it so that LLVM doesn't add any builtin calls (e.g. turning for-loops into memcpy), but Clang could still turn calls into builtins. Maybe the patch description could be expanded to explain this?

I'm not too sure, but I think D68028 just adds the attribute nobuiltin, but it gets ignored. It doesn't seem to affect the calls in the function. They're still being converted into builtins even with the attribute on the function.

Correct -- that commit was just adding the attribute and ensuring it parsed correctly and met semantic requirements. That's why its documentation says .. Note:: This attribute is not yet fully implemented, it is validated but has no effect on the generated code. There is more interesting discussion in https://reviews.llvm.org/D61634 that relates.

[steplong]

clang-formatted patch

[aaron.ballman]

clang/lib/CodeGen/CGExpr.cpp

5037

Might as well use a Twine since the next line uses one as well.

clang/test/CodeGen/no-builtin-2.c

39

We should have test coverage for the wildcard form of the attribute.

[efriedma]

clang/lib/CodeGen/CGExpr.cpp

5068

I think you're looking for isPredefinedLibFunction()

[steplong]

• Added isPredefinedLibFunction() check
• Added testcase for __attribute__((no_builtin("*")))

[aaron.ballman]

I think the only thing missing is an update to the documentation: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/AttrDocs.td#L5999, and add a release note about the change in functionality.

[steplong]

• Update documentation

[hans]

clang/test/CodeGen/no-builtin-2.c

52

In AttrDocs.td it says the wildcard case is spelled "attribute((no_builtin))".

[steplong]

clang/test/CodeGen/no-builtin-2.c

52

Ah whoops, missed that. I don't think I need the changes in SemaDeclAttr.cpp then. Thanks for catching this!

[steplong]

• Removed __attribute__((no_builtin("*"))) from test
• Remove the logic that accepted __attribute__((no_builtin("*")))

[hans]

Just one more comment, then I'm happy :)

clang/lib/CodeGen/CGExpr.cpp

5067

The comment needs updating too (for the wildcard thing).

[steplong]

• Fixed comment about attribute((no_builtin))

[hans]

lgtm

clang/docs/ReleaseNotes.rst

 - Statement attributes ``[[clang::noinline]]`` and  ``[[clang::always_inline]]``
   can be used to control inlining decisions at callsites.
 
 - ``#pragma clang attribute push`` now supports multiple attributes within a single directive.
 
 - The ``__declspec(naked)`` attribute can no longer be written on a member
   function in Microsoft compatibility mode, matching the behavior of cl.exe.
 
+- Attribute ``no_builtin`` should now affect the generated code. It now disables
+  builtins (corresponding to the specific names listed in the attribute) in the
+  body of the function the attribute is on.
+
 Windows Support
 ---------------
 
 - Add support for MSVC-compatible ``/JMC``/``/JMC-`` flag in clang-cl (supports
   X86/X64/ARM/ARM64). ``/JMC`` could only be used when ``/Zi`` or ``/Z7`` is
   turned on. With this addition, clang-cl can be used in Visual Studio for the
   JustMyCode feature. Note, you may need to manually add ``/JMC`` as additional
   compile options in the Visual Studio since it currently assumes clang-cl does not support ``/JMC``.

clang/include/clang/Basic/AttrDocs.td

 purpose for other architecture, there is a general form for the
 attribute `clang_builtin_alias`.
   }];
 }
 
 def NoBuiltinDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
-.. Note:: This attribute is not yet fully implemented, it is validated but has
-          no effect on the generated code.
-
 The ``__attribute__((no_builtin))`` is similar to the ``-fno-builtin`` flag
 except it is specific to the body of a function. The attribute may also be
 applied to a virtual function but has no effect on the behavior of overriding
 functions in a derived class.
 
 It accepts one or more strings corresponding to the specific names of the
 builtins to disable (e.g. "memcpy", "memset").
 If the attribute is used without parameters it will disable all buitins at

clang/lib/CodeGen/CGExpr.cpp

if (!PD->isInlineBuiltinDeclaration())
       return false;
   return true;
 }
 
 static CGCallee EmitDirectCallee(CodeGenFunction &CGF, GlobalDecl GD) {
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
 
   if (auto builtinID = FD->getBuiltinID()) {
+    std::string NoBuiltinFD = ("no-builtin-" + FD->getName()).str();
+    std::string NoBuiltins = "no-builtins";
     std::string FDInlineName = (FD->getName() + ".inline").str();
+
+    bool IsPredefinedLibFunction =
+        CGF.getContext().BuiltinInfo.isPredefinedLibFunction(builtinID);
+    bool HasAttributeNoBuiltin =
+        CGF.CurFn->getAttributes().hasFnAttr(NoBuiltinFD) ||
+        CGF.CurFn->getAttributes().hasFnAttr(NoBuiltins);
+
     // When directing calling an inline builtin, call it through it's mangled
     // name to make it clear it's not the actual builtin.
     if (CGF.CurFn->getName() != FDInlineName &&
         OnlyHasInlineBuiltinDeclaration(FD)) {
       llvm::Constant *CalleePtr = EmitFunctionDeclPointer(CGF.CGM, GD);
       llvm::Function *Fn = llvm::cast<llvm::Function>(CalleePtr);
       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);
       }
       return CGCallee::forDirect(Clone, GD);
     }
 
     // Replaceable builtins provide their own implementation of a builtin. If we
     // are in an inline builtin implementation, avoid trivial infinite
-    // recursion.
-    else
+    // recursion. Honor __attribute__((no_builtin("foo"))) or
+    // __attribute__((no_builtin)) on the current function unless foo is
+    // not a predefined library function which means we must generate the
+    // builtin no matter what.
+    else if (!IsPredefinedLibFunction || !HasAttributeNoBuiltin)
       return CGCallee::forBuiltin(builtinID, FD);
   }
 
   llvm::Constant *CalleePtr = EmitFunctionDeclPointer(CGF.CGM, GD);
   if (CGF.CGM.getLangOpts().CUDA && !CGF.CGM.getLangOpts().CUDAIsDevice &&
       FD->hasAttr<CUDAGlobalAttr>())
     CalleePtr = CGF.CGM.getCUDARuntime().getKernelStub(
         cast<llvm::GlobalValue>(CalleePtr->stripPointerCasts()));

clang/test/CodeGen/no-builtin-2.c

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+
+typedef typeof(sizeof(0)) size_t;
+
+void bar(char *s);
+void *memset(void *s, int c, size_t n);
+void *memcpy(void *d, const void *s, size_t n);
+void *memmove(void *d, const void *s, size_t n);
+
+// CHECK: define{{.*}} void @foo1({{.*}}) #[[NO_NOBUILTIN:[0-9]+]]
+// CHECK:   call void @bar
+// CHECK:   call void @llvm.memset
+// CHECK:   call void @llvm.memcpy
+// CHECK:   call void @llvm.memmove
+void foo1(char *s, char *d, size_t n) {
+  bar(s);
+  memset(s, 0, n);
+  memcpy(d, s, n);
+  memmove(d, s, n);
+}
+
+// CHECK: define{{.*}} void @foo2({{.*}}) #[[NOBUILTIN_MEMSET:[0-9]+]]
+// CHECK:   call void @bar
+// CHECK:   {{.*}}call {{.*}} @memset
+// CHECK:   call void @llvm.memcpy
+// CHECK:   call void @llvm.memmove
+void foo2(char *s, char *d, size_t n) __attribute__((no_builtin("memset"))) {
+  bar(s);
+  memset(s, 1, n);
+  memcpy(d, s, n);
+  memmove(d, s, n);
+}
+
+// CHECK: define{{.*}} void @foo3({{.*}}) #[[NOBUILTIN_MEMSET_MEMCPY:[0-9]+]]
+// CHECK:   call void @bar
+// CHECK:   {{.*}}call {{.*}} @memset
+// CHECK:   {{.*}}call {{.*}} @memcpy
+// CHECK:   call void @llvm.memmove
+void foo3(char *s, char *d, size_t n) __attribute__((no_builtin("memset", "memcpy"))) {
+  bar(s);
+  memset(s, 2, n);
+  memcpy(d, s, n);
+  memmove(d, s, n);
+}
+
+// CHECK: define{{.*}} void @foo4({{.*}}) #[[NOBUILTINS:[0-9]+]]
+// CHECK:   call void @bar
+// CHECK:   {{.*}}call {{.*}} @memset
+// CHECK:   {{.*}}call {{.*}} @memcpy
+// CHECK:   {{.*}}call {{.*}} @memmove
+void foo4(char *s, char *d, size_t n) __attribute__((no_builtin)) {
+  bar(s);
+  memset(s, 2, n);
+  memcpy(d, s, n);
+  memmove(s, d, n);
+}
+
+// CHECK-NOT: attributes #[[NO_NOBUILTIN]] = {{{.*}}"no-builtin-memset"{{.*}}}
+// CHECK-NOT: attributes #[[NO_NOBUILTIN]] = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
+// CHECK-NOT: attributes #[[NO_NOBUILTIN]] = {{{.*}}"no-builtins"{{.*}}}
+// CHECK:     attributes #[[NOBUILTIN_MEMSET]] = {{{.*}}"no-builtin-memset"{{.*}}}
+// CHECK:     attributes #[[NOBUILTIN_MEMSET_MEMCPY]] = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
+// CHECK:     attributes #[[NOBUILTINS]] = {{{.*}}"no-builtins"{{.*}}}