aaron.ballman added a comment.

In D110485#3308878 <https://reviews.llvm.org/D110485#3308878>, @mstorsjo wrote:

> In D110485#3308854 <https://reviews.llvm.org/D110485#3308854>, @mstorsjo 
> wrote:
>
>> To pick up the thread here again, `[[no_unique_address]]` is done and 
>> settled in MSVC, with the slightly surprising semantics: 
>> `[[no_unique_address]]` is accepted, without any warning (in C++20 mode), 
>> but it has no effect. (This, not related to LLVM, but because they had 
>> shipped it in earlier versions without having an effect, and changing that 
>> later would break things.) `[[msvc::no_unique_address]]` does have an effect 
>> though. See 
>> https://github.com/microsoft/STL/issues/1364#issuecomment-1034167093 for a 
>> more authoritative source on that.
>>
>> So, separately from implementing `[[msvc::no_unique_address]]`, I think we 
>> also should also silence the current warning about unknown attribute for the 
>> standard `[[no_unique_address]]`, to match MSVC.
>
> Oh, also, according to 
> https://devblogs.microsoft.com/cppblog/msvc-cpp20-and-the-std-cpp20-switch/, 
> the plan is to change `[[no_unique_address]]` to actually have an effect the 
> next time the compiler breaks its C++ ABI at an unknown point in the future. 
> (This shouldn't be an issue for Clang, as we'd have to make a conscious 
> effort to implement the new ABI whenever that happens anyway.)

Thanks for the update! I'll have to think about the appropriate way forward 
here a bit more. We have a rule with attributes to issue an "attribute ignored" 
warning for any attribute that isn't accepted by Clang, and we typically extend 
that to attributes which are accepted by Clang but have no impact in a way the 
user may be surprised by (e.g., if they write it on the wrong construct, we 
don't silently eat the attribute, we warn the user that the attribute is being 
ignored). So my initial thinking is, have an on-by-default diagnostic (grouped 
separately under the ignored attributes warning flag) about a standard 
attribute being accepted and purposefully ignored that's only used for 
`[[no_unique_address]]` in MS compatibility mode (for now, we may get more such 
attributes in the future). Then users get a different warning than they do 
today, and they can control it separately from `-Wignored-attributes`. I think 
still having a diagnostic is useful for some class of users (but not all). 
People who only care about compatibility with MSVC likely want the warning off 
by default, while people who care about compatibility outside of MSVC likely 
want the warning on by default. Given the security implications of ignoring the 
attribute, I think erring on the side of caution and warning by default is 
appropriate (this is a standard attribute, not a vendor one, so users are 
understandably going to expect the attribute to do something if they've used it 
correctly according to the standard; silencing it is a minor burden that will 
not impact other kinds of ignored attributes). The fact that MS intends to 
support the attribute in the future (which breaks ABI) is all the more reason 
why I think we should warn by default; Clang users shouldn't be caught off 
guard when MSVC eventually breaks their ABI.


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

https://reviews.llvm.org/D110485

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

Reply via email to