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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: teemperor, jingham, jasonmolenda, friss.
Herald added a subscriber: kristof.beyls.
shafik marked an inline comment as done.
shafik added a subscriber: rsmith.
shafik added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1190
   if (UseExternalLayout) {
-    // FIXME: This appears to be reversed.
     if (Base->IsVirtual)
----------------
@rsmith you were correct, this was indeed reversed.


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.


https://reviews.llvm.org/D83008

Files:
  clang/lib/AST/RecordLayoutBuilder.cpp
  lldb/test/Shell/Expr/Inputs/layout.cpp
  lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test


Index: lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
===================================================================
--- /dev/null
+++ lldb/test/Shell/Expr/TestLayoutNonVirtualBaseClasses.test
@@ -0,0 +1,7 @@
+# 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
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===================================================================
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,30 @@
+#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;
+}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1187,11 +1187,10 @@
   // 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D83008.274946.patch
Type: text/x-patch
Size: 1726 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200701/1ac7a408/attachment.bin>