[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked an inline comment 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:
> > gchatelet wrote:
> > > 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 ]].
> > > So after some investigations it turns out that FunctionDecl::isThisDeclarationADefinition is buggy (returns always false) when called from ProcessDeclAttribute.
> > 
> > That is a bit odd to me; we call it in a half dozen places in SemaDeclAttr.cpp, all of which get called from `ProcessDeclAttribute`. Are ALL of those uses broken currently?!
> > 
> > > 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)
> > 
> > You got four of the six. What about the use in `handleObjCSuppresProtocolAttr()` and the second use in `handleAliasAttr()`?
> > 
> > > I'm unsure of how to fix this, it may be possible of using FunctionDecl::setWillHaveBodyin this switch.
> > 
> > I'm also unsure of a good way forward. @rsmith may have ideas too.
> > That is a bit odd to me; we call it in a half dozen places in SemaDeclAttr.cpp, all of which get called from ProcessDeclAttribute. Are ALL of those uses broken currently?!
> > You got four of the six. What about the use in handleObjCSuppresProtocolAttr() and the second use in handleAliasAttr()?
> 
> It really is `FunctionDecl::isThisDeclarationADefinition` that is buggy, the other places where we call `isThisDeclarationADefinition` in `SemaDeclAttr.cpp` are ok, i.e. `VarDecl::isThisDeclarationADefinition` and `ObjCProtocolDecl::isThisDeclarationADefinition`
> 
> I tried to use `FunctionDecl::setWillHaveBody`but it broke many tests so it seems inappropriate.
> 
I've tried to fix it in D68701. I'm pretty sure this is not the right way to do it though,
@rsmith any idea on how to proceed?

In the meantime I'll add a FIXME and move on with this patch if you don't mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028