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

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/include/clang/Basic/Attr.td

   let Documentation = [ArmMveStrictPolymorphismDocs];
 }
 
-def NoUniqueAddress : InheritableAttr, TargetSpecificAttr<TargetItaniumCXXABI> {
+def NoUniqueAddress : InheritableAttr {
   let Spellings = [CXX11<"", "no_unique_address", 201803>];
   let Subjects = SubjectList<[NonBitField], ErrorDiag>;
   let Documentation = [NoUniqueAddressDocs];

clang/lib/AST/RecordLayoutBuilder.cpp

 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
-#include "clang/AST/VTableBuilder.h"
 #include "clang/AST/RecordLayout.h"
+#include "clang/AST/VTableBuilder.h"
 #include "clang/Basic/TargetInfo.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/Format.h"

   bool IsVirtual;
 
   /// Bases - Information about the base subobjects.
-  SmallVector<BaseSubobjectInfo*, 4> Bases;
+  SmallVector<BaseSubobjectInfo *, 4> Bases;
 
   /// PrimaryVirtualBaseInfo - Holds the base info for the primary virtual base
   /// of this base info (if one exists).

   /// Get the offset of the given field. The external source must provide
   /// entries for all fields in the record.
   uint64_t getExternalFieldOffset(const FieldDecl *FD) {
-    assert(FieldOffsets.count(FD) &&
-           "Field does not have an external offset");
+    assert(FieldOffsets.count(FD) && "Field does not have an external offset");
     return FieldOffsets[FD];
   }
 

     return Offset <= MaxEmptyClassOffset;
   }
 
-  CharUnits
-  getFieldOffset(const ASTRecordLayout &Layout, unsigned FieldNo) const {
+  CharUnits getFieldOffset(const ASTRecordLayout &Layout,
+                           unsigned FieldNo) const {
     uint64_t FieldOffset = Layout.getFieldOffset(FieldNo);
     assert(FieldOffset % CharWidth == 0 &&
            "Field offset not at char boundary!");

   CharUnits SizeOfLargestEmptySubobject;
 
   EmptySubobjectMap(const ASTContext &Context, const CXXRecordDecl *Class)
       : Context(Context), CharWidth(Context.getCharWidth()), Class(Class) {
-      ComputeEmptySubobjectSizes();
   }
 
   /// CanPlaceBaseAtOffset - Return whether the given base class can be placed
   /// at the given offset.
   /// Returns false if placing the record will result in two components
   /// (direct or indirect) of the same type having the same offset.
-  bool CanPlaceBaseAtOffset(const BaseSubobjectInfo *Info,
-                            CharUnits Offset);
+  bool CanPlaceBaseAtOffset(const BaseSubobjectInfo *Info, CharUnits Offset);
 
   /// CanPlaceFieldAtOffset - Return whether a field can be placed at the given
   /// offset.

   }
 }
 
-bool
-EmptySubobjectMap::CanPlaceSubobjectAtOffset(const CXXRecordDecl *RD,
-                                             CharUnits Offset) const {
+bool EmptySubobjectMap::CanPlaceSubobjectAtOffset(const CXXRecordDecl *RD,
+                                                  CharUnits Offset) const {
   // We only need to check empty bases.
   if (!RD->isEmpty())
     return true;

     MaxEmptyClassOffset = Offset;
 }
 
-bool
-EmptySubobjectMap::CanPlaceBaseSubobjectAtOffset(const BaseSubobjectInfo *Info,
-                                                 CharUnits Offset) {
+bool EmptySubobjectMap::CanPlaceBaseSubobjectAtOffset(
+    const BaseSubobjectInfo *Info, CharUnits Offset) {
   // We don't have to keep looking past the maximum offset that's known to
   // contain an empty class.
   if (!AnyEmptySubobjectsBeyondOffset(Offset))

   // Traverse all member variables.
   unsigned FieldNo = 0;
   for (CXXRecordDecl::field_iterator I = Info->Class->field_begin(),
-       E = Info->Class->field_end(); I != E; ++I, ++FieldNo) {
+                                     E = Info->Class->field_end();
+       I != E; ++I, ++FieldNo) {
     if (I->isBitField())
       continue;
 

   // Traverse all member variables.
   unsigned FieldNo = 0;
   for (CXXRecordDecl::field_iterator I = Info->Class->field_begin(),
-       E = Info->Class->field_end(); I != E; ++I, ++FieldNo) {
+                                     E = Info->Class->field_end();
+       I != E; ++I, ++FieldNo) {
     if (I->isBitField())
       continue;
 

   return true;
 }
 
-bool
-EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const CXXRecordDecl *RD,
-                                                  const CXXRecordDecl *Class,
-                                                  CharUnits Offset) const {
+bool EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(
+    const CXXRecordDecl *RD, const CXXRecordDecl *Class,
+    CharUnits Offset) const {
   // We don't have to keep looking past the maximum offset that's known to
   // contain an empty class.
   if (!AnyEmptySubobjectsBeyondOffset(Offset))

   return true;
 }
 
-bool
-EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const FieldDecl *FD,
-                                                  CharUnits Offset) const {
+bool EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const FieldDecl *FD,
+                                                       CharUnits Offset) const {
   // We don't have to keep looking past the maximum offset that's known to
   // contain an empty class.
   if (!AnyEmptySubobjectsBeyondOffset(Offset))

   return true;
 }
 
-bool
-EmptySubobjectMap::CanPlaceFieldAtOffset(const FieldDecl *FD,
-                                         CharUnits Offset) {
+bool EmptySubobjectMap::CanPlaceFieldAtOffset(const FieldDecl *FD,
+                                              CharUnits Offset) {
   if (!CanPlaceFieldSubobjectAtOffset(FD, Offset))
     return false;
 

   }
 }
 
-typedef llvm::SmallPtrSet<const CXXRecordDecl*, 4> ClassSetTy;
+typedef llvm::SmallPtrSet<const CXXRecordDecl *, 4> ClassSetTy;
 
 class ItaniumRecordLayoutBuilder {
 protected:

                           bool FieldPacked, const FieldDecl *D);
   void LayoutBitField(const FieldDecl *D);
 
-  TargetCXXABI getCXXABI() const {
-    return Context.getTargetInfo().getCXXABI();
-  }
+  TargetCXXABI getCXXABI() const { return Context.getTargetInfo().getCXXABI(); }
 
   /// BaseSubobjectInfoAllocator - Allocator for BaseSubobjectInfo objects.
   llvm::SpecificBumpPtrAllocator<BaseSubobjectInfo> BaseSubobjectInfoAllocator;
 
   typedef llvm::DenseMap<const CXXRecordDecl *, BaseSubobjectInfo *>
       BaseSubobjectInfoMapTy;
 
   /// VirtualBaseInfo - Map from all the (direct or indirect) virtual bases
   /// of the class we're laying out to their base subobject info.

                                      uint64_t ComputedOffset);
 
   void CheckFieldPadding(uint64_t Offset, uint64_t UnpaddedOffset,
-                          uint64_t UnpackedOffset, unsigned UnpackedAlign,
-                          bool isPacked, const FieldDecl *D);
 
   DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID);
 

     // Traversing the bases must have created the base info for our primary
     // virtual base.
     PrimaryVirtualBaseInfo = VirtualBaseInfo.lookup(PrimaryVirtualBase);
-    assert(PrimaryVirtualBaseInfo &&
-           "Did not create a primary virtual base!");
+    assert(PrimaryVirtualBaseInfo && "Did not create a primary virtual base!");
 
     // Claim the primary virtual base as our primary virtual base.
     Info->PrimaryVirtualBaseInfo = PrimaryVirtualBaseInfo;

     const CXXRecordDecl *BaseDecl = I.getType()->getAsCXXRecordDecl();
 
     // Compute the base subobject info for this base.
-    BaseSubobjectInfo *Info = ComputeBaseSubobjectInfo(BaseDecl, IsVirtual,
-                                                       nullptr);
+    BaseSubobjectInfo *Info =
+        ComputeBaseSubobjectInfo(BaseDecl, IsVirtual, nullptr);
 
     if (IsVirtual) {
       // ComputeBaseInfo has already added this base for us.
-      assert(VirtualBaseInfo.count(BaseDecl) &&
-             "Did not add virtual base!");
+      assert(VirtualBaseInfo.count(BaseDecl) && "Did not add virtual base!");
     } else {
       // Add the base info to the map of non-virtual bases.
       assert(!NonVirtualBaseInfo.count(BaseDecl) &&

       LayoutVirtualBase(PrimaryBaseInfo);
     } else {
       BaseSubobjectInfo *PrimaryBaseInfo =
           NonVirtualBaseInfo.lookup(PrimaryBase);
       assert(PrimaryBaseInfo &&
              "Did not find base info for non-virtual primary base!");
 
       LayoutNonVirtualBase(PrimaryBaseInfo);
     }
 
     // If this class needs a vtable/vf-table and didn't get one from a
     // primary base, add it in now.
   } else if (RD->isDynamicClass()) {
     assert(DataSize == 0 && "Vtable pointer must be at offset zero!");
     CharUnits PtrWidth =
         Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerWidth(0));
     CharUnits PtrAlign =
         Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerAlign(0));
     EnsureVTablePointerAlignment(PtrAlign);
     HasOwnVFPtr = true;
 

 
   // Add its base class offset.
   assert(!VBases.count(Base->Class) && "vbase offset already exists!");
-  VBases.insert(std::make_pair(Base->Class,
-                       ASTRecordLayout::VBaseInfo(Offset, false)));
+  VBases.insert(
+      std::make_pair(Base->Class, ASTRecordLayout::VBaseInfo(Offset, false)));
 
   AddPrimaryVirtualBaseOffsets(Base, Offset);
 }

 }
 
 // Rounds the specified size to have it a multiple of the char size.
