Support [[no_unique_address]] for all targets.

[expnkx]

Summary

I know msvc does not support [[no_unique_address]], but it supports [[msvc::no_unique_address]]. There is no attribute that matches this behavior for msvc, which leads to silent abi break.

clang 14 is probably going to release when the MSVC breaks abi. Yes we can support msvc::no_unique_address too, but it adds headache for future maintanence.

Also, x86_64-windows-gnu target and GCC targeting windows does support [[no_unique_address]] too, the abi is already "broken" in some form.

I propose to just support [[no_unique_address]] for all targets to prevent future problems.

[expnkx]

remove test for msvc

[aaron.ballman]

I've not given this enough thought, but I wanted to add some more reviewers for greater visibility.

Meanwhile, the premerge CI failures look related, so it looks like there are more tests to update.

[expnkx]

do not know why it cannot be built. upload and then verify again

[expnkx]

ninja: error: unknown target 'check-libc', did you mean 'check-lit'?

what's wrong with this?

[expnkx]

I just find that Preprocessor test has a windows (msvc not gnu) specific test that tests no_unique_address.

add that to patch and see whether it is mergable.

[rjmccall]

We should not add a standard feature with ABI impact if we aren't certain we're going to match MSVC's ABI on it. I'm sorry, but this needs to wait until there's an MSVC release that officially supports this.

[expnkx]

itanium + microsoft test both

[expnkx]

In D110485#3026533, @rjmccall wrote:

We should not add a standard feature with ABI impact if we aren't certain we're going to match MSVC's ABI on it. I'm sorry, but this needs to wait until there's an MSVC release that officially supports this.

but how are you going to deal with [[msvc::no_unique_address]] that works?

[expnkx]

In D110485#3026533, @rjmccall wrote:

We should not add a standard feature with ABI impact if we aren't certain we're going to match MSVC's ABI on it. I'm sorry, but this needs to wait until there's an MSVC release that officially supports this.

You are not actually matching msvc abi's layout because msvc supports [[msvc::no_unique_address]]. That is an MSVC release that officially supports.

https://github.com/tearosccebe/fast_io/blob/fa355154463704cde13cc9917e315e73281b03e9/include/fast_io_freestanding_impl/io_buffer/main.h#L160

Only clang --target=x86_64-windows-msvc does not support no_unique_address. If they are using my library they already get a break since clang does not support [[msvc::no_unique_address]] and it does not support [[no_unique_address]] either.

If you really have concerns about it, has_cpp_attribute(no_unique_address) always works.
has_cpp_attribute(no_unique_address)

Also what happens if I am using --target=x86_64-windows-gnu (that is actually my default target on windows since I use Canadian compiler). You still get abi break.

Just break ABI. This is a good start to fix the language.

[expnkx]

should be diff3

[ivan171]

In D110485#3026533, @rjmccall wrote:

We should not add a standard feature with ABI impact if we aren't certain we're going to match MSVC's ABI on it. I'm sorry, but this needs to wait until there's an MSVC release that officially supports this.

This issue on STL's repo suggests that they're waiting Clang get support for no_unique_address on Windows to start using it. Stephan said here MSVC has implemented this, but with a different name.

[rjmccall]

MSVC gets to chose the ABI rules for their platform. It is not Clang policy to pick ABI rules and then break them later.

If you'd like to contribute an implementation of [[msvc::no_unique_address]] that matches MSVC's ABI, that would be welcome. I don't know, maybe that's as simple as making it use the existing implementation; @zygoloid might know better. But "add an ABI feature ahead of the platform owner and then fix the ABI problems later" is not how we do things.

[expnkx]

I cannot find out where to add attribute that starts with msvc::xxxxx.

I am just applying this "potential" patch to the clang upstream. In the future we can just remove TargetItaniumCXXABI attr in the clang table gen file without worrying about too much. So it is a "noop".

So this patch changes nothing but it makes us be convenient to fix it in the future.

[expnkx]

it should first update the field and then test no_unique_address

[expnkx]

fixing clang format

[expnkx]

clang format + clang tidy

all clang tests pass locally.

flang is bugged. Do not know why flang cannot find llvm-jxx

[aaron.ballman]

In D110485#3026629, @rjmccall wrote:

MSVC gets to chose the ABI rules for their platform. It is not Clang policy to pick ABI rules and then break them later.

If you'd like to contribute an implementation of [[msvc::no_unique_address]] that matches MSVC's ABI, that would be welcome. I don't know, maybe that's as simple as making it use the existing implementation; @zygoloid might know better. But "add an ABI feature ahead of the platform owner and then fix the ABI problems later" is not how we do things.

