Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[shafik]

Summary

Currently the ItaniumRecordLayoutBuilder when laying out base classes has the virtual and non-virtual bases mixed up when pulling the base class layouts from the external source.

This came up in an LLDB bug where on arm64 because of differences in how it deals with tail padding would layout the bases differently without the correct layout from the external source (LLDB). This would result in some fields being off by 4 bytes.

[shafik]

clang/lib/AST/RecordLayoutBuilder.cpp

1190

@rsmith you were correct, this was indeed reversed.

[teemperor]

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.

[tschuett]

You could try:

clangxx_host -Xclang -fdump-record-layouts %p/Inputs/layout.cpp -emit-llvm -o /dev/null

You could commit the record layouts without your fix to show the differences that you patch makes.

[teemperor]

In D83008#2131796, @tschuett wrote:

You could try:

clangxx_host -Xclang -fdump-record-layouts %p/Inputs/layout.cpp -emit-llvm -o /dev/null

You could commit the record layouts without your fix to show the differences that you patch makes.

Yeah that's I think the proper way to check the results, but IIRC the problem with testing this in Clang is that foverride-record-layout= (the testing approach for this code) doesn't support specifying base class offset), so we can't even *trigger* the bug itself in Clang itself. That what I was referring to with "layout overwrite JSON file" in my comment above.

[shafik]

Adding a second test that is not arm64 specific.

[shafik]

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

[teemperor]

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

clang/lib/AST/RecordLayoutBuilder.cpp

2197

See here for the POD check that we might get wrong.

lldb/test/Shell/Expr/Inputs/layout.cpp

23

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

41

Do we actually need these locals in addition to the globals?

[shafik]

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

[shafik]

• Removing spurious local variables in test
• Simplifying the test

[shafik]

clang/lib/AST/RecordLayoutBuilder.cpp

2197

I don't believe we are getting this wrong or at least not in the original problem. Going back and checking it we seem to be getting this right.

lldb/test/Shell/Expr/Inputs/layout.cpp

41

I left them in by accident.

[shafik]

Moved from shell test

[teemperor]

LGTM, thanks for the patch!

clang/lib/AST/RecordLayoutBuilder.cpp

ItaniumRecordLayoutBuilder::LayoutBase(const BaseSubobjectInfo *Base) {
   const ASTRecordLayout &Layout = Context.getASTRecordLayout(Base->Class);
 
 
   CharUnits Offset;
 
   // Query the external layout to see if it provides an offset.
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
-    // FIXME: This appears to be reversed.
     if (Base->IsVirtual)
-      HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
-    else
       HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
+    else
+      HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
   }
 
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
   // Per GCC's documentation, it only applies to non-static data members.
   CharUnits UnpackedBaseAlign = Layout.getNonVirtualAlignment();
   CharUnits BaseAlign =
       (Packed && ((Context.getLangOpts().getClangABICompat() <=
                    LangOptions::ClangABI::Ver6) ||
static bool mustSkipTailPadding(TargetCXXABI ABI, const CXXRecordDecl *RD) {
 
   case TargetCXXABI::UseTailPaddingUnlessPOD11:
     // This is equivalent to RD->getTypeForDecl().isCXX11PODType(),
     // but with a lot of abstraction penalty stripped off.  This does
     // assume that these properties are set correctly even in C++98
     // mode; fortunately, that is true because we want to assign
     // consistently semantics to the type-traits intrinsics (or at
     // least as many of them as possible).
     return RD->isTrivial() && RD->isCXX11StandardLayout();
   }
 
   llvm_unreachable("bad tail-padding use kind");
 }
 
 static bool isMsLayout(const ASTContext &Context) {
   return Context.getTargetInfo().getCXXABI().isMicrosoft();
 }

lldb/test/Shell/Expr/Inputs/layout.cpp

+#include <cstdint>
+
+struct B1 {
+  uint8_t a;
+};
+
+struct D1 : public B1 {
+  uint8_t a;
+  uint32_t ID;
+  uint8_t b;
+};
+
+struct Mixin : public D1 {
+  uint8_t a;
+  uint32_t *arr[3];
+};
+
+struct B2 {
+  uint32_t a;
+};
+
+class D2 : public B2, public Mixin {};
+
+D2 sg;
+
+int main() {
+  D2 s;
+
+  return s.ID;
+}

lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test

+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t)&sg.ID - (intptr_t)&sg
+# CHECK: (lldb) expr (intptr_t)&sg.ID - (intptr_t)&sg
+# CHECK: (long) $0 = 12