-static uint64_t
-roundUpSizeToCharAlignment(uint64_t Size,
-                           const ASTContext &Context) {
+static uint64_t roundUpSizeToCharAlignment(uint64_t Size,
+                                           const ASTContext &Context) {
   uint64_t CharAlignment = Context.getTargetInfo().getCharAlign();
   return llvm::alignTo(Size, CharAlignment);
 }

   //   sizeof(T')*8 <= n.
 
   QualType IntegralPODTypes[] = {
       Context.UnsignedCharTy, Context.UnsignedShortTy, Context.UnsignedIntTy,
-    Context.UnsignedLongTy, Context.UnsignedLongLongTy
-  };
+      Context.UnsignedLongTy, Context.UnsignedLongLongTy};
 
   QualType Type;
   for (const QualType &QT : IntegralPODTypes) {

   uint64_t UnpaddedFieldOffset = getDataSizeInBits() - UnfilledBitsInLastUnit;
 
   if (IsUnion) {
-    uint64_t RoundedFieldSize = roundUpSizeToCharAlignment(FieldSize,
-                                                           Context);
+    uint64_t RoundedFieldSize = roundUpSizeToCharAlignment(FieldSize, Context);
     setDataSize(std::max(getDataSizeInBits(), RoundedFieldSize));
     FieldOffset = 0;
   } else {

 
   // Compute the next available bit offset.
   uint64_t FieldOffset =
       IsUnion ? 0 : (getDataSizeInBits() - UnfilledBitsInLastUnit);
 
   // Handle targets that don't honor bitfield type alignment.
   if (!IsMsStruct && !Context.getTargetInfo().useBitFieldTypeAlignment()) {

             Context.getTargetInfo().getZeroLengthBitfieldBoundary();
         FieldAlign = std::max(FieldAlign, ZeroLengthBitfieldBoundary);
       }
       // If that doesn't apply, just ignore the field alignment.
     } else {
       FieldAlign = 1;
     }

     }
     setDataSize(std::max(getDataSizeInBits(), RoundedFieldSize));
 
     // For non-zero-width bitfields in ms_struct structs, allocate a new
     // storage unit if necessary.
   } else if (IsMsStruct && FieldSize) {
     // We should have cleared UnfilledBitsInLastUnit in every case
     // where we changed storage units.

       // array of zero-length, remains of Size 0
       if (RD->isEmpty())
         setSize(CharUnits::One());
-    }
-    else
+    } else
       setSize(CharUnits::One());
   }
 

         InBits = false;
       }
       Diag(RD->getLocation(), diag::warn_padded_struct_size)
