[PATCH] D55212: Handle alloc_size attribute on function pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think all that's missing are some C++ tests and some nits.



================
Comment at: include/clang/Basic/Attr.td:1072
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
+  let Subjects = SubjectList<[HasFunctionProto]>;
----------------
arichardson wrote:
> aaron.ballman wrote:
> > arichardson wrote:
> > > aaron.ballman wrote:
> > > > arichardson wrote:
> > > > > aaron.ballman wrote:
> > > > > > Does GCC support writing `alloc_size` on function pointers?
> > > > > Yes it does and it seems to be used by some projects. I first discovered this while compiling libxml2: https://github.com/GNOME/libxml2/blob/35e83488505d501864826125cfe6a7950d6cba78/include/libxml/xmlmemory.h#L66
> > > > Parsed and ignored is different than supported. For instance, I can't seem to get GCC to produce different behavior here: https://godbolt.org/z/MI5k_m
> > > > 
> > > > Am I missing something?
> > > Ah yes, it does seem like it is ignored. I assumed it would work for GCC since it handles, e.g. the format attribute on function pointers.
> > > 
> > > However, I would like it if we could support it on function pointers even if GCC just ignores is.
> > > However, I would like it if we could support it on function pointers even if GCC just ignores is.
> > 
> > We can, but the question is: under what spelling(s)? I think we can support this under `__attribute__((alloc_size(N)))` without issue. If GCC is looking to implement this same behavior, then I think we can also support it under `[[gnu::alloc_size(N)]]`.
> > 
> > If GCC doesn't have plans to add this functionality any time soon, we'd be stepping on their implementation namespace by implementing this functionality under the gnu vendor namespace and users would then be confused as to whose bug it is. In that case, I'd recommend we add `[[clang::alloc_size(N)]]` that behaves identically to `[[gnu::alloc_size(N)]]' except that it can be applied to function pointer types, and not change the behavior of `[[gnu::alloc_size(N)]]`. This can be implemented as a single semantic attribute by looking at the semantic spelling of the attribute and issuing a compatibility warning if you see `[[gnu::alloc_size(N)]]` being applied to a function pointer type, and can even recommend a fix-it to switch to `[[clang::alloc_size(N)]]` in that case. (See uses of `getSemanticSpelling()` elsewhere in the project for examples of how to test which spelling a given sematic attribute was spelled with.)
> It seems like GCC will also implement this behaviour so using `[[gnu::alloc_size()]]` (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88372#c1) should be fine.
Fantastic, thank you for coordinating this! I think using the `GCC` spelling is the right way to go then.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55212