[llvm-dwp] Fix merging of debug_str_offsets with multiple contributions #90461
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-debuginfo Author: Morten larsen (molar) ChangesThis pull request will change the merging of Full diff: https://github.com/llvm/llvm-project/pull/90461.diff 5 Files Affected:
diff --git a/llvm/lib/DWP/DWP.cpp b/llvm/lib/DWP/DWP.cpp
index 77bd22d1f071c6..fecd184ca68a86 100644
--- a/llvm/lib/DWP/DWP.cpp
+++ b/llvm/lib/DWP/DWP.cpp
@@ -415,6 +415,17 @@ Expected<InfoSectionUnitHeader> parseInfoSectionUnitHeader(StringRef Info) {
return Header;
}
+static void writeNewOffsetsTo(MCStreamer &Out, DataExtractor &Data,
+ DenseMap<uint64_t, uint32_t> &OffsetRemapping,
+ uint64_t &Offset, uint64_t &Size) {
+
+ while (Offset < Size) {
+ auto OldOffset = Data.getU32(&Offset);
+ auto NewOffset = OffsetRemapping[OldOffset];
+ Out.emitIntValue(NewOffset, 4);
+ }
+}
+
void writeStringsAndOffsets(MCStreamer &Out, DWPStringPool &Strings,
MCSection *StrOffsetSection,
StringRef CurStrSection,
@@ -439,17 +450,30 @@ void writeStringsAndOffsets(MCStreamer &Out, DWPStringPool &Strings,
Out.switchSection(StrOffsetSection);
- uint64_t HeaderSize = debugStrOffsetsHeaderSize(Data, Version);
uint64_t Offset = 0;
uint64_t Size = CurStrOffsetSection.size();
- // FIXME: This can be caused by bad input and should be handled as such.
- assert(HeaderSize <= Size && "StrOffsetSection size is less than its header");
- // Copy the header to the output.
- Out.emitBytes(Data.getBytes(&Offset, HeaderSize));
- while (Offset < Size) {
- auto OldOffset = Data.getU32(&Offset);
- auto NewOffset = OffsetRemapping[OldOffset];
- Out.emitIntValue(NewOffset, 4);
+ if (Version > 4) {
+ while (Offset < Size) {
+ uint64_t HeaderSize = debugStrOffsetsHeaderSize(Data, Version);
+ assert(HeaderSize <= Size - Offset &&
+ "StrOffsetSection size is less than its header");
+
+ uint64_t ContributionEnd = 0;
+ uint64_t ContributionSize = 0;
+ uint64_t HeaderLengthOffset = Offset;
+ if (HeaderSize == 8) {
+ ContributionSize = Data.getU32(&HeaderLengthOffset);
+ } else if (HeaderSize == 16) {
+ HeaderLengthOffset += 4; // skip the dwarf64 marker
+ ContributionSize = Data.getU64(&HeaderLengthOffset);
+ }
+ ContributionEnd = ContributionSize + HeaderLengthOffset;
+ Out.emitBytes(Data.getBytes(&Offset, HeaderSize));
+ writeNewOffsetsTo(Out, Data, OffsetRemapping, Offset, ContributionEnd);
+ }
+
+ } else {
+ writeNewOffsetsTo(Out, Data, OffsetRemapping, Offset, Size);
}
}
diff --git a/llvm/test/tools/llvm-dwp/Inputs/merge_v5/notypes/ab.dwp b/llvm/test/tools/llvm-dwp/Inputs/merge_v5/notypes/ab.dwp
new file mode 100644
index 00000000000000..a2388f0e37dff0
Binary files /dev/null and b/llvm/test/tools/llvm-dwp/Inputs/merge_v5/notypes/ab.dwp differ
diff --git a/llvm/test/tools/llvm-dwp/Inputs/merge_v5/notypes/c.dwo b/llvm/test/tools/llvm-dwp/Inputs/merge_v5/notypes/c.dwo
new file mode 100644
index 00000000000000..e58a4b0c34d0bb
Binary files /dev/null and b/llvm/test/tools/llvm-dwp/Inputs/merge_v5/notypes/c.dwo differ
diff --git a/llvm/test/tools/llvm-dwp/X86/merge.test b/llvm/test/tools/llvm-dwp/X86/merge.test
index 0cf56bd858a04b..25a1315ee63f33 100644
--- a/llvm/test/tools/llvm-dwp/X86/merge.test
+++ b/llvm/test/tools/llvm-dwp/X86/merge.test
@@ -44,3 +44,8 @@ CHECK: Index Signature INFO ABBREV
CHECK-DAG: [[DWOC]] [0x00000000[[#COFF]], 0x00000000[[#AOFF]]) [0x0000[[CAOFF]], 0x0000[[AAOFF]]) [0x00000000, 0x00000011) [0x00000000, 0x00000018)
CHECK-DAG: [[DWOA]] [0x00000000[[#AOFF]], 0x00000000[[#BOFF]]) [0x0000[[AAOFF]], 0x0000[[BAOFF]]) [0x00000011, 0x00000022) [0x00000018, 0x00000028)
CHECK-DAG: [[DWOB]] [0x00000000[[#BOFF]], 0x00000000[[#XOFF]]) [0x0000[[BAOFF]], 0x000000c3) [0x00000022, 0x00000033) [0x00000028, 0x0000003c)
+
+CHECK-LABEL: .debug_str_offsets.dwo contents:
+CHECK: Contribution size = 24, Format = DWARF32, Version = 4
+CHECK: Contribution size = 16, Format = DWARF32, Version = 4
+CHECK: Contribution size = 20, Format = DWARF32, Version = 4
diff --git a/llvm/test/tools/llvm-dwp/X86/merge_v5.test b/llvm/test/tools/llvm-dwp/X86/merge_v5.test
new file mode 100644
index 00000000000000..ea9ef030e5d05b
--- /dev/null
+++ b/llvm/test/tools/llvm-dwp/X86/merge_v5.test
@@ -0,0 +1,48 @@
+RUN: llvm-dwp %p/../Inputs/merge_v5/notypes/c.dwo %p/../Inputs/merge_v5/notypes/ab.dwp -o - | \
+RUN: llvm-dwarfdump -v - | FileCheck --check-prefix=CHECK %s
+
+DWP from a DWO (c.dwo) and a DWP (ab.dwp, created from a.dwo and b.dwo)
+Make sure the entries for A and B are updated correctly when read/processed from ab.dwp
+a.cpp:
+ struct foo { };
+ foo a;
+
+b.cpp:
+ struct bar { };
+ void b(bar) {
+ }
+
+c.cpp:
+ typedef int baz;
+ baz c() {
+ }
+
+CHECK-LABEL: .debug_abbrev.dwo contents:
+CHECK-LABEL: Abbrev table for offset:
+CHECK: 0x0000[[CAOFF:.*]]
+CHECK-LABEL: Abbrev table for offset:
+CHECK: 0x0000[[AAOFF:.*]]
+CHECK-LABEL: Abbrev table for offset:
+CHECK: 0x0000[[BAOFF:.*]]
+
+CHECK: .debug_info.dwo contents:
+CHECK: 0x[[#%.8x,COFF:]]:
+CHECK-LABEL: Compile Unit: length = {{.*}}, format = DWARF32, version = 0x0005, unit_type = DW_UT_split_compile, abbr_offset =
+CHECK: 0x[[CAOFF]], addr_size = 0x08, DWO_id = 0x[[DWOC:.*]] (next unit at 0x[[#%.8x,BOFF:]])
+CHECK: [[#BOFF]]:
+CHECK-LABEL: Compile Unit: length = {{.*}}, format = DWARF32, version = 0x0005, unit_type = DW_UT_split_compile, abbr_offset =
+CHECK: 0x[[BAOFF]], addr_size = 0x08, DWO_id = 0x[[DWOB:.*]] (next unit at 0x[[#%.8x,AOFF:]])
+CHECK: [[#AOFF]]:
+CHECK-LABEL: Compile Unit: length = {{.*}}, format = DWARF32, version = 0x0005, unit_type = DW_UT_split_compile, abbr_offset =
+CHECK: 0x[[AAOFF]], addr_size = 0x08, DWO_id = 0x[[DWOA:.*]] (next unit at 0x[[#%.8x,XOFF:]])
+
+CHECK-LABEL: .debug_cu_index contents:
+CHECK: Index Signature INFO ABBREV STR_OFFSETS
+CHECK-DAG: [[DWOC]] [0x00000000[[#COFF]], 0x00000000[[#BOFF]]) [0x0000[[CAOFF]], 0x0000[[AAOFF]]) [0x00000000, 0x00000024)
+CHECK-DAG: [[DWOB]] [0x00000000[[#BOFF]], 0x00000000[[#AOFF]]) [0x0000[[BAOFF]],
+CHECK-DAG: [[DWOA]] [0x00000000[[#AOFF]], 0x00000000[[#XOFF]]) [0x0000[[AAOFF]],
+
+CHECK-LABEL: .debug_str_offsets.dwo contents:
+CHECK: Contribution size = 32, Format = DWARF32, Version = 5
+CHECK: Contribution size = 24, Format = DWARF32, Version = 5
+CHECK: Contribution size = 28, Format = DWARF32, Version = 5
|
|
Ping @dwblaikie kindly requesting a review |
|
Thanks! Generally I think we're moving away from checked in binary object files - could you make the test from checked in assembly instead (perhaps using @MaskRay's new https://llvm.org/docs/TestingGuide.html#elaborated-tests perhaps) So if I'm understanding correctly - you're saying llvm-dwp did not work when an input was a DWP and was DWARFv5? huh, surprising oversight... oh, I guess not that many people use dwp files as input to dwp actions, so I can see how some people would've been working along fine with this state of affairs. |
I think the dwarf YAML support should work for debug_str_offsets too? |
molar
force-pushed
the
molar_fix_dwp_merging_debug_str_offsets_upstream
branch
3 times, most recently
from
7847693
to
120b7b1
Compare
| CHECK-LABEL: .debug_str_offsets.dwo contents: | ||
| CHECK: Contribution size = 32, Format = DWARF32, Version = 5 | ||
| CHECK: Contribution size = 24, Format = DWARF32, Version = 5 | ||
| CHECK: Contribution size = 28, Format = DWARF32, Version = 5 | ||
|
|
There was a problem hiding this comment.
So the old behavior would've been a single contribution, and now there's three? /might/ be worth checking the content too, but I wouldn't insist on it/I could see it going either way, the length probably provides a fair amount of checking (like we didn't emit all the strings into one contribution, and then emit two empty headers)
There was a problem hiding this comment.
I have added a verbose check of the contents, let me know if there is a better way to do it, other than just comparing against the output.
Thank you for looking at this PR. We encounter this bug when using bazel along with llvm-dwp tool to generate .dwp files for our top-level binaries. In bazel it will switch to a tree merging when the number of .dwo inputs is larger than 100. It does not use the |
|
(Might be useful to use |
…ection added test of merging contributions in debug_str_offsets
molar
force-pushed
the
molar_fix_dwp_merging_debug_str_offsets_upstream
branch
from
8b4abbe
to
d843f12
Compare
molar
changed the title
|
Do you need someone to merge this for you? |
Yes please |
|
@molar Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…ns (llvm#90461) This pull request will change the merging of ``debug_str_offset`` to merge per contribution and correctly copy over each contribution header to the merged section. I have added some test data which is in dwarf5 format as this is where the section contribution header was introduced, as far as i can tell.
This pull request will change the merging of
debug_str_offsetto merge per contribution and correctly copy over each contribution header to the merged section. I have added some test data which is in dwarf5 format as this is where the section contribution header was introduced, as far as i can tell.