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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to