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