I concur with John.

In D110485#3028553, @expnkx wrote:

I cannot find out where to add attribute that starts with msvc::xxxxx.

To Attr.td; you'd give a new spelling, like: CXX11<"msvc", "no_unique_address">

I am just applying this "potential" patch to the clang upstream. In the future we can just remove TargetItaniumCXXABI attr in the clang table gen file without worrying about too much. So it is a "noop".

So this patch changes nothing but it makes us be convenient to fix it in the future.

This patch is changing behavior and cannot be landed as-is. I don't think we should do anything in this area unless it's an implementation of [[msvc::no_unique_address]] that is ABI compatible with the Microsoft implementation.

[erichkeane]

clang/include/clang/Basic/Attr.td

1690 ↗

(On Diff #375775)

The way to do a [[msvc::no_unique_address]] is likely to create a new attribute here in Attr.td, then make the spelling be something like:

let Spellings = [CXX11<"msvc", "no_unique_address", N>];

Where N is a number that is the value of __has_attribute (which I note that MSVC has yet to implement?).

[expnkx]

clang simply just does not truly support everything msvc supports. There are no things that deals [[msvc::]] attributes at all in the clang. In fact the point of using clang on windows is that we do not want to do the same as Microsoft does.

Support [[no_unique_address]] is nothing wrong tbh and that is what ISO C++ standard requires.

If we do not change, the Microsoft team won't wanna change either because they are afraid of breaking clang too.

I never understand why [[no_unique_address]] is a problem.

[erichkeane]

In D110485#3030853, @expnkx wrote:

clang simply just does not truly support everything msvc supports. There are no things that deals [[msvc::]] attributes at all in the clang. In fact the point of using clang on windows is that we do not want to do the same as Microsoft does.

Support [[no_unique_address]] is nothing wrong tbh and that is what ISO C++ standard requires.

If we do not change, the Microsoft team won't wanna change either because they are afraid of breaking clang too.

I never understand why [[no_unique_address]] is a problem.

A requirement of Clang on Windows is that we are ABI compatible (so that we can link between them!), the same way that Linux must match the Itanium ABI. Allowing no_unique_address on Windows is an ABI compatibility issue. In this case, we have to pay attention to what the platform's ABI OWNERS define as the ABI, which in this case is Microsoft.

THAT is why this is a problem, and THAT is what is wrong with this change.

If Microsoft defines an ABI for what this does, we can follow them and just do that.

[expnkx]

In D110485#3030874, @erichkeane wrote:

In D110485#3030853, @expnkx wrote:

clang simply just does not truly support everything msvc supports. There are no things that deals [[msvc::]] attributes at all in the clang. In fact the point of using clang on windows is that we do not want to do the same as Microsoft does.

Support [[no_unique_address]] is nothing wrong tbh and that is what ISO C++ standard requires.

If we do not change, the Microsoft team won't wanna change either because they are afraid of breaking clang too.

I never understand why [[no_unique_address]] is a problem.

A requirement of Clang on Windows is that we are ABI compatible (so that we can link between them!), the same way that Linux must match the Itanium ABI. Allowing no_unique_address on Windows is an ABI compatibility issue. In this case, we have to pay attention to what the platform's ABI OWNERS define as the ABI, which in this case is Microsoft.

THAT is why this is a problem, and THAT is what is wrong with this change.

If Microsoft defines an ABI for what this does, we can follow them and just do that.

https://github.com/microsoft/STL/issues/1364

As Microsoft said, the reason why things like msvc::no_unique_address exist are because clang does not support it. In fact, it has become a vicious cycle. msvc waits clang, clang waits msvc.

It is a vicious cycle. I would argue it is LLVM's fault, not Microsoft's fault.

Sure you can add another attribute, i tried that, but adding another attribute for doing exactly the same thing through entire code base is just annoying and headache.

If clang does not support [[no_unique_address]], Microsoft is not going to support it either. Microsoft's own words.

[[no_unique_address]] is ISO C++ standard. Why not support it? I am sorry the entire LLVM did a crappy job on support C++20 standard in general.

[erichkeane]

In D110485#3033889, @expnkx wrote:

In D110485#3030874, @erichkeane wrote:

In D110485#3030853, @expnkx wrote:

clang simply just does not truly support everything msvc supports. There are no things that deals [[msvc::]] attributes at all in the clang. In fact the point of using clang on windows is that we do not want to do the same as Microsoft does.

Support [[no_unique_address]] is nothing wrong tbh and that is what ISO C++ standard requires.

If we do not change, the Microsoft team won't wanna change either because they are afraid of breaking clang too.

I never understand why [[no_unique_address]] is a problem.

A requirement of Clang on Windows is that we are ABI compatible (so that we can link between them!), the same way that Linux must match the Itanium ABI. Allowing no_unique_address on Windows is an ABI compatibility issue. In this case, we have to pay attention to what the platform's ABI OWNERS define as the ABI, which in this case is Microsoft.

THAT is why this is a problem, and THAT is what is wrong with this change.

If Microsoft defines an ABI for what this does, we can follow them and just do that.

https://github.com/microsoft/STL/issues/1364

As Microsoft said, the reason why things like msvc::no_unique_address exist are because clang does not support it. In fact, it has become a vicious cycle. msvc waits clang, clang waits msvc.

It is a vicious cycle. I would argue it is LLVM's fault, not Microsoft's fault.

Sure you can add another attribute, i tried that, but adding another attribute for doing exactly the same thing through entire code base is just annoying and headache.

If clang does not support [[no_unique_address]], Microsoft is not going to support it either. Microsoft's own words.

[[no_unique_address]] is ISO C++ standard. Why not support it? I am sorry the entire LLVM did a crappy job on support C++20 standard in general.

My understanding is Microsoft DOES have [[no_unique_address]], it just doesn't do anything, right? So it seems they have decided the ABI is 'pretend this attribute doesn't exist'. Implementing this as anything else is an ABI break, which we don't do.

[aaron.ballman]

In D110485#3033889, @expnkx wrote:

https://github.com/microsoft/STL/issues/1364

As Microsoft said, the reason why things like msvc::no_unique_address exist are because clang does not support it. In fact, it has become a vicious cycle. msvc waits clang, clang waits msvc.

It is a vicious cycle. I would argue it is LLVM's fault, not Microsoft's fault.

I don't know why anyone would assume that Clang would be more responsible for defining the behavior of a feature on Windows than Microsoft. Microsoft is the owner for their ABI, not Clang. When this feature was introduced into Clang, we explicitly did not allow it on Windows until Microsoft defined what the ABI is for the feature. Now that Microsoft has decided to define [[msvc::no_unique_address]] with a particular ABI, it would be reasonable for us to support *that* spelling. However, it would be unacceptable for us to define the behavior for [[no_unique_address]] in a way that's different from the ABI Microsoft picks for it.

Note: it *might* be reasonable for us to define [[no_unique_address]] as being ignored on Windows targets as that seems to be the direction Microsoft wants to go. However, I personally view the Microsoft behavior here to be user-hostile and so I'd like to see whether they're planning to change directions before we decide to follow along. However, if Microsoft's position is that they're not going to implement the semantic effects for [[no_unique_address]], Clang will follow suit on the Windows target.

Sure you can add another attribute, i tried that, but adding another attribute for doing exactly the same thing through entire code base is just annoying and headache.

If clang does not support [[no_unique_address]], Microsoft is not going to support it either. Microsoft's own words.

And Microsoft's own choice.

[[no_unique_address]] is ISO C++ standard. Why not support it? I am sorry the entire LLVM did a crappy job on support C++20 standard in general.

In keeping with our code of conduct (https://llvm.org/docs/CodeOfConduct.html), please keep your comments respectful.

[mstorsjo]

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.

[mstorsjo]

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

[aaron.ballman]

In D110485#3308878, @mstorsjo wrote:

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

clang/lib/AST/RecordLayoutBuilder.cpp

   ElementInfo Info = getAdjustedElementInfo(FD);
   Alignment = std::max(Alignment, Info.Alignment);
   CharUnits FieldOffset;
+  auto *FieldClass = FD->getType()->getAsCXXRecordDecl();
+  bool PotentiallyOverlapping =
+      FD->hasAttr<NoUniqueAddressAttr>() && FieldClass;
+  bool IsOverlappingEmptyField =
+      PotentiallyOverlapping && FieldClass->isEmpty();
   if (UseExternalLayout)
     FieldOffset =
         Context.toCharUnitsFromBits(External.getExternalFieldOffset(FD));
-  else if (IsUnion)
+  else if (IsUnion || IsOverlappingEmptyField)
+  {
     FieldOffset = CharUnits::Zero();
+  }
   else
     FieldOffset = Size.alignTo(Info.Alignment);
   placeFieldAtOffset(FieldOffset);