aaron.ballman added reviewers: erik.pilkington, dexonsmith. aaron.ballman added a comment.
In D55212#1347994 <https://reviews.llvm.org/D55212#1347994>, @arichardson wrote: > I don't see an easy way of fixing the pragma clang attribute support for > this. > Would it be okay to commit this change anyway? It causes a regression in behavior, so I'm not certain we should move forward with it just yet (unless there's a pressing reason?). > Since `alloc_size` is only used for very few functions I'd be very surprised > if there's any existing code that relies on using `#pragma clang atrribute` > with `alloc_size`. This feature has already been available in a public release and there's no way to know how much code it will break (if any). It does seem rather unlikely, but I've been surprised by how people use this feature in the past. > By the way GCC now also supports the attribute on function pointers: > https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=267158 Yeah, my concern isn't with the new functionality so much as it breaking old functionality. I'm also adding Erik and Duncan to the review because they may have some more insights into whether `alloc_size` is being used with `#pragma clang attribute` in the wild. My feeling is: if we can't spot any uses of that feature being used to apply `alloc_size` with a reasonable amount of looking for it, then we can go ahead with this patch even if it removes support for `alloc_size` from the pragma. If we get push back from the community, we can fix or revert at that time. However, given that this is plausibly a breaking change, I'd rather not commit to trunk until after we branch to give folks more time to react. WDYT? ================ Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:15 +// FIXME: After changing the subject from Function to HasFunctionProto, AllocSize is no longer listed (similar to Format, etc) +// FIXME-NEXT: AllocSize (SubjectMatchRule_function) // CHECK-NEXT: AlwaysDestroy (SubjectMatchRule_variable) ---------------- arichardson wrote: > aaron.ballman wrote: > > arichardson wrote: > > > This seems to also affect __attribute((format)) and others so I'm not > > > sure whether removing AllocSize from this list is a blocker for this > > > patch. > > I believe you may need to add an `AttrSubjectMatcherSubRule` in Attr.td to > > expose the attribute for `#pragma clang attribute` support. Can you see if > > you can support it instead of shrinking this list? Otherwise, I worry that > > this change will break code that was using the pragma to apply the > > attribute to declarations. > The problem here already has a fixme in attr.td: > > ``` > // FIXME: There's a matcher ambiguity with objc methods and blocks since > // functionType excludes them but functionProtoType includes them. > // AttrSubjectMatcherSubRule<"functionProtoType", [HasFunctionProto]> > ``` > > It seems like this was already added as part of the initial pragma clang > attribute commit in https://reviews.llvm.org/D30009 > > I had a look at the AttrEmitter.cpp in tablegen but couldn't find a > straightforward solution. > I'll add @arphaman to the review. Ah, good to know that this was already a known issue. Hopefully @arphaman has some ideas on how to support this so we don't remove a supported attribute from the list. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55212/new/ https://reviews.llvm.org/D55212 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
