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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

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).

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).



================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2197
     // least as many of them as possible).
     return RD->isTrivial() && RD->isCXX11StandardLayout();
   }
----------------
See here for the POD check that we might get wrong.


================
Comment at: lldb/test/Shell/Expr/Inputs/layout.cpp:22
+
+class D2 : public B2, public Mixin {};
+
----------------
I think there should be some comment that explains why this test is structured like this (maybe point out where the tail padding change is happening).


================
Comment at: lldb/test/Shell/Expr/Inputs/layout.cpp:40
+  D2 d2;
+  D3 d3;
+
----------------
Do we actually need these locals in addition to the globals?


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

https://reviews.llvm.org/D83008