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

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG63b0f8c788d8: [RecordLayout] Fix ItaniumRecordLayoutBuilder 
so that is grabs the correct… (authored by shafik).
Herald added projects: clang, LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83008

Files:
  clang/lib/AST/RecordLayoutBuilder.cpp
  lldb/test/API/lang/cpp/alignas_base_class/Makefile
  lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
  lldb/test/API/lang/cpp/alignas_base_class/main.cpp


Index: lldb/test/API/lang/cpp/alignas_base_class/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/main.cpp
@@ -0,0 +1,13 @@
+struct B1 {
+  char f1;
+};
+
+struct alignas(8) B2 {
+  char f2;
+};
+
+struct D : B1, B2 {};
+
+D d3g;
+
+int main() {}
Index: lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
@@ -0,0 +1,16 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test(self):
+self.build()
+self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+# The offset of f2 should be 8 because of `alignas(8)`.
+self.expect_expr("(intptr_t) - (intptr_t)", 
result_value="8")
Index: lldb/test/API/lang/cpp/alignas_base_class/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
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/API/lang/cpp/alignas_base_class/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/main.cpp
@@ -0,0 +1,13 @@
+struct B1 {
+  char f1;
+};
+
+struct alignas(8) B2 {
+  char f2;
+};
+
+struct D : B1, B2 {};
+
+D d3g;
+
+int main() {}
Index: lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
@@ -0,0 +1,16 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test(self):
+self.build()
+self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+# The offset of f2 should be 8 because of `alignas(8)`.
+self.expect_expr("(intptr_t) - (intptr_t)", result_value="8")
Index: lldb/test/API/lang/cpp/alignas_base_class/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
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

[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 accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for the patch!


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

[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 updated this revision to Diff 276407.
shafik added a comment.

Moved from shell test


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

https://reviews.llvm.org/D83008

Files:
  clang/lib/AST/RecordLayoutBuilder.cpp
  lldb/test/API/lang/cpp/alignas_base_class/Makefile
  lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
  lldb/test/API/lang/cpp/alignas_base_class/main.cpp


Index: lldb/test/API/lang/cpp/alignas_base_class/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/main.cpp
@@ -0,0 +1,13 @@
+struct B1 {
+  char f1;
+};
+
+struct alignas(8) B2 {
+  char f2;
+};
+
+struct D : B1, B2 {};
+
+D d3g;
+
+int main() {}
Index: lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
@@ -0,0 +1,16 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test(self):
+self.build()
+self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+# The offset of f2 should be 8 because of `alignas(8)`.
+self.expect_expr("(intptr_t) - (intptr_t)", 
result_value="8")
Index: lldb/test/API/lang/cpp/alignas_base_class/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
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/API/lang/cpp/alignas_base_class/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/main.cpp
@@ -0,0 +1,13 @@
+struct B1 {
+  char f1;
+};
+
+struct alignas(8) B2 {
+  char f2;
+};
+
+struct D : B1, B2 {};
+
+D d3g;
+
+int main() {}
Index: lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
@@ -0,0 +1,16 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test(self):
+self.build()
+self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+# The offset of f2 should be 8 because of `alignas(8)`.
+self.expect_expr("(intptr_t) - (intptr_t)", result_value="8")
Index: lldb/test/API/lang/cpp/alignas_base_class/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/alignas_base_class/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
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

[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 added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2197
 // least as many of them as possible).
 return RD->isTrivial() && RD->isCXX11StandardLayout();
   }

teemperor wrote:
> See here for the POD check that we might get wrong.
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.



Comment at: lldb/test/Shell/Expr/Inputs/layout.cpp:40
+  D2 d2;
+  D3 d3;
+

teemperor wrote:
> Do we actually need these locals in addition to the globals?
I left them in by accident.


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

[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 updated this revision to Diff 276229.
shafik marked 5 inline comments as done.
shafik added a comment.

- Removing spurious local variables in test
- Simplifying the test


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

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) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# CHECK: (long) $0 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,17 @@
+#include 
+
+struct B1 {
+  char f1;
+};
+
+struct alignas(8) B2 {
+  char f2;
+};
+
+struct D : B1, B2 {
+};
+
+D d3g;
+
+int main() {
+}
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) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# CHECK: (long) $0 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,17 @@
+#include 
+
+struct B1 {
+  char f1;
+};
+
+struct alignas(8) B2 {
+  char f2;
+};
+
+struct D : B1, B2 {
+};
+
+D d3g;
+
+int main() {
+}
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

[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 added a comment.

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.


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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 added a comment.

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.


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

[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 updated this revision to Diff 275865.
shafik added a comment.

Adding a second test that is not arm64 specific.


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

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,11 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# CHECK: (long) $0 = 12
+
+expr (intptr_t) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# CHECK: (long) $1 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,43 @@
+#include 
+
+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 {};
+
+struct B3 {
+  char f1;
+};
+
+struct alignas(8) B4 {
+  char f2;
+};
+
+struct D3 : B3, B4 {
+};
+
+D2 d2g;
+D3 d3g;
+
+int main() {
+  D2 d2;
+  D3 d3;
+
+  return d2.ID + d3.f2;
+}
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,11 @@
+# RUN: %clangxx_host %p/Inputs/layout.cpp -g -o %t
+
+# RUN: %lldb %t -b -s %s | FileCheck %s
+
+expr (intptr_t) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# CHECK: (long) $0 = 12
+
+expr (intptr_t) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# CHECK: (long) $1 = 8
Index: lldb/test/Shell/Expr/Inputs/layout.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/Inputs/layout.cpp
@@ -0,0 +1,43 @@
+#include 
+
+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 {};
+
+struct B3 {
+  char f1;
+};
+
+struct alignas(8) B4 {
+  char f2;
+};
+
+struct D3 : B3, B4 {
+};
+
+D2 d2g;
+D3 d3g;
+
+int main() {
+  D2 d2;
+  D3 d3;
+
+  return d2.ID + d3.f2;
+}
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

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

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.


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

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

You could try:

  clangxx_host -Xclang -fdump-record-layouts %p/Inputs/layout.cpp 


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

[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 requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

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 arm64e (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.


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

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


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

[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) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# 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 
+
+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) - (intptr_t)
+# CHECK: (lldb) expr (intptr_t) - (intptr_t)
+# 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 
+
+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