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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
