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

Mon Jul 6 17:07:38 PDT 2020

shafik added a comment.

In D83008#2131776 <https://reviews.llvm.org/D83008#2131776>, @teemperor wrote:

> 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 arm64 (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.

Good idea using `alignas` that actually did the trick, I was having trouble getting this to reproduce otherwise. I added a second test which should also reproduce on non-arm64 cases but I will keep the original test as well since they are hitting the bug from slightly different paths.

The goal of using a shell test was to avoid writing a device specific test and even though the first case should also pass on all other platforms.

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

https://reviews.llvm.org/D83008