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

Fri Jan 14 09:03:37 PST 2022

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a subscriber: erichkeane.
aaron.ballman added a comment.

In D114235#3243429 <https://reviews.llvm.org/D114235#3243429>, @mboehme wrote:

> Thanks for the feedback! And no worries about the delay -- I know you've got a lot on your plate, and the proposed change is invasive.
>
> To make sure I understand correctly: The issue is that if a `Type` is replaced by an `AttributedType` in places where Clang does not (yet) expect this to happen, this can cause performance regressions or assertions?

More that an attributed type generally carries extra information that needs to be stored somewhere (like, a calling convention type attribute needs to store which calling convention is represented by the attributed type) and that extra overhead can cause performance regressions. Additionally, depending on the kind of type attribute the plugin wants to create, there may be failed assertions without updating all of the places in clang that expect to handle a class type attribute (such as a plugin attempting to add a new calling convention attribute). And on top of that, there are very likely places where the compiler is inspecting the type via `isa<>` (etc) and that's not going to look through the attributed type because one isn't expected yet (so you may not get crashes, but obtuse compile errors about mismatched types).

> The motivation behind making type attributes pluggable is that I'd like to annotate pointers with additional information for use in a memory safety static analysis. The goal here is the same as for the proposal I discussed with you a while ago of extending the `annotate` attribute to types (or possibly adding a new `annotate_type` attribute) on this lengthy mailing list thread (no need to reread it):
>
> https://lists.llvm.org/pipermail/cfe-dev/2021-October/thread.html#69087
>
> In this discussion, I mentioned that I was thinking about making type attributes pluggable too. I eventually realized that this might actually be an easier avenue to pursue than extending `annotate` to types. (As we discussed, there might be cases where the GNU spelling of the `annotate` attribute would associate with a declaration while the C++ spelling would associate with a type, and we hadn't reached a firm conclusion on how best to resolve this. An entirely new pluggable attribute wouldn't have this problem.)
>
> From your feedback, it sounds as if I should return to my earlier idea of extending `annotate` to types. I wonder though: Wouldn't this suffer from the same problems that you raise above since, again, Clang would see `AttributedType`s in places where it might not expect them?

An `AttributedType` for an annotation attribute would require a new type in the type system, 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. There's still the concern about memory overhead, but that should only be paid by people who use the annotated type and hopefully isn't too much of a problem.

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

Adding @erichkeane as he's also thought a fair amount about attributes before, just in case I'm forgetting anything.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114235