[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

5060

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

5060

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

5060

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

5059

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

[steplong]

• Fixed comment about attribute((no_builtin))

[hans]

lgtm

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 AttributeNoBuiltin = "no-builtin-" + FD->getName().str();
     std::string FDInlineName = (FD->getName() + ".inline").str();
     // 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"))) on the current
+    // function.
+    else if (!CGF.CurFn->getAttributes().hasFnAttr(AttributeNoBuiltin))
       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/lib/Sema/SemaDeclAttr.cpp

if (AL.getNumArgs() == 0)
     AddBuiltinName(kWildcard);
   else
     for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
       StringRef BuiltinName;
       SourceLocation LiteralLoc;
       if (!S.checkStringLiteralArgumentAttr(AL, I, BuiltinName, &LiteralLoc))
         return;
 
-      if (Builtin::Context::isBuiltinFunc(BuiltinName))
+      bool ParsedAttrIsWildCard = BuiltinName == kWildcard;
+      if (Builtin::Context::isBuiltinFunc(BuiltinName) || ParsedAttrIsWildCard)
         AddBuiltinName(BuiltinName);
       else
         S.Diag(LiteralLoc, diag::warn_attribute_no_builtin_invalid_builtin_name)
             << BuiltinName << AL;
     }
 
   // Repeating the same attribute is fine.
   llvm::sort(Names);

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);
+
+// CHECK: define{{.*}} void @foo1({{.*}}) #[[NO_NOBUILTIN:[0-9]+]]
+// CHECK:   call void @bar
+// CHECK:   call void @llvm.memset
+// CHECK:   call void @llvm.memcpy
+void foo1(char *s, char *d, size_t n)
+{
+    bar(s);
+    memset(s, 0, n);
+    memcpy(d, s, n);
+}
+
+// CHECK: define{{.*}} void @foo2({{.*}}) #[[NOBUILTIN_MEMSET:[0-9]+]]
+// CHECK:   call void @bar
+// CHECK:   {{.*}}call {{.*}} @memset
+// CHECK:   call void @llvm.memcpy
+void foo2(char *s, char *d, size_t n) __attribute__((no_builtin("memset")))
+{
+    bar(s);
+    memset(s, 1, n);
+    memcpy(d, s, n);
+}
+
+// CHECK: define{{.*}} void @foo3({{.*}}) #[[NOBUILTIN_MEMSET_MEMCPY:[0-9]+]]
+// CHECK:   call void @bar
+// CHECK:   {{.*}}call {{.*}} @memset
+// CHECK:   {{.*}}call {{.*}} @memcpy
+void foo3(char *s, char *d, size_t n) __attribute__((no_builtin("memset", "memcpy")))
+{
+    bar(s);
+    memset(s, 2, n);
+    memcpy(d, s, n);
+}
+
+// CHECK-NOT: attributes #[[NO_NOBUILTIN]] = {{{.*}}"no-builtin-memset"{{.*}}}
+// CHECK-NOT: attributes #[[NO_NOBUILTIN]] = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
+// CHECK:     attributes #[[NOBUILTIN_MEMSET]] = {{{.*}}"no-builtin-memset"{{.*}}}
+// CHECK:     attributes #[[NOBUILTIN_MEMSET_MEMCPY]] = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}