Skip to content

Support big endian in llvm-symbolizer's data location dwarf info parser #67284

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 8 commits into from
Oct 10, 2023

Conversation

chenzheng1030
Copy link
Collaborator

@chenzheng1030 chenzheng1030 commented Sep 25, 2023

For now, data location expression is hard coded to little endian. We are going to support sanitizers on AIX which is big endian. Support big endian too in the data location expression parser of llvm-symbolizer.

@llvmbot
Copy link
Member

llvmbot commented Sep 25, 2023

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-binary-utilities

Changes

For now, data location expression is hard coded to little endian. We are going to support sanitizers on AIX which is big endian. Support big endian too in the data location expression parser of llvm-symbolizer.

Currently obj2yaml/yaml2obj can not support debug info for xcoff, so I used an object. Will update this once obj2yaml/yaml2obj are ready for XCOFF objects with DWARF info


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

3 Files Affected:

  • (modified) llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp (+1-1)
  • (added) llvm/test/tools/llvm-symbolizer/Inputs/xcoff-dwarf.o ()
  • (added) llvm/test/tools/llvm-symbolizer/xcoff.test (+42)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index 387345a4ac2d601..0cd45bde3e25349 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -784,7 +784,7 @@ void DWARFUnit::updateVariableDieMap(DWARFDie Die) {
 
   for (const DWARFLocationExpression &Location : *Locations) {
     uint8_t AddressSize = getAddressByteSize();
-    DataExtractor Data(Location.Expr, /*IsLittleEndian=*/true, AddressSize);
+    DataExtractor Data(Location.Expr, isLittleEndian(), AddressSize);
     DWARFExpression Expr(Data, AddressSize);
     auto It = Expr.begin();
     if (It == Expr.end())
diff --git a/llvm/test/tools/llvm-symbolizer/Inputs/xcoff-dwarf.o b/llvm/test/tools/llvm-symbolizer/Inputs/xcoff-dwarf.o
new file mode 100644
index 000000000000000..04326b6ccedd046
Binary files /dev/null and b/llvm/test/tools/llvm-symbolizer/Inputs/xcoff-dwarf.o differ
diff --git a/llvm/test/tools/llvm-symbolizer/xcoff.test b/llvm/test/tools/llvm-symbolizer/xcoff.test
new file mode 100644
index 000000000000000..26a57f1f87b16bd
--- /dev/null
+++ b/llvm/test/tools/llvm-symbolizer/xcoff.test
@@ -0,0 +1,42 @@
+
+RUN: llvm-symbolizer --obj=%p/Inputs/xcoff-dwarf.o 'DATA 0x60' \
+RUN:    'DATA 0x61' 'DATA 0x64' 'DATA 0X68' 'DATA 0x90' 'DATA 0x94' \
+RUN:    'DATA 0X98' | FileCheck %s
+
+CHECK: bss_global
+CHECK-NEXT: 96 4
+CHECK-NEXT: /t.cpp:1
+CHECK-EMPTY:
+
+CHECK: bss_global
+CHECK-NEXT: 96 4
+CHECK-NEXT: /t.cpp:1
+CHECK-EMPTY:
+
+CHECK: data_global
+CHECK-NEXT: 100 4
+CHECK-NEXT: /t.cpp:2
+CHECK-EMPTY:
+
+CHECK: str
+CHECK-NEXT: 104 4
+CHECK-NEXT: /t.cpp:4
+CHECK-EMPTY:
+
+FIXME: fix the wrong size 152
+CHECK: f()::function_global
+CHECK-NEXT: 144 152
+CHECK-NEXT: /t.cpp:8
+CHECK-EMPTY:
+
+FIXME: fix the wrong size 152
+CHECK: beta
+CHECK-NEXT: 148 152
+CHECK-NEXT: /t.cpp:13
+CHECK-EMPTY:
+
+FIXME: fix the wrong size 152
+CHECK: alpha
+CHECK-NEXT: 152 152
+CHECK-NEXT: /t.cpp:12
+CHECK-EMPTY:

@chenzheng1030 chenzheng1030 self-assigned this Sep 25, 2023
@jh7370
Copy link
Collaborator

jh7370 commented Sep 25, 2023

I've added a couple of colleagues who are more familiar with the details of the DWARF standard and library than me, as I don't think my knowledge in this area is strong enough to review the change.

Also, the functional change is in the DWARFDebugInfo library, not in the symbolizer code, so the test really belongs with the tests for that library, not with the llvm-symbolizer tool tests.

@chenzheng1030
Copy link
Collaborator Author

Also, the functional change is in the DWARFDebugInfo library, not in the symbolizer code, so the test really belongs with the tests for that library, not with the llvm-symbolizer tool tests.

Thanks, will update soon. The symbol size issue is fixed in #67304

@pogo59
Copy link
Collaborator

pogo59 commented Sep 25, 2023

I've added a couple of colleagues who are more familiar with the details of the DWARF standard and library than me, as I don't think my knowledge in this area is strong enough to review the change.

yes, well, the code change looks completely appropriate (I suspect it was hard-coded to little endian originally due to lack of big-endian DWARF targets). On the other hand, I am unfamiliar with llvm-symbolizer so I have no idea what the test is testing.

@dwblaikie dwblaikie requested review from jh7370 and removed request for jh7370 September 25, 2023 17:45
@dwblaikie
Copy link
Collaborator

Currently obj2yaml/yaml2obj can not support debug info for xcoff, so I used an object. Will update this once obj2yaml/yaml2obj are ready for XCOFF objects with DWARF info

Could you use some raw assembly & assemble it with llvm-mc, rather than a checked in object? That'd be a bit easier to maintain, maybe... (& if it's compiled from some higher level source - include that source and a command used to produce the assembly)

@chenzheng1030
Copy link
Collaborator Author

Currently obj2yaml/yaml2obj can not support debug info for xcoff, so I used an object. Will update this once obj2yaml/yaml2obj are ready for XCOFF objects with DWARF info

Could you use some raw assembly & assemble it with llvm-mc, rather than a checked in object? That'd be a bit easier to maintain, maybe... (& if it's compiled from some higher level source - include that source and a command used to produce the assembly)

Yes, this would make more sense if we can generate the object from some kinds of source codes. However, we are also lacking XCOFF support in LLVM asm parser. For a very simple but fundamental assembly like below:

	.csect [PR],5

LLVM AsmParser reports LLVM ERROR: XCOFFAsmParser directive not yet supported!...

And very sorry about this. We(IBM power compiler team) have plans to support AIX assembly syntax in AsmParser, but this seems need a big effort and still not yet start.

@jh7370
Copy link
Collaborator

jh7370 commented Sep 26, 2023

Is it possible to go from LLVM IR and use llc to generate the data you need for this test case? I don't know if that would work in lieu of the canned object and lack of asm support? (I'll admit to not knowing a huge amount in this area).

@chenzheng1030
Copy link
Collaborator Author

Is it possible to go from LLVM IR and use llc to generate the data you need for this test case? I don't know if that would work in lieu of the canned object and lack of asm support? (I'll admit to not knowing a huge amount in this area).

Very good comment actually. I've updated the patch, now the xcoff object is generated from IR input.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM from my point of view, but it should get another sign off too.

@chenzheng1030
Copy link
Collaborator Author

Thanks, LGTM from my point of view, but it should get another sign off too.

Thanks @jh7370

Is there any comments from the DWARF side? @pogo59 @jmorse @hctim @davidbolvansky @dwblaikie

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Sounds OK to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants