[PATCH] D39347: Fix __has_unique_object_representations based on rsmith's input

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 
----------------
erichkeane wrote:
> rsmith wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > rsmith wrote:
> > > > > This seems to be backwards?
> > > > > 
> > > > > Also, I'm not sure whether this is correct. In the strange case where `Width` is not a multiple of `Align` (because we don't round up the width), there is no padding. We should only set `HasPadding` to `true` in the `alignTo` case below.
> > > > I think you're right about it being backwards.
> > > > 
> > > > However, doesn't the struct with a single Ptr and either 1 or 3 Ints have tail padding as well?  I do believe you're right about the alignTo situation below, but only if Width changes, right?
> > > I think the idea is that a struct with one pointer and an odd number of ints, on 32-bit Windows, will have no padding per se, but its size will simply not be a multiple of its alignment. (So struct layout can put things in the four bytes after it, but will insert padding before it to place it at an 8 byte boundary.)
> > See example here: https://godbolt.org/g/Nr8C2L
> Well... oh dear.  
> 
> That would mean that: 
>   auto p = &A::N;
>   static_assert(!has_unique_object_representations_v<decltype(p)>); // not unique, since it has padding, right?
>  
>   struct S {
>      decltype(p) s;
>   };
>   static_assert(!has_unique_object_representations_v<S>); // also not unique, since 's' has padding.
> 
>   struct S2 {
>      decltype(p) s;
>      int a;
>   };
>   static_assert(has_unique_object_representations_v<S2>); // Actually unique again, since the padding is filled.
> 
> 
> Do I have this right?
The first `static_assert` looks wrong. Should be:

```
static_assert(has_unique_object_representations_v<decltype(p)>); // unique, has no padding
```

Note that `sizeof(decltype(p))` is the size without any padding. The type does not have padding itself, but might induce lead padding (due to alignment) and tail padding (due to size not being divisible by alignment) in objects it's placed within. But I think your current approach will get this all right, so long as `MPI.HasPadding` is only set to `true` in the cases where there actually is padding in the `MPI.Width` bytes of the representation (which only happens when `Width` is rounded up for alignment).


https://reviews.llvm.org/D39347