[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked 3 inline comments as done.
gchatelet added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+  if (D->hasAttr<NoBuiltinAttr>())
+    D->dropAttr<NoBuiltinAttr>();
+
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > Just making sure I understand the semantics you want: redeclarations do not have to have matching lists of builtins, but instead the definition will use a merged list? e.g.,
> > ```
> > [[clang::no_builtin("memset")]] void whatever();
> > [[clang::no_builtin("memcpy")]] void whatever();
> > 
> > [[clang::no_builtin("memmove")]] void whatever() {
> >  // Will not use memset, memcpy, or memmove builtins.
> > }
> > ```
> > That seems a bit strange, to me. In fact, being able to apply this attribute to a declaration seems a bit like a mistake -- this only impacts the definition of the function, and I can't imagine good things coming from hypothetical code like:
> > ```
> > [[clang::no_builtin("memset")]] void whatever();
> > #include "whatever.h" // Provides a library declaration of whatever() with no restrictions on it
> > ```
> > WDYT about restricting this attribute to only appear on definitions?
> That's a very good point. Thx for noticing.
> Indeed I think it only makes sense to have the attribute on the function definition.
> 
> I've tried to to use `FunctionDecl->hasBody()` during attribute handling in the Sema phase but it seems like the `FunctionDecl` is not complete at this point.
> All calls to `hasBody()` return `false`, if I repeat the operation in `CGCall` then `hasBody` returns `true` and I can see the `CompoundStatement`.
> 
> Do you have any recommendations on where to perform the check?
So after some investigations it turns out that `FunctionDecl::isThisDeclarationADefinition` is buggy (returns always `false`) when called from `ProcessDeclAttribute`.
I believe it's because the `CompoundStatement` is not parsed at this point.

As a matter of fact all code using this function in `ProcessDeclAttribute` is dead code (see D68379 which disables the dead code, tests are still passing)


I'm unsure of how to fix this, it may be possible of using `FunctionDecl::setWillHaveBody`in [[ https://github.com/llvm/llvm-project/blob/0577a0cedbc5be4cd4c20ba53d3dbdac6bff9a0a/clang/lib/Sema/SemaDecl.cpp#L8820 | this switch ]].


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68028/new/

https://reviews.llvm.org/D68028