Skip to content

[lldb][test] Add test-cases for packed/aligned structures #96932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented Jun 27, 2024

Adds test that checks whether LLDB correctly infers the
alignment of packed structures. Specifically, the
InferAlignment code-path of the ItaniumRecordLayoutBuilder
where it assumes that overlapping field offsets imply a
packed structure and thus sets alignment to 1. See discussion
in #93809.

While here, also added a test-case where we check alignment of
a class whose base has an explicit `DW_AT_alignment
(those don't get transitively propagated in DWARF, but don't seem
like a problem for LLDB).

Lastly, also added an XFAIL-ed tests where the aforementioned
InferAlignment kicks in for overlapping fields (but in this
case incorrectly since the structure isn't actually packed).

@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

Adds test that checks that LLDB correctly infers the alignment of packed structures. Specifically, the
InferAlignment code-path of the ItaniumRecordLayoutBuilder where it assumes that overlapping field offsets imply a packed structure and thus sets alignment to 1. See discussion in #93809.

Also adds two XFAIL-ed tests:

  1. where LLDB doesn't correctly infer the alignment of a derived class whose base has an explicit `DW_AT_alignment. See DebugInfo: No alignment info for "transitive" alignment-enforced structures. #73623.
  2. where the aforementioned InferAlignment kicks in for overlapping fields (but in this case incorrectly since the structure isn't actually packed).

Full diff: https://github.com/llvm/llvm-project/pull/96932.diff

4 Files Affected:

  • (modified) lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py (+8)
  • (modified) lldb/test/API/lang/cpp/alignas_base_class/main.cpp (+6)
  • (added) lldb/test/Shell/SymbolFile/DWARF/no_unique_address-alignment.cpp (+25)
  • (added) lldb/test/Shell/SymbolFile/DWARF/packed.cpp (+36)
diff --git a/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py b/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
index 7d97b0c42b7e1..f9c99d15e5891 100644
--- a/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
+++ b/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
@@ -16,3 +16,11 @@ def test(self):
         # Verify specified class alignments.
         self.expect_expr("alignof(B2)", result_value="8")
         self.expect_expr("alignof(EmptyClassAlign8)", result_value="8")
+
+    @no_debug_info_test
+    @expectedFailureAll(bugnumber="https://github.com/llvm/llvm-project/issues/73623")
+    def test(self):
+        self.build()
+        self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+        self.expect_expr("alignof(Derived)", result_value="8")
diff --git a/lldb/test/API/lang/cpp/alignas_base_class/main.cpp b/lldb/test/API/lang/cpp/alignas_base_class/main.cpp
index 9d37554957ba3..fb922c42edfc3 100644
--- a/lldb/test/API/lang/cpp/alignas_base_class/main.cpp
+++ b/lldb/test/API/lang/cpp/alignas_base_class/main.cpp
@@ -13,4 +13,10 @@ D d3g;
 struct alignas(8) EmptyClassAlign8 {
 } t;
 
+struct alignas(8) __attribute__((packed)) AlignedAndPackedBase {} foo;
+
+struct Derived : AlignedAndPackedBase {
+} bar;
+static_assert(alignof(Derived) == 8);
+
 int main() {}
diff --git a/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-alignment.cpp b/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-alignment.cpp
new file mode 100644
index 0000000000000..79199a9237a63
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-alignment.cpp
@@ -0,0 +1,25 @@
+// XFAIL: *
+
+// RUN: %clangxx_host -gdwarf -o %t %s
+// RUN: %lldb %t \
+// RUN:   -o "b main" \
+// RUN:   -o "expr alignof(OverlappingFields)" \
+// RUN:   -o "expr sizeof(OverlappingFields)" \
+// RUN:   -o exit | FileCheck %s
+
+// CHECK:      (lldb) expr alignof(OverlappingFields)
+// CHECK-NEXT: ${{.*}} = 4
+// CHECK:      (lldb) expr sizeof(OverlappingFields)
+// CHECK-NEXT: ${{.*}} = 8
+
+struct Empty {};
+
+struct OverlappingFields {
+    char y;
+    [[no_unique_address]] Empty e;
+    int z;
+} g_packed_struct;
+static_assert(alignof(OverlappingFields) == 4);
+static_assert(sizeof(OverlappingFields) == 8);
+
+int main() {}
diff --git a/lldb/test/Shell/SymbolFile/DWARF/packed.cpp b/lldb/test/Shell/SymbolFile/DWARF/packed.cpp
new file mode 100644
index 0000000000000..53b5ee624472c
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/packed.cpp
@@ -0,0 +1,36 @@
+// RUN: %clangxx_host -gdwarf -o %t %s
+// RUN: %lldb %t \
+// RUN:   -o "b main" \
+// RUN:   -o "expr alignof(packed)" \
+// RUN:   -o "expr sizeof(packed)" \
+// RUN:   -o "expr alignof(packed_and_aligned)" \
+// RUN:   -o "expr sizeof(packed_and_aligned)" \
+// RUN:   -o exit | FileCheck %s
+
+// CHECK:      (lldb) expr alignof(packed)
+// CHECK-NEXT: ${{.*}} = 1
+// CHECK:      (lldb) expr sizeof(packed)
+// CHECK-NEXT: ${{.*}} = 9
+
+// CHECK:      (lldb) expr alignof(packed_and_aligned)
+// CHECK-NEXT: ${{.*}} = 16
+// CHECK:      (lldb) expr sizeof(packed_and_aligned)
+// CHECK-NEXT: ${{.*}} = 16
+
+struct __attribute__((packed)) packed {
+    int x;
+    char y;
+    int z;
+} g_packed_struct;
+static_assert(alignof(packed) == 1);
+static_assert(sizeof(packed) == 9);
+
+struct __attribute__((packed, aligned(16))) packed_and_aligned {
+    int x;
+    char y;
+    int z;
+} g_packed_and_aligned_struct;
+static_assert(alignof(packed_and_aligned) == 16);
+static_assert(sizeof(packed_and_aligned) == 16);
+
+int main() {}

@Michael137 Michael137 requested a review from labath June 27, 2024 23:04
Adds test that checks whether LLDB correctly infers the
alignment of packed structures. Specifically, the
`InferAlignment` code-path of the `ItaniumRecordLayoutBuilder`
where it assumes that overlapping field offsets imply a
packed structure and thus sets alignment to `1`. See discussion
in llvm#93809.

While here, also added a test-case where we check alignment of
a class whose base has an explicit `DW_AT_alignment
(those don't get transitively propagated in DWARF, but don't seem
like a problem for LLDB).

Lastly, also added an XFAIL-ed tests where the aforementioned
`InferAlignment` kicks in for overlapping fields (but in this
case incorrectly since the structure isn't actually packed).
@Michael137 Michael137 force-pushed the lldb/test/packed-attr branch from 2113f05 to d2c2870 Compare June 27, 2024 23:45

// RUN: %clangxx_host -gdwarf -o %t %s
// RUN: %lldb %t \
// RUN: -o "b main" \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you're not actually running the binary. Do you need the breakpoint?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, removed

@@ -0,0 +1,36 @@
// RUN: %clangxx_host -gdwarf -o %t %s
// RUN: %lldb %t \
// RUN: -o "b main" \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@Michael137 Michael137 requested review from DavidSpickett and removed request for DavidSpickett June 28, 2024 13:05
@Michael137 Michael137 merged commit 46e848a into llvm:main Jun 28, 2024
6 checks passed
@Michael137 Michael137 deleted the lldb/test/packed-attr branch June 28, 2024 14:30
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jun 28, 2024
Follow-up to llvm#96932

Adds XFAILed test where LLDB incorrectly infers the
alignment of a derived class whose bases are overlapping
due to [[no_unique_address]]. Specifically, the `InferAlignment`
code-path of the `ItaniumRecordLayoutBuilder` assumes that overlapping
base offsets imply a packed structure and thus sets alignment
to 1. See discussion in llvm#93809.

Also adds test where LLDB correctly infers an alignment
of `1` when a packed base class is overlapping with other bases.

Lastly, there were a couple of `alignof` inconsistencies which
I encapsulated in an XFAIL-ed `packed-alignof.cpp`.
Michael137 added a commit that referenced this pull request Jun 29, 2024
…97068)

Follow-up to #96932

Adds XFAILed test where LLDB incorrectly infers the alignment of a
derived class whose bases are overlapping due to [[no_unique_address]].
Specifically, the `InferAlignment` code-path of the
`ItaniumRecordLayoutBuilder` assumes that overlapping base offsets imply
a packed structure and thus sets alignment to 1. See discussion in
#93809.

Also adds test where LLDB correctly infers an alignment of `1` when a
packed base class is overlapping with other bases.

Lastly, there were a couple of `alignof` inconsistencies which I
encapsulated in an XFAIL-ed `packed-alignof.cpp`.
omjavaid added a commit that referenced this pull request Jun 29, 2024
SymbolFile/DWARF/packed.cpp introduced by #96932 has been failing on
AArch64/Windows buildbot.

https://lab.llvm.org/buildbot/#/builders/141/builds/374

I am marking it as an XFAIL to make the buildbot happy.
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
Adds test that checks whether LLDB correctly infers the
alignment of packed structures. Specifically, the
`InferAlignment` code-path of the `ItaniumRecordLayoutBuilder`
where it assumes that overlapping field offsets imply a
packed structure and thus sets alignment to `1`. See discussion
in llvm#93809.

While here, also added a test-case where we check alignment of
a class whose base has an explicit `DW_AT_alignment
(those don't get transitively propagated in DWARF, but don't seem
like a problem for LLDB).

Lastly, also added an XFAIL-ed tests where the aforementioned
`InferAlignment` kicks in for overlapping fields (but in this
case incorrectly since the structure isn't actually packed).
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…lvm#97068)

Follow-up to llvm#96932

Adds XFAILed test where LLDB incorrectly infers the alignment of a
derived class whose bases are overlapping due to [[no_unique_address]].
Specifically, the `InferAlignment` code-path of the
`ItaniumRecordLayoutBuilder` assumes that overlapping base offsets imply
a packed structure and thus sets alignment to 1. See discussion in
llvm#93809.

Also adds test where LLDB correctly infers an alignment of `1` when a
packed base class is overlapping with other bases.

Lastly, there were a couple of `alignof` inconsistencies which I
encapsulated in an XFAIL-ed `packed-alignof.cpp`.
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
SymbolFile/DWARF/packed.cpp introduced by llvm#96932 has been failing on
AArch64/Windows buildbot.

https://lab.llvm.org/buildbot/#/builders/141/builds/374

I am marking it as an XFAIL to make the buildbot happy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants