[llvm-dwp] turn duplicate dwo id into warning, continue to gen dwp #121193
Conversation
|
@llvm/pr-subscribers-debuginfo Author: Jinjie Huang (Labman-001) ChangesThe current behavior of binutils dwp when encountering duplicate DWO inputs is to issue a warning and continue generating the DWP. like this： However, llvm-dwp uses a Map structure, which cannot store duplicate entries, so it throws an error and exits when faced with such scenarios. Can we consider skipping the duplicate DWO inputs in such cases, issuing a warning instead, and continuing to generate the DWP? And it seems unnecessary to provide an option to maintain the error and exit behavior？ Full diff: https://github.com/llvm/llvm-project/pull/121193.diff 4 Files Affected:
diff --git a/llvm/lib/DWP/DWP.cpp b/llvm/lib/DWP/DWP.cpp
index fecd184ca68a86..52bf062247d31c 100644
--- a/llvm/lib/DWP/DWP.cpp
+++ b/llvm/lib/DWP/DWP.cpp
@@ -786,8 +786,11 @@ Error write(MCStreamer &Out, ArrayRef<std::string> Inputs,
return createFileError(Input, EID.takeError());
const auto &ID = *EID;
auto P = IndexEntries.insert(std::make_pair(ID.Signature, Entry));
- if (!P.second)
- return buildDuplicateError(*P.first, ID, "");
+ if (!P.second) {
+ WithColor::defaultWarningHandler(buildDuplicateError(*P.first, ID, ""));
+ FoundCUUnit = true;
+ continue;
+ }
P.first->second.Name = ID.Name;
P.first->second.DWOName = ID.DWOName;
@@ -858,8 +861,10 @@ Error write(MCStreamer &Out, ArrayRef<std::string> Inputs,
if (!EID)
return createFileError(Input, EID.takeError());
const auto &ID = *EID;
- if (!P.second)
- return buildDuplicateError(*P.first, ID, Input);
+ if (!P.second) {
+ WithColor::defaultWarningHandler(buildDuplicateError(*P.first, ID, Input));
+ continue;
+ }
auto &NewEntry = P.first->second;
NewEntry.Name = ID.Name;
NewEntry.DWOName = ID.DWOName;
diff --git a/llvm/test/tools/llvm-dwp/X86/duplicate.test b/llvm/test/tools/llvm-dwp/X86/duplicate.test
index 66eede9b5d2df7..e2049901de1600 100644
--- a/llvm/test/tools/llvm-dwp/X86/duplicate.test
+++ b/llvm/test/tools/llvm-dwp/X86/duplicate.test
@@ -1,27 +1,27 @@
-RUN: not llvm-dwp %p/../Inputs/duplicate/c.dwo %p/../Inputs/duplicate/c.dwo -o %t 2>&1 \
+RUN: llvm-dwp %p/../Inputs/duplicate/c.dwo %p/../Inputs/duplicate/c.dwo -o %t 2>&1 \
RUN: | FileCheck --check-prefix=DWOS %s
-RUN: not llvm-dwp %p/../Inputs/duplicate/c.dwo %p/../Inputs/duplicate/bc.dwp -o %t 2>&1 \
+RUN: llvm-dwp %p/../Inputs/duplicate/c.dwo %p/../Inputs/duplicate/bc.dwp -o %t 2>&1 \
RUN: | FileCheck --check-prefix=2DWP %s
-RUN: not llvm-dwp %p/../Inputs/duplicate/ac.dwp %p/../Inputs/duplicate/c.dwo -o %t 2>&1 \
+RUN: llvm-dwp %p/../Inputs/duplicate/ac.dwp %p/../Inputs/duplicate/c.dwo -o %t 2>&1 \
RUN: | FileCheck --check-prefix=1DWP %s
-RUN: not llvm-dwp %p/../Inputs/duplicate_dwo_name/c.dwo %p/../Inputs/duplicate_dwo_name/c.dwo -o %t 2>&1 \
+RUN: llvm-dwp %p/../Inputs/duplicate_dwo_name/c.dwo %p/../Inputs/duplicate_dwo_name/c.dwo -o %t 2>&1 \
RUN: | FileCheck --check-prefix=DWODWOS %s
-RUN: not llvm-dwp %p/../Inputs/duplicate_dwo_name/c.dwo %p/../Inputs/duplicate_dwo_name/bc.dwp -o %t 2>&1 \
+RUN: llvm-dwp %p/../Inputs/duplicate_dwo_name/c.dwo %p/../Inputs/duplicate_dwo_name/bc.dwp -o %t 2>&1 \
RUN: | FileCheck --check-prefix=DWO2DWP %s
-RUN: not llvm-dwp %p/../Inputs/duplicate_dwo_name/ac.dwp %p/../Inputs/duplicate_dwo_name/c.dwo -o %t 2>&1 \
+RUN: llvm-dwp %p/../Inputs/duplicate_dwo_name/ac.dwp %p/../Inputs/duplicate_dwo_name/c.dwo -o %t 2>&1 \
RUN: | FileCheck --check-prefix=DWO1DWP %s
Build from a, b, and c.c all containing a single void() func by the name of the file.
-DWOS: error: duplicate DWO ID ({{.*}}) in 'c.c' and 'c.c'{{$}}
-1DWP: error: duplicate DWO ID ({{.*}}) in 'c.c' (from '{{.*}}ac.dwp') and 'c.c'{{$}}
-2DWP: error: duplicate DWO ID ({{.*}}) in 'c.c' and 'c.c' (from '{{.*}}bc.dwp'){{$}}
+DWOS: warning: duplicate DWO ID ({{.*}}) in 'c.c' and 'c.c'{{$}}
+1DWP: warning: duplicate DWO ID ({{.*}}) in 'c.c' (from '{{.*}}ac.dwp') and 'c.c'{{$}}
+2DWP: warning: duplicate DWO ID ({{.*}}) in 'c.c' and 'c.c' (from '{{.*}}bc.dwp'){{$}}
-DWODWOS: error: duplicate DWO ID ({{.*}}) in 'c.c' (from 'c.dwo') and 'c.c' (from 'c.dwo'){{$}}
-DWO1DWP: error: duplicate DWO ID ({{.*}}) in 'c.c' (from 'c.dwo' in '{{.*}}ac.dwp') and 'c.c' (from 'c.dwo'){{$}}
-DWO2DWP: error: duplicate DWO ID ({{.*}}) in 'c.c' (from 'c.dwo') and 'c.c' (from 'c.dwo' in '{{.*}}bc.dwp'){{$}}
+DWODWOS: warning: duplicate DWO ID ({{.*}}) in 'c.c' (from 'c.dwo') and 'c.c' (from 'c.dwo'){{$}}
+DWO1DWP: warning: duplicate DWO ID ({{.*}}) in 'c.c' (from 'c.dwo' in '{{.*}}ac.dwp') and 'c.c' (from 'c.dwo'){{$}}
+DWO2DWP: warning: duplicate DWO ID ({{.*}}) in 'c.c' (from 'c.dwo') and 'c.c' (from 'c.dwo' in '{{.*}}bc.dwp'){{$}}
diff --git a/llvm/test/tools/llvm-dwp/X86/gcc_type.test b/llvm/test/tools/llvm-dwp/X86/gcc_type.test
index eb8f2ba9fd3740..1130fc6d88d2b1 100644
--- a/llvm/test/tools/llvm-dwp/X86/gcc_type.test
+++ b/llvm/test/tools/llvm-dwp/X86/gcc_type.test
@@ -1,5 +1,5 @@
RUN: llvm-dwp %p/../Inputs/gcc_type/a.dwo -o - | llvm-dwarfdump -v - | FileCheck %s
-RUN: not llvm-dwp %p/../Inputs/gcc_type/a.dwo %p/../Inputs/gcc_type/a.dwo -o %t 2>&1 | FileCheck --check-prefix=DUP %s
+RUN: llvm-dwp %p/../Inputs/gcc_type/a.dwo %p/../Inputs/gcc_type/a.dwo -o %t 2>&1 | FileCheck --check-prefix=DUP %s
CHECK: Type Unit
CHECK: Type Unit
diff --git a/llvm/test/tools/llvm-dwp/X86/handle_strx.test b/llvm/test/tools/llvm-dwp/X86/handle_strx.test
index c62f923f30e228..3df5447a50c8f8 100644
--- a/llvm/test/tools/llvm-dwp/X86/handle_strx.test
+++ b/llvm/test/tools/llvm-dwp/X86/handle_strx.test
@@ -1,7 +1,7 @@
RUN: llvm-dwp %p/../Inputs/handle_strx/dw5.dwo -o %t 2>/dev/null
RUN: llvm-dwarfdump --verbose %t 2>/dev/null | FileCheck --check-prefix=READ_STRX %s
-RUN: not llvm-dwp %p/../Inputs/handle_strx/dw5.dwo %p/../Inputs/handle_strx/dw5.dwo -o %t 2>&1 \
+RUN: llvm-dwp %p/../Inputs/handle_strx/dw5.dwo %p/../Inputs/handle_strx/dw5.dwo -o %t 2>&1 \
RUN: | FileCheck --check-prefix=PARSE_STRX %s
@@ -10,5 +10,5 @@ with options -gdwarf-5 and -gsplit-dwarf.
READ_STRX: DW_AT_name [DW_FORM_strx1]{{.*}}dw5.cc
-PARSE_STRX: error: duplicate DWO ID ({{.*}}) in 'dw5.cc' (from 'dw5.dwo') and 'dw5.cc' (from 'dw5.dwo')
+PARSE_STRX: warning: duplicate DWO ID ({{.*}}) in 'dw5.cc' (from 'dw5.dwo') and 'dw5.cc' (from 'dw5.dwo')
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@dwblaikie @ayermolo, can you help to review? thanks |
|
Is there a case for it to be a warning, other then binutils does it? |
One case is when multiple .a files in a project contain duplicate .o files. The linker can link them normally, but llvm-dwp reports an error during the process, which is somewhat counterintuitive. In such a scenario, it might be better to generate a DWP like binutils dwp does?
Those duplicate DWOs seem to be redundant when generating DWP? And btw, it seems we don't need to add an option to control the behavior when encountering duplicate DWOs? |
|
Yeah, pretty much every duplicate DWO ID issue I've tracked down inside google were ultimately benign as far as the DWARF structure is concerned - the same file compiled/linked in twice, and the linker discards one but the dwp actions don't know how to do that (if you're using Some cases might confuse a consumer - I think I saw some cases where /nearly/ empty files ended up with duplicate DWO IDs (totally empty files shouldn't produce a CU so there wouldn't be any duplicate/collission issues. Imagine some code like this: https://godbolt.org/z/qxf1x9bW9 - if there were two files with the same file name (in different directories, or compiled twice with different |
Yes, the normal duplicate .cpp should be discarded when linking as mentioned. What's interesting here is the real case we met: multiple .a files have the same .o files (which only contain declarations and are linked using whole-archive, I know this is a bit inappropriate), causing the linker to link all the duplicate .o files in the main program. Using llvm-dwp -e will result in an error in this case because of the duplicate skeletons. So it seems there is a possibility of such skeleton duplication...
Do we have any thoughts on this? Should we consider skipping the duplicate DWOs and then generating the DWP(better than nothing?), or packaging the duplicate DWOs in the DWP (what binutil dwp does), or continue with the current approach of reporting an error and then exiting? In the current llvm-dwp implementation, skipping the duplicate DWOs and generating a DWP seems to be more convenient. |
|
@clayborg Any thoughts on this? Maybe leave default an error, with an option to turn it into a warning and skipping? |
Ah, that's probably even easier to handle, then (compared to the duplicate DWO IDs that happen when building DWP files without In the Perhaps "same DWO ID with different contents" could be an error by default (downgradeable to a warning) - but means more expensive checking when duplicate DWO IDs are encountered, even if they turn out to be benign.
We should skip them - but we have to do so carefully. If we're doing incremental DWP building, then one DWP might have a duplicate CU DWO ID, but we need to verify that all the split type units that reference the sections shared with that CU DWO ID aren't needed either. (eg: .debug_str_offsets.dwo) Maybe we've done this correctly already, since this would be needed for type units that share sections - but I might've been lazy/cut corners and relied on the CU already pulling in the relevant sections. One way to test would be to see if llvm-dwp can handle a file with /only/ type units, and correctly pull in the section contents and emit it into the dwp. If that doesn't work, then there's probably work to be done to allow ignoring a CU and not ignoring a TU that references the same contents - or checking and failing in that case would be fine too/marginally better. |
4 participants
The current behavior of binutils dwp when encountering duplicate DWO inputs is to issue a warning and continue generating the DWP. like this：
However, llvm-dwp uses a map structure, which cannot store duplicate entries, so it throws an error and exits when faced with such scenarios.
Therefore, this PR attempts to skip the duplicate DWO inputs in such case, and generate the dwp.