-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Give a warning when no dwo files are provided #94336
Give a warning when no dwo files are provided #94336
Conversation
@llvm/pr-subscribers-debuginfo Author: Jinjie Huang (Labman-001) ChangesIn some scenarios based on the split-dwarf build process, the dwo file is not generated as expected. When the llvm-dwp tool is called to generate the .dwp file, it will exit without any warning. Full diff: https://github.com/llvm/llvm-project/pull/94336.diff 1 Files Affected:
diff --git a/llvm/tools/llvm-dwp/llvm-dwp.cpp b/llvm/tools/llvm-dwp/llvm-dwp.cpp
index 81556b3ad4bcb..f0edaeb07f0ba 100644
--- a/llvm/tools/llvm-dwp/llvm-dwp.cpp
+++ b/llvm/tools/llvm-dwp/llvm-dwp.cpp
@@ -188,8 +188,10 @@ int llvm_dwp_main(int argc, char **argv, const llvm::ToolContext &) {
std::make_move_iterator(DWOs->end()));
}
- if (DWOFilenames.empty())
+ if (DWOFilenames.empty()) {
+ WithColor::defaultWarningHandler(make_error<DWPError>("No dwo files found!\n"));
return 0;
+ }
std::string ErrorStr;
StringRef Context = "dwarf streamer init";
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Can you help to review this? @dwblaikie @ayermolo thanks |
Can you add a test? |
what does binutils dwp do in this case? (not that we should necessarily do the same thing) (& generally warnings are all lower case, no ending punctuation (no & yeah, test would be good |
Thank you for the review comments.
Since the scenario here targets the branch where the dwo path list is empty, there might not be a concept of "which" here. In regular scenarios, if a specific dwo file is missing, the 'write' function will report an error indicating that the corresponding file cannot be found.
In my Debian 10 system(Binutils dwp 2.31.1), it will just trigger a core dump in this case, while llvm-dwp just quits silently:
So it may be helpful to give a warning here?
Yes, I will add a targeted test. |
I think the right explanation/error message would be "executable does not contain any skeleton units" (technically correct, but probably doesn't mean much to users) or "executable file does not contain any references to dwo files"? "no dwo files found" sounds more like there weren't any dwo files on disk (and as a user I'd be asking "why didn't you tell me which dwo files you /tried/ to find/couldn't read") |
The message has been changed to "executable file does not contain any references to dwo files". |
please take a look @dwblaikie @ayermolo, thank you |
@@ -0,0 +1,218 @@ | |||
# Note: This file is compiled from the following code, for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit. I think it's now ##
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you simplify/reduce test. There is no need to rely on printf I think.
…:Labman-001/llvm-project into users/huangjinjie/llvm-dwp-empty-warning
Sorry, I might not have fully understood what is meant by "simplify/reduce test" here. Does it mean that I should consider not printing and checking the warning information? I'm not quite sure if it's okay to check without it |
The test itself:
Can you change it not to rely on stdio.h. Just returning 0 should be enough to repro correct? |
Would https://llvm.org/docs/TestingGuide.html#elaborated-tests help make the tests cleaner/easier to maintain? (or is there enough post-processing of the files, needs other tools, etc?) |
Thank you for the suggestion. I have changed it to compile a non-split-dwarf binary from the source code only, so that it does not contain any skeleton information. Originally, I introduced the .s file to modify the skeleton in the split-dwarf binary to trigger an error when the corresponding .dwo file could not be found (but this does not seem to be my target scenario?), so I have removed it. |
…injie/llvm-dwp-empty-warning
LGTM. Thanks for your patience. |
@dwblaikie Could you please help me review the test? Thank you. |
@@ -0,0 +1,9 @@ | |||
; RUN: rm -rf %t && mkdir -p %t && split-file %s %t && cd %t | |||
; RUN: clang -g -gdwarf-4 %t/non_split.c -o %t/non_split.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't run clang as part of llvm's tests (it's a layering violation - clang depends on llvm, not the other way around).
This test should contain something more basic - perhaps an empty or trivial assembly file that llvm-mc -g
could be used to compile to an object file and that might be enough for the situation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh didn't know that.
Why did the test pass? Thought there would be an error "clang not found" or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lit RUN lines basically boil down to a shell script, so it inherits the system PATH - but if you ran the tests on a machine that didn't have clang installed (and didn't build clang) the test would fail, so far as I understand.
Thank you, I have changed it to use llvm-mc to generate the binary for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is OK
Though, if you can - https://llvm.org/docs/TestingGuide.html#elaborated-tests - use this so you can include the source code in the test, and make it easy to regenerate, if needed?
Yes, the tool is useful. And source code has been included in the test. |
In some scenarios based on the split-dwarf build process, the dwo file is not generated as expected(That is to say, no dwo file path is stored in the binary). When the llvm-dwp tool is called to generate the .dwp file, it will exit without any warning. So, the plan is to prompt a warning to tell the user that the dwo file was not actually generated. <img width="699" alt="image" src="https://github.com/llvm/llvm-project/assets/150100070/5e5742f6-daad-450f-87e9-cb25449c3c7a">
In some scenarios based on the split-dwarf build process, the dwo file is not generated as expected(That is to say, no dwo file path is stored in the binary). When the llvm-dwp tool is called to generate the .dwp file, it will exit without any warning. So, the plan is to prompt a warning to tell the user that the dwo file was not actually generated. <img width="699" alt="image" src="https://github.com/llvm/llvm-project/assets/150100070/5e5742f6-daad-450f-87e9-cb25449c3c7a">
In some scenarios based on the split-dwarf build process, the dwo file is not generated as expected(That is to say, no dwo file path is stored in the binary). When the llvm-dwp tool is called to generate the .dwp file, it will exit without any warning.
So, the plan is to prompt a warning to tell the user that the dwo file was not actually generated.