[PATCH] D114235: [clang] Extend ParsedAttr to allow custom handling for type attributes

Wed Feb 23 06:53:44 PST 2022

mboehme added a comment.

Sorry for the delay -- finally back from leave.

High-level, I think I'd like to return to my earlier approach <https://lists.llvm.org/pipermail/cfe-dev/2021-October/thread.html#69087> of adding a new `annotate_type` attribute (see also discussion above).

In D114235#3244054 <https://reviews.llvm.org/D114235#3244054>, @aaron.ballman wrote:

> An `AttributedType` for an annotation attribute would require a new type in the type system,

Not sure I understand what you mean here?

FWIW, the intent would be to consider types with different `annotate_type` attributes to be the same type (and also the same as the un-attributed type). For example, it would not be possible for two overloads to differ merely by `annotate_type` attributes on their parameters.

Or are you saying this would require us to add a new subclass of `Type` to the Clang implementation -- e.g. `AnnotatedType`?

> so there could likely be places that need updating to look "through" the attributed type. But unlike with arbitrary plugins, the number of places is hopefully more reasonable.

IIUC, this already happens to some extent for other type attributes -- correct? For example, the nullability attributes (`_Nonnull` and the like). However, these can only be applied to pointer types (e.g. `int * _Nonnull ptr`), while an `annotate_type` attribute could also be applied to other types (e.g. `int [[clang::annotate_type("foo")]] i`). Is this an example of the places you're thinking of that would need to be touched?

>> If the same concerns apply to both of these approaches, do you have any suggestions for alternative ways that I could add annotations to types? Or does this mean that I would have to do the work of making sure that Clang can handle `AttributedType`s in these new places after all?
>
> No, I think you basically have to muck about with the type system no matter what. I love the idea of plugin type attributes in theory, but I don't think we're architecturally in a place where we can do that very easily. I suspect a dedicated attribute may be easier, but that's largely a guess. Both approaches strike me as likely to have a long tail of bugs we'll have to fight through.

What would be your recommendation for the best way to approach this? I.e. after I've implemented an `annotate_type` attribute (I already have a draft patch at https://reviews.llvm.org/D111548), how would I best test it for possible issues? I would obviously try to make my tests as comprehensive as possible -- but is there anything else I could do? There obviously isn't any existing code that uses this attribute -- so should I try to locally patch the compiler in some way where it would add the attribute to as many places in the AST as possible, in an attempt to break things?

In D114235#3244088 <https://reviews.llvm.org/D114235#3244088>, @erichkeane wrote:

> Right, yeah, so there are a couple of problems with AttributedType.  First, it gets lost almost as soon as you get out of SemaType about 90% of the time.  Anything that does some level of canonicalization ends up losing it, so the AttributedType information is lost almost immediately.  This is why the current ones all store information in the ExtInfo.  The worst place for this ends up being in the template instantiator, which immediately canonicalizes/desugars types all over the place.
>
> However, making AttributedType 'survive' is actually going to be troublesome as well. A lot of the type-checking compares types using == on their pointer values, so that would be broken if they are an AttributedType.
>
> So I think the 'first' thing that needs to happen is to make the entire CFE 'AttributedType' maintaining, AND tolerant. I can't think of a good way to do that reliably (my naive thought would be to come up with some way to temporarily (during development) wrap EVERY type in an AttributedType with a random attribute (so that they are all unique!) and do comparisons that way. Additionally, you'd need SOMETHING to validate that the AttributedTypes are the only one that survives (again, during development) to make sure it 'works right'.
>
> Additionally, you'll likely  have a lot of work to do in the template engine to make sure that the attributes are inherited correctly through  instantiations, specializations, etc.

Thanks for pointing out those issues!

I think, if possible, I'd like to avoid having to make `AttributedType

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114235