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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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

> I think we are talking about different things. My question was why is this a 'Shell' test (like, a test in the `Shell` directory that uses FileCheck) and not a test in the `API` directory (using Python). An 'API' test could use the proper expression testing tools. And it could actually run when doing on-device testing (which is to my knowledge not supported for Shell tests) which seems important for a test concerning a bug that only triggers when doing on-device testing (beside that one ubuntu ARM bot).


I did not realize the shell tests were not tested on device.

> Also when looking over the ARM-specific test, I think there might be two bugs that were involved in triggering it. One is the bug fixed here which triggers that Clang will produce its own layout for those classes. Now I also wonder why the layout the expression parser Clang generates doesn't match the one from the test (which appears to be a second bug). The ARM-specific test doesn't have any information in its AST that isn't also available in the expression AST, so why would they produce different layouts? Not sure what exactly is behind the "differences in how it deals with tail padding" description but I assume this is related to tail padding reuse? If yes, then maybe the second bug is that the records in our AST are (not) PODs in the expression AST (see the inline comment for where the tail padding is enabled/disabling based on whether the RD is POD).

So we are hitting this code for non-arm64 case:

  case TargetCXXABI::UseTailPaddingUnlessPOD03:
    //...
    return RD->isPOD();

and we return `false` which is correct for the original test case since it has a base class and base therefore not an aggregate (until C++17) see it here <https://github.com/llvm/llvm-project/blob/825e3bb58082eafa8db87a9034379b88f892ce9d/clang/lib/AST/DeclCXX.cpp#L206> and struct that had the difference I was looking at. I did not check for the test cases in the PR though.

In the arm64 case from the original problem we hit the following:

  case TargetCXXABI::UseTailPaddingUnlessPOD11:
     // ...
     return RD->isTrivial() && RD->isCXX11StandardLayout();

which returns `true` for the original case.


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

https://reviews.llvm.org/D83008