This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by steplong on Apr 29 2022, 2:44 PM.

Details

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.

Diff Detail

Event Timeline

steplong created this revision.Apr 29 2022, 2:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 2:44 PM
steplong requested review of this revision.Apr 29 2022, 2:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 2:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hans added a subscriber: hans.May 2 2022, 3:42 AM

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
5219

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

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

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

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.

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 updated this revision to Diff 426400.May 2 2022, 6:47 AM
steplong marked an inline comment as done.
steplong updated this revision to Diff 426700.May 3 2022, 7:13 AM
steplong updated this revision to Diff 426795.May 3 2022, 11:48 AM
steplong added inline comments.May 4 2022, 1:13 PM
clang/lib/CodeGen/CGExpr.cpp
5219

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 updated this revision to Diff 427741.May 6 2022, 2:07 PM

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.

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 updated this revision to Diff 428195.May 9 2022, 1:58 PM

clang-formatted patch

steplong edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.May 11 2022, 11:49 AM
clang/lib/CodeGen/CGExpr.cpp
5196

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 added inline comments.May 11 2022, 2:45 PM
clang/lib/CodeGen/CGExpr.cpp
5219

I think you're looking for isPredefinedLibFunction()

steplong updated this revision to Diff 428970.EditedMay 12 2022, 9:25 AM
steplong edited the summary of this revision. (Show Details)
  • Added isPredefinedLibFunction() check
  • Added testcase for __attribute__((no_builtin("*")))

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 updated this revision to Diff 429703.May 16 2022, 6:56 AM
  • Update documentation
hans added inline comments.May 17 2022, 4:26 AM
clang/test/CodeGen/no-builtin-2.c
52

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

steplong added inline comments.May 17 2022, 6:50 AM
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 updated this revision to Diff 430060.May 17 2022, 7:37 AM
  • Removed __attribute__((no_builtin("*"))) from test
  • Remove the logic that accepted __attribute__((no_builtin("*")))
hans added a comment.May 18 2022, 6:02 AM

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

clang/lib/CodeGen/CGExpr.cpp
5218

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

steplong updated this revision to Diff 430365.May 18 2022, 6:51 AM
  • Fixed comment about attribute((no_builtin))
hans accepted this revision.May 18 2022, 7:57 AM

lgtm

This revision is now accepted and ready to land.May 18 2022, 7:57 AM