-          << Context.getTypeDeclType(RD)
-          << PadSize
+          << Context.getTypeDeclType(RD) << PadSize
           << (InBits ? 1 : 0); // (byte|bit)
     }
 

 /// \returns diagnostic %select index.
 static unsigned getPaddingDiagFromTagKind(TagTypeKind Tag) {
   switch (Tag) {
-  case TTK_Struct: return 0;
-  case TTK_Interface: return 1;
-  case TTK_Class: return 2;
-  default: llvm_unreachable("Invalid tag kind for field padding diagnostic!");
+  case TTK_Struct:
+    return 0;
+  case TTK_Interface:
+    return 1;
+  case TTK_Class:
+    return 2;
+  default:
+    llvm_unreachable("Invalid tag kind for field padding diagnostic!");
   }
 }
 

     if (D->getIdentifier())
       Diag(D->getLocation(), diag::warn_padded_struct_field)
           << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
-          << Context.getTypeDeclType(D->getParent())
-          << PadSize
+          << Context.getTypeDeclType(D->getParent()) << PadSize
           << (InBits ? 1 : 0) // (byte|bit)
           << D->getIdentifier();
     else
       Diag(D->getLocation(), diag::warn_padded_struct_anon_field)
           << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
-          << Context.getTypeDeclType(D->getParent())
-          << PadSize
+          << Context.getTypeDeclType(D->getParent()) << PadSize
           << (InBits ? 1 : 0); // (byte|bit)
   }
   if (isPacked && Offset != UnpackedOffset) {
     HasPackedField = true;
   }
 }
 
 static const CXXMethodDecl *computeKeyFunction(ASTContext &Context,

     return nullptr;
 
   bool allowInlineFunctions =
       Context.getTargetInfo().getCXXABI().canKeyFunctionBeInline();
 
   for (const CXXMethodDecl *MD : RD->methods()) {
     if (!MD->isVirtual())

   };
   typedef llvm::DenseMap<const CXXRecordDecl *, CharUnits> BaseOffsetsMapTy;
   MicrosoftRecordLayoutBuilder(const ASTContext &Context) : Context(Context) {}
+
 private:
   MicrosoftRecordLayoutBuilder(const MicrosoftRecordLayoutBuilder &) = delete;
   void operator=(const MicrosoftRecordLayoutBuilder &) = delete;
+
 public:
   void layout(const RecordDecl *RD);
   void cxxLayout(const CXXRecordDecl *RD);

   // the alignment in the case of pragma pack.  Note that the required alignment
   // doesn't actually apply to the struct alignment at this point.
   Alignment = std::max(Alignment, Info.Alignment);
-  RequiredAlignment = std::max(RequiredAlignment, Layout.getRequiredAlignment());
+  RequiredAlignment =
+      std::max(RequiredAlignment, Layout.getRequiredAlignment());
   Info.Alignment = std::max(Info.Alignment, Layout.getRequiredAlignment());
   Info.Size = Layout.getNonVirtualSize();
   return Info;
 }
 
 MicrosoftRecordLayoutBuilder::ElementInfo
