Skip to content
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

[lldb][test] Add test for no_unique_address when mixed with bitfields #108155

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

Michael137
Copy link
Member

This is the root-cause for the LLDB failures that started occurring after #105865.

The DWARFASTParserClang has logic to try derive unnamed bitfields from DWARF offsets. In this case we treat padding as a 1-byte size field that would overlap with flag, and decide we need to introduce an unnamed bitfield into the AST, which is incorrect.

This is the root-cause for the LLDB failures that started
occurring after llvm#105865.

The DWARFASTParserClang has logic to try derive unnamed bitfields
from DWARF offsets. In this case we treat `padding` as a 1-byte
size field that would overlap with `flag`, and decide we need to
introduce an unnamed bitfield into the AST, which is incorrect.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 11, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This is the root-cause for the LLDB failures that started occurring after #105865.

The DWARFASTParserClang has logic to try derive unnamed bitfields from DWARF offsets. In this case we treat padding as a 1-byte size field that would overlap with flag, and decide we need to introduce an unnamed bitfield into the AST, which is incorrect.


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

1 Files Affected:

  • (added) lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp (+30)
diff --git a/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp b/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp
new file mode 100644
index 00000000000000..950bd9585baa22
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp
@@ -0,0 +1,30 @@
+// LLDB currently erroneously adds an unnamed bitfield
+// into the AST when a overlapping no_unique_address
+// field precedes a bitfield.
+
+// XFAIL: *
+
+// RUN: %clangxx_host -gdwarf -o %t %s
+// RUN: %lldb %t \
+// RUN:   -o "target var global" \
+// RUN:   -o "image dump ast" \
+// RUN:   -o exit | FileCheck %s
+
+// CHECK:      (lldb) image dump ast
+// CHECK-NEXT: CXXRecordDecl {{.*}} struct Foo definition
+// CHECK:      |-FieldDecl {{.*}} data 'char[5]'
+// CHECK-NEXT: |-FieldDecl {{.*}} padding 'Empty'
+// CHECK-NEXT: |-FieldDecl {{.*}} 'int'
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 8
+// CHECK-NEXT: `-FieldDecl {{.*}} sloc> mem 'unsigned long'
+// CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+
+struct Empty {};
+
+struct Foo {
+  char data[5];
+  [[no_unique_address]] Empty padding;
+  unsigned long flag : 1;
+};
+
+Foo global;

Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

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

The test will fail on windows because no_unique_address is not a thing there:

$ bin/clang++ -c --target=x86_64-pc-windows -gdwarf -o /tmp/a.out /tmp/a.cc
/tmp/a.cc:23:5: warning: unknown attribute 'no_unique_address' ignored [-Wunknown-attributes]
   23 |   [[no_unique_address]] Empty padding;
      |     ^~~~~~~~~~~~~~~~~

I suggest hardcoding the test with your favourite triple.

@frederick-vs-ja
Copy link
Contributor

The test will fail on windows because no_unique_address is not a thing there:

Seems like that we should spell it as [[msvc::no_unique_address]] on Windows. Is there already such a macro in lldb/test covering this?

@Michael137
Copy link
Member Author

The test will fail on windows because no_unique_address is not a thing there:

Seems like that we should spell it as [[msvc::no_unique_address]] on Windows. Is there already such a macro in lldb/test covering this?

Don't think we have coverage for the [[msvc::no_unique_address]]. I'm not sure whether this attribute affects PDB (or DWARF for that matter) in the same way that [[no_unique_address]] does. And I don't have the setup to test this unfortunately. So I'd rather add a triple explicitly like Pavel suggested, since that exercises exactly the failing codepath that I saw.

@Michael137 Michael137 merged commit da69449 into llvm:main Sep 11, 2024
7 checks passed
@Michael137 Michael137 deleted the bugfix/lldb-nua-bitfield branch September 11, 2024 10:36
Michael137 added a commit that referenced this pull request Sep 12, 2024
… into helper (#108196)

This logic will need adjusting soon for
#108155

This patch pulls out the logic for detecting/creating unnamed bitfields
out of `ParseSingleMember` to make the latter (in my opinion) more
readable. Otherwise we have a large number of similarly named variables
in scope.
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
…llvm#108155)

This is the root-cause for the LLDB failures that started occurring
after llvm#105865.

The DWARFASTParserClang has logic to try derive unnamed bitfields from
DWARF offsets. In this case we treat `padding` as a 1-byte size field
that would overlap with `flag`, and decide we need to introduce an
unnamed bitfield into the AST, which is incorrect.
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
… into helper (llvm#108196)

This logic will need adjusting soon for
llvm#108155

This patch pulls out the logic for detecting/creating unnamed bitfields
out of `ParseSingleMember` to make the latter (in my opinion) more
readable. Otherwise we have a large number of similarly named variables
in scope.
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.

4 participants