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

Wed Feb 23 07:03:28 PST 2022

Sorry for the incomplete comment -- I pressed "send" too soon. I've edited
my comment on Phabricator to add the rest of my reply. (Pointing this out
here for people who are reading this in email.)

On Wed, 23 Feb 2022 at 15:53, Martin Böhme via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>

-- 

Martin *Brænne*

Software Engineer

mboehme at google.com

Google Germany GmbH
Erika-Mann-Straße 33
80363 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und
löschen Sie die E-Mail und alle Anhänge. Vielen Dank.

This e-mail is confidential. If you are not the right addressee please do
not forward it, please inform the sender, and please erase this e-mail
including any attachments. Thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220223/f9797e49/attachment.html>