aaron.ballman added a subscriber: rsmith.
aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1072
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema &S, Decl *D, const AttributeCommonInfo &CI,
+ llvm::ArrayRef<StringRef> FunctionNames) {
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > You're missing a call to this function within `mergeDeclAttribute()` in
> > SemaDecl.cpp.
> Thx, I rearranged the signature a bit, do you happen to know how
> `mergeDeclAttribute` is tested?
Through redeclarations, e.g.,
```
[[some_attr]] void func();
[[some_other_attr]] void func();
[[a_third_attr, some_attr]] void func() {
}
```
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1086-1089
+ if (FunctionNamesSet.count(Wildcard) > 0) {
+ FunctionNamesSet.clear();
+ FunctionNamesSet.insert(Wildcard);
+ }
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > Rather than walking the entire set like this, would it make more sense to
> > look for the wildcard in the above loop before inserting the name, and set
> > a local variable if found, so that you can do the clear without having to
> > rescan the entire list?
> This is is conflict with the `llvm::copy` suggestion above. Which one do you
> prefer?
Walking the list and not calling `llvm::copy`.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+ if (D->hasAttr<NoBuiltinAttr>())
+ D->dropAttr<NoBuiltinAttr>();
+
----------------
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.
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