-MicrosoftRecordLayoutBuilder::getAdjustedElementInfo(
-    const FieldDecl *FD) {
+MicrosoftRecordLayoutBuilder::getAdjustedElementInfo(const FieldDecl *FD) {
   // Get the alignment of the field type's natural alignment, ignore any
   // alignment attributes.
   auto TInfo =

     // alignment when it is applied to bitfields.
     Info.Alignment = std::max(Info.Alignment, FieldRequiredAlignment);
   else {
-    if (auto RT =
+    if (auto const *RT =
             FD->getType()->getBaseElementTypeUnsafe()->getAs<RecordType>()) {
       auto const &Layout = Context.getASTRecordLayout(RT->getDecl());
       EndsWithZeroSizedObject = Layout.endsWithZeroSizedObject();
-      FieldRequiredAlignment = std::max(FieldRequiredAlignment,
-                                        Layout.getRequiredAlignment());
+      FieldRequiredAlignment =
+          std::max(FieldRequiredAlignment, Layout.getRequiredAlignment());
     }
     // Capture required alignment as a side-effect.
     RequiredAlignment = std::max(RequiredAlignment, FieldRequiredAlignment);

   MaxFieldAlignment = CharUnits::Zero();
   // Honor the default struct packing maximum alignment flag.
   if (unsigned DefaultMaxFieldAlignment = Context.getLangOpts().PackStruct)
-      MaxFieldAlignment = CharUnits::fromQuantity(DefaultMaxFieldAlignment);
   // Honor the packing attribute.  The MS-ABI ignores pragma pack if its larger
   // than the pointer size.
-  if (const MaxFieldAlignmentAttr *MFAA = RD->getAttr<MaxFieldAlignmentAttr>()){
+  if (const MaxFieldAlignmentAttr *MFAA =
+          RD->getAttr<MaxFieldAlignmentAttr>()) {
     unsigned PackedAlignment = MFAA->getAlignment();
     if (PackedAlignment <= Context.getTargetInfo().getPointerWidth(0))
       MaxFieldAlignment = Context.toCharUnitsFromBits(PackedAlignment);

         External.BaseOffsets, External.VirtualBaseOffsets);
 }
 
-void
-MicrosoftRecordLayoutBuilder::initializeCXXLayout(const CXXRecordDecl *RD) {
+void MicrosoftRecordLayoutBuilder::initializeCXXLayout(
+    const CXXRecordDecl *RD) {
   EndsWithZeroSizedObject = false;
   LeadsWithZeroSizedBase = false;
   HasOwnVFPtr = false;

     PointerInfo.Alignment = std::min(PointerInfo.Alignment, MaxFieldAlignment);
 }
 
-void
-MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
+void MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(
+    const CXXRecordDecl *RD) {
   // The MS-ABI lays out all bases that contain leading vfptrs before it lays
   // out any bases that do not contain vfptrs.  We implement this as two passes
   // over the bases.  This approach guarantees that the primary base is laid out

 }
 
 void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
-    const CXXRecordDecl *RD,
-    const CXXRecordDecl *BaseDecl,
+    const CXXRecordDecl *RD, const CXXRecordDecl *BaseDecl,
     const ASTRecordLayout &BaseLayout,
     const ASTRecordLayout *&PreviousBaseLayout) {
   // Insert padding between two bases if the left first one is zero sized or

   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
+  } else
     FieldOffset = Size.alignTo(Info.Alignment);
   placeFieldAtOffset(FieldOffset);
   Size = std::max(Size, FieldOffset + Info.Size);

   }
 }
 
-void
-MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(const FieldDecl *FD) {
+void MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(
+    const FieldDecl *FD) {
   // Zero-width bitfields are ignored unless they follow a non-zero-width
   // bitfield.
   if (!LastFieldIsNonZeroWidthBitfield) {

 
     assert(BaseOffset >= Size && "base offset already allocated");
 
-    VBases.insert(std::make_pair(BaseDecl,
-        ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp)));
+    VBases.insert(std::make_pair(
+        BaseDecl, ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp)));
     Size = BaseOffset + BaseLayout.getNonVirtualSize();
     PreviousBaseLayout = &BaseLayout;
   }

 
 // Recursively walks the non-virtual bases of a class and determines if any of
 // them are in the bases with overridden methods set.
-static bool
-RequiresVtordisp(const llvm::SmallPtrSetImpl<const CXXRecordDecl *> &
-                     BasesWithOverriddenMethods,
-                 const CXXRecordDecl *RD) {
+static bool RequiresVtordisp(const llvm::SmallPtrSetImpl<const CXXRecordDecl *>
+                                 &BasesWithOverriddenMethods,
+                             const CXXRecordDecl *RD) {
   if (BasesWithOverriddenMethods.count(RD))
     return true;
   // If any of a virtual bases non-virtual bases (recursively) requires a

   // until we *finish* parsing the definition.
 
   if (D->hasExternalLexicalStorage() && !D->getDefinition())
-    getExternalSource()->CompleteType(const_cast<RecordDecl*>(D));
+    getExternalSource()->CompleteType(const_cast<RecordDecl *>(D));
 
   D = D->getDefinition();
   assert(D && "Cannot get layout of forward declarations!");

   // Note that we can't save a reference to the entry because this function
   // is recursive.
   const ASTRecordLayout *Entry = ASTRecordLayouts[D];
-  if (Entry) return *Entry;
+  if (Entry)
+    return *Entry;
 
   const ASTRecordLayout *NewEntry = nullptr;
 

   return *NewEntry;
 }
 
-const CXXMethodDecl *ASTContext::getCurrentKeyFunction(const CXXRecordDecl *RD) {
+const CXXMethodDecl *
+ASTContext::getCurrentKeyFunction(const CXXRecordDecl *RD) {
   if (!getTargetInfo().getCXXABI().hasKeyFunctions())
     return nullptr;
 

 
   // Store it back if it changed.
   if (Entry.isOffset() || Entry.isValid() != bool(Result))
-    KeyFunctions[RD] = const_cast<Decl*>(Result);
+    KeyFunctions[RD] = const_cast<Decl *>(Result);
 
   return cast_or_null<CXXMethodDecl>(Result);
 }

   auto I = Map.find(Method->getParent());
 
   // If it's not cached, there's nothing to do.
-  if (I == Map.end()) return;
+  if (I == Map.end())
+    return;
 
   // If it is cached, check whether it's the target method, and if so,
   // remove it from the cache. Note, the call to 'get' might invalidate

   // directly.
   unsigned Index = 0;
 
-  for (const ObjCIvarDecl *IVD = Container->all_declared_ivar_begin();
-       IVD; IVD = IVD->getNextIvar()) {
+  for (const ObjCIvarDecl *IVD = Container->all_declared_ivar_begin(); IVD;
+       IVD = IVD->getNextIvar()) {
     if (Ivar == IVD)
       break;
     ++Index;

                           const ObjCImplementationDecl *Impl) const {
   // Retrieve the definition
   if (D->hasExternalLexicalStorage() && !D->getDefinition())
-    getExternalSource()->CompleteType(const_cast<ObjCInterfaceDecl*>(D));
+    getExternalSource()->CompleteType(const_cast<ObjCInterfaceDecl *>(D));
   D = D->getDefinition();
   assert(D && !D->isInvalidDecl() && D->isThisDeclarationADefinition() &&
          "Invalid interface decl!");
 
   // Look up this layout, if already laid out, return what we have.
   const ObjCContainerDecl *Key =
-    Impl ? (const ObjCContainerDecl*) Impl : (const ObjCContainerDecl*) D;
+      Impl ? (const ObjCContainerDecl *)Impl : (const ObjCContainerDecl *)D;
   if (const ASTRecordLayout *Entry = ObjCLayouts[Key])
     return *Entry;
 

   return *NewEntry;
 }
 
-static void PrintOffset(raw_ostream &OS,
-                        CharUnits Offset, unsigned IndentLevel) {
+static void PrintOffset(raw_ostream &OS, CharUnits Offset,
+                        unsigned IndentLevel) {
   OS << llvm::format("%10" PRId64 " | ", (int64_t)Offset.getQuantity());
   OS.indent(IndentLevel * 2);
 }

 }
 
 static void DumpRecordLayout(raw_ostream &OS, const RecordDecl *RD,
-                             const ASTContext &C,
-                             CharUnits Offset,
-                             unsigned IndentLevel,
-                             const char* Description,
-                             bool PrintSizeInfo,
-                             bool IncludeVirtualBases) {
+                             const ASTContext &C, CharUnits Offset,
+                             unsigned IndentLevel, const char *Description,
+                             bool PrintSizeInfo, bool IncludeVirtualBases) {
   const ASTRecordLayout &Layout = C.getASTRecordLayout(RD);
-  auto CXXRD = dyn_cast<CXXRecordDecl>(RD);
+  auto const *CXXRD = dyn_cast<CXXRecordDecl>(RD);
 
   PrintOffset(OS, Offset, IndentLevel);
-  OS << C.getTypeDeclType(const_cast<RecordDecl*>(RD)).getAsString();
+  OS << C.getTypeDeclType(const_cast<RecordDecl *>(RD)).getAsString();
   if (Description)
     OS << ' ' << Description;
   if (CXXRD && CXXRD->isEmpty())

 
   // Dump fields.
   uint64_t FieldNo = 0;
-  for (RecordDecl::field_iterator I = RD->field_begin(),
-         E = RD->field_end(); I != E; ++I, ++FieldNo) {
+  for (RecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();
+       I != E; ++I, ++FieldNo) {
     const FieldDecl &Field = **I;
     uint64_t LocalFieldOffsetInBits = Layout.getFieldOffset(FieldNo);
     CharUnits FieldOffset =
         Offset + C.toCharUnitsFromBits(LocalFieldOffsetInBits);
 
     // Recursively dump fields of record type.
-    if (auto RT = Field.getType()->getAs<RecordType>()) {
+    if (auto const *RT = Field.getType()->getAs<RecordType>()) {
       DumpRecordLayout(OS, RT->getDecl(), C, FieldOffset, IndentLevel,
                        Field.getName().data(),
                        /*PrintSizeInfo=*/false,

   // Dump virtual bases.
   if (CXXRD && IncludeVirtualBases) {
     const ASTRecordLayout::VBaseOffsetsMapTy &VtorDisps =
         Layout.getVBaseOffsetsMap();
 
     for (const CXXBaseSpecifier &Base : CXXRD->vbases()) {
       assert(Base.isVirtual() && "Found non-virtual class!");

       }
 
       DumpRecordLayout(OS, VBase, C, VBaseOffset, IndentLevel,
-                       VBase == Layout.getPrimaryBase() ?
-                         "(primary virtual base)" : "(virtual base)",
+                       VBase == Layout.getPrimaryBase()
+                           ? "(primary virtual base)"
+                           : "(virtual base)",
                        /*PrintSizeInfo=*/false,
                        /*IncludeVirtualBases=*/false);
     }
   }
 
-  if (!PrintSizeInfo) return;
+  if (!PrintSizeInfo)
+    return;
 
   PrintIndentNoOffset(OS, IndentLevel - 1);
   OS << "[sizeof=" << Layout.getSize().getQuantity();

clang/test/Preprocessor/has_attribute.cpp

 // CHECK: likely: 201803L
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
-// WINDOWS: no_unique_address: 0
+// WINDOWS: no_unique_address: 201803L
 // CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // CHECK: unlikely: 201803L

clang/test/SemaCXX/cxx2a-no-unique-address.cpp

 // RUN: %clang_cc1 -std=c++2a %s -verify -triple x86_64-linux-gnu
-// RUN: %clang_cc1 -std=c++2a %s -verify=unsupported -triple x86_64-windows
+// RUN: %clang_cc1 -std=c++2a %s -verify -triple x86_64-windows
+// RUN: %clang_cc1 -std=c++2a %s -verify -triple x86_64-windows-gnu
 
 [[no_unique_address]] int a; // expected-error {{only applies to non-bit-field non-static data members}} unsupported-warning {{unknown}}
 [[no_unique_address]] void f(); // expected-error {{only applies to non-bit-field non-static data members}} unsupported-warning {{unknown}}