[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

Sun Jul 5 04:16:16 PDT 2020

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Thanks for tracking this down, this is a really nasty bug...

The fix itself is obviously fine, but I think I'm out of the loop regarding the testing strategy. We talked about adding a Clang test for this with the help of this layout overwrite JSON file. I assume that extending this to cover virtual bases turned out to be more complicated than expected? FWIW, I'm not necessarily the biggest fan of this Clang test option so I would be fine if we leave it as-is.

I think having an LLDB test is a good idea, but it's not clear why it's a Shell test. If I understand correctly this test requires running on arm64e (so, a remote test target), but from what I remember all the remote platform support is only in dotest.py? Also pretty much all other expression evaluation tests and the utilities for that are in the Python/API test infrastructure, so it's a bit out of place.

Also I think the test can be in general much simpler than an arm64-specific test. We get all base class offsets wrong in LLDB, so we can just write a simple test where you change the layout of some structs in a way that it doesn't fit the default layout. E.g., just putting `alignas` on a base class and then reading fields should be enough to trigger the same bug.

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

https://reviews.llvm.org/D83008