[Lldb-commits] [clang] [lldb] [clang][RecordLayoutBuilder] Be stricter about inferring packed-ness in ExternalLayouts (PR #97443)

[Michael Buch via lldb-commits]

Michael137 wrote:

> > FWIW, I came across another no_unique_address-related crash today:
> > ```
> > $ cat a.cc
> > struct S {
> > private:
> >   int i;
> >   short s;
> > };
> > static_assert(sizeof(S) == 8);
> > 
> > struct T {
> >   [[no_unique_address]] S s;
> >   char c;
> > };
> > static_assert(sizeof(T) == 8);
> > 
> > T t;
> > $ clang++ -c -o /tmp/a.out /tmp/a.cc -g
> > $ lldb /tmp/a.out -o "expr t" -x
> > (lldb) target create "/tmp/a.out"
> > Current executable set to '/tmp/a.out' (x86_64).
> > (lldb) expr t
> > assertion failed at clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:960 in void (anonymous namespace)::CGRecordLowering::checkBitfieldClipping(bool) const: M.Offset >= Tail && "Bitfield access unit is not clipped"
> > ...
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > I was hoping this PR would fix it, but it doesn't appear to help.
> 
> Hmmm interesting, yea that looks more like something that #96422 should've addressed. Though this is a special case where the attribute is applied to a non-empty type. Would need to step through the `CGRecordLayoutBuilder` to see why Clang believes the offsets from DWARF aren't correct.

Yea I took a quick look at this and the problem is the following check: https://github.com/llvm/llvm-project/blob/44be5a7fdc20a7f90d63dc18699a470e900bd3ba/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp#L392

`S` is potentially overlapping because it has the `NoUniqueAddress` attribute, but that isn't present in the DWARF AST. So when we compute the record layout using the DWARF AST we push_back the incorrect MemberInfo here:
```
(lldb) p getStorageType(Field->getType()->getAsCXXRecordDecl())->dump()                                                                                                                           
%struct.S.base = type <{ i32, i16 }>                                                                                                                                                              
(lldb) p getStorageType(*Field)->dump()                                                                                                                                                                                                                                                                   
%struct.S = type <{ i32, i16, [2 x i8] }>                                                                 
(lldb) n                                                                                                  
```
In the `isPotentiallyOverlapping` case we would've picked the layout without the tail padding.

So we're back to "how do we encode this isPotentiallyOverlapping property DWARF". OR, maybe we can get away with removing that dependency on the AST in the same way we did with `isZeroSize`..

https://github.com/llvm/llvm-project/pull/97443