gchatelet marked 2 inline comments as done.
gchatelet added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+ if (D->hasAttr<NoBuiltinAttr>())
+ D->dropAttr<NoBuiltinAttr>();
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68028/new/
https://reviews.llvm.org/D68028
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
