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