[dsymutil] Fix parallel linker's self-recursive typedef DIE by including referred-to types into synthetic name #166767
Conversation
royitaqi
requested review from
Michael137,
avl-llvm and
clayborg
and removed request for
JDevlieghere
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
royitaqi
force-pushed
the
dsymutil-fix-parallel-linker-2
branch
from
cfe98a3 to
448888c
Compare
royitaqi
changed the title
|
@llvm/pr-subscribers-debuginfo Author: Roy Shi (royitaqi) ChangesSee #166675 for description of the problem, the root cause, and one solution. This patch is a "different implementation" descried there. This patch tries to fix the problem by recursively including the referred-to types into the synthetic name. This way, the synthetic name of the typedef DIE is canonicalized. See example below: The advantages of this approach over #166675 are:
Caveat is that it won't work if any of the referenced types resolve to the same name for some reason (similar to how the two typedefs resolved to the same name before this patch). Full diff: https://github.com/llvm/llvm-project/pull/166767.diff 3 Files Affected:
diff --git a/llvm/lib/DWARFLinker/Parallel/SyntheticTypeNameBuilder.cpp b/llvm/lib/DWARFLinker/Parallel/SyntheticTypeNameBuilder.cpp
index 34174f98b7e37..ca918f6e17b38 100644
--- a/llvm/lib/DWARFLinker/Parallel/SyntheticTypeNameBuilder.cpp
+++ b/llvm/lib/DWARFLinker/Parallel/SyntheticTypeNameBuilder.cpp
@@ -377,8 +377,10 @@ Error SyntheticTypeNameBuilder::addTypeName(UnitEntryPairTy InputUnitEntryPair,
} break;
}
- // If name for the DIE is not determined yet add referenced types to the name.
- if (!HasLinkageName && !HasShortName && !HasDeclFileName) {
+ // If name for the DIE is not determined yet or if the DIE is a typedef, add
+ // referenced types to the name.
+ if ((!HasLinkageName && !HasShortName && !HasDeclFileName) ||
+ InputUnitEntryPair.DieEntry->getTag() == dwarf::DW_TAG_typedef) {
if (InputUnitEntryPair.CU->find(InputUnitEntryPair.DieEntry,
getODRAttributes()))
if (Error Err = addReferencedODRDies(InputUnitEntryPair, AddParentNames,
diff --git a/llvm/test/tools/dsymutil/Inputs/typedefs-with-same-name.o b/llvm/test/tools/dsymutil/Inputs/typedefs-with-same-name.o
new file mode 100644
index 0000000000000..6cc47c1a783b3
Binary files /dev/null and b/llvm/test/tools/dsymutil/Inputs/typedefs-with-same-name.o differ
diff --git a/llvm/test/tools/dsymutil/typedefs-with-same-name.test b/llvm/test/tools/dsymutil/typedefs-with-same-name.test
new file mode 100644
index 0000000000000..96ec583293977
--- /dev/null
+++ b/llvm/test/tools/dsymutil/typedefs-with-same-name.test
@@ -0,0 +1,41 @@
+#RUN: dsymutil --linker=parallel -f -oso-prepend-path=%p/Inputs/ -y %s -o %t.dwarf
+#RUN: llvm-dwarfdump %t.dwarf | FileCheck %s
+
+# There should be two typedef DIE named "BarInt" in the resultant .dwarf file.
+# The second should refer to the first, which refer to "Foo<int>".
+# CHECK: 0x[[FIRST_BARINT_ADDR:[0-9a-f]{8}]]: DW_TAG_typedef
+# CHECK-NEXT: DW_AT_type (0x{{([[:xdigit:]]{8})}} "Foo<int>")
+# CHECK-NEXT: DW_AT_name ("BarInt")
+# CHECK: 0x{{([[:xdigit:]]{8})}}: DW_TAG_typedef
+# CHECK-NEXT: DW_AT_type (0x[[FIRST_BARINT_ADDR]] "BarInt")
+# CHECK-NEXT: DW_AT_name ("BarInt")
+
+# Source:
+#
+# template <typename T> struct Foo;
+# typedef Foo<int> BarInt;
+# template <typename T>
+# struct [[clang::preferred_name(BarInt)]] Foo{};
+# int main() {
+# BarInt barInt;
+# return 0;
+# }
+#
+# Compile with:
+#
+# $ clang++ -g -O0 -c typedefs-with-same-name.cpp -o typedefs-with-same-name.o
+#
+# To generate the debug map:
+#
+# $ clang++ typedefs-with-same-name.o -o typedefs-with-same-name
+# $ dsymutil -dump-debug-map typedefs-with-same-name
+
+---
+triple: 'arm64-apple-darwin'
+objects:
+ - filename: '/typedefs-with-same-name.o'
+ timestamp: 1762438746
+ type: 102
+ symbols:
+ - { sym: _main, objAddr: 0x0, binAddr: 0x100000360, size: 0x14 }
+...
|
|
I prefer this approach over the one in #166675 for sure, but...
...this still worries me. Not like it's a common scenario, but it would be nice for the solution to be complete. Could you elaborate on the scenario you had in mind? Maybe I'm misunderstanding, but I think that would constitute a redefinition error in C++. The reason the typedef chain you have in your test is legal, is that it's just a simple redeclaration. I'm by no means an expert on the internals of dsymutil, so I'll defer to @JDevlieghere @avl-llvm re. implementation. But the idea makes sense to me. |
Sorry for the confusion. It was just a hypothetical caveat that I can think of. I have no evidence to either support or deny such hypothesis. I will look up to @JDevlieghere and @avl-llvm for their understanding of if such case actually exists. Let me update that text so that it's clear that it's hypothetical. |
|
Hi @royitaqi , Thank you for the patch! I think this version is correct one (comparing with checking for line numbers). LGTM! |
|
@avl-llvm: Thanks for the review. Just double checking about the "caveat" (see in the description and @Michael137 's comment and my comment below his): Do you think there is a real risk where two dependent types (not typedefs but other type of DIE) will have the same synthetic name? If it can happen, then it will cause two different typedefs to have name collision even though the dependent types' names are included (because they are the same). |
|
I do not think there are more errors here. The DW_AT_linkage_name/DW_AT_name attribute should be enough to unique types(except special cases already listed in SyntheticTypeNameBuilder::addTypeName). |
|
Thank you for confirming~! I appreciate it. I will wait till tomorrow morning before merging just in case other folks have additional input. Other than that, thank you both for the review and suggestions~ cc @Michael137 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/39616 Here is the relevant piece of the build log for the reference |
|
you probably have to move the test into an arm64 subdirectory. So bots that only have an x86 target dont try running it |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/16909 Here is the relevant piece of the build log for the reference |
Thanks I will do that now. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/33724 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/26015 Here is the relevant piece of the build log for the reference |
|
Fixing the test error in #167005 |
#166767 mistakenly put the test file outside the ARM folder, causing build bots that only have an x86 target to fail. Moving it into the ARM folder. ``` royshi-mac-home ~/public_llvm/build % bin/llvm-lit \ ../llvm-project/llvm/test/tools/dsymutil/ARM/typedefs-with-same-name.test \ ../llvm-project/llvm/test/tools/dsymutil/X86/DWARFLinkerParallel/odr-fwd-declaration.test -- Testing: 2 tests, 2 workers -- PASS: LLVM :: tools/dsymutil/ARM/typedefs-with-same-name.test (1 of 2) PASS: LLVM :: tools/dsymutil/X86/DWARFLinkerParallel/odr-fwd-declaration.test (2 of 2) Testing Time: 0.23s Total Discovered Tests: 2 Passed: 2 (100.00%) ```
…er (#167005) llvm/llvm-project#166767 mistakenly put the test file outside the ARM folder, causing build bots that only have an x86 target to fail. Moving it into the ARM folder. ``` royshi-mac-home ~/public_llvm/build % bin/llvm-lit \ ../llvm-project/llvm/test/tools/dsymutil/ARM/typedefs-with-same-name.test \ ../llvm-project/llvm/test/tools/dsymutil/X86/DWARFLinkerParallel/odr-fwd-declaration.test -- Testing: 2 tests, 2 workers -- PASS: LLVM :: tools/dsymutil/ARM/typedefs-with-same-name.test (1 of 2) PASS: LLVM :: tools/dsymutil/X86/DWARFLinkerParallel/odr-fwd-declaration.test (2 of 2) Testing Time: 0.23s Total Discovered Tests: 2 Passed: 2 (100.00%) ```
5 participants
TL;DR: See #166675 for description of the problem, the root cause, and one solution. This patch is the "different implementation" descried there.
This patch tries to fix the problem by recursively including the referred-to types into the synthetic name. This way, the synthetic name of the typedef DIE is canonicalized. See example debug prints below:
The advantages of this approach over #166675 are:
DW_TAG_typedefgenerated byclang::preferred_namehas wrongDW_AT_decl_linevalue #166673 to be fixed.A hypothetical caveat is that it would work if any of the referenced types resolve to the same name for some reason (similar to how the two typedefs resolved to the same name before this patch).
Tests