[PATCH] D122673: Add kcfi_unchecked attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D122673#3414650 <https://reviews.llvm.org/D122673#3414650>, @samitolvanen wrote:

> Note that this was split from D119296 <https://reviews.llvm.org/D119296>.
>
> In the previous discussion, @joaomoreira  pointed out that this is very similar to `nocf_check` and proposed reusing that attribute. In an offline discussion, @pcc  was concerned that an attribute may not be the right approach here and suggested  a `__builtin_kcfi_unchecked(function(args))` built-in function to avoid changing the type system.
>
> I would appreciate hearing your thoughts.

I tend to be very wary of modifying the type system with attributes -- questions always arise of what the type system effects are of the attribute. e.g., does it impact overload sets or template specialization? Name mangling? If it doesn't have type system impacts... why does it need to be in the type system at all? This also matters because adding more bits to types can have unintended side effects like accidentally reducing the depth of template instantiations we can process (because of the extra memory pressure). So while I'm not certain what you and @pcc  talked about, it does seem like an approach at least worth thinking about, especially because this patch needs to bump the size of `Type`.



================
Comment at: clang/include/clang/AST/Type.h:1832
                                canon.isNull() ? QualType(this_(), 0) : canon) {
-    static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase),
+    static_assert(sizeof(*this) <= 16 + sizeof(ExtQualsTypeCommonBase),
                   "changing bitfields changed sizeof(Type)!");
----------------
Needing to bump this limit worries me. It's correct for the changes, but it means that `Type` just because even more expensive and so I wonder how this will impact compile time performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122673