-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lld] Warn for missing needed sections by --gdb-index #166626
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
base: main
Are you sure you want to change the base?
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 notified. 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 permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. 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-lld @llvm/pr-subscribers-lld-elf Author: None (aury6623) ChangesIssue a warning when lld is passed Full diff: https://github.com/llvm/llvm-project/pull/166626.diff 1 Files Affected:
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index a4150ebfa1653..77487de805960 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -3566,6 +3566,16 @@ std::unique_ptr<GdbIndexSection> GdbIndexSection::create(Ctx &ctx) {
DWARFContext dwarf(std::make_unique<LLDDwarfObj<ELFT>>(file));
auto &dobj = static_cast<const LLDDwarfObj<ELFT> &>(dwarf.getDWARFObj());
+ if (dobj.getGnuPubnamesSection().sec == nullptr &&
+ dobj.getGnuPubtypesSection().sec == nullptr) {
+ Warn(ctx)
+ << files[i]
+ << ": file contains debug info but is missing .debug_gnu_pubnames or "
+ ".debug_gnu_pubtypes sections required by --gdb-index. Compile "
+ "the file using -gsplit-dwarf or -ggnu-pubnames to generate these "
+ "sections.";
+ }
+
// If the are multiple compile units .debug_info (very rare ld -r --unique),
// this only picks the last one. Other address ranges are lost.
chunks[i].sec = dobj.getInfoSection();
|
|
This edit is a solution for Issue #156732 I posted a few months ago. Having this warning would have saved me a day or two of debugging! In particular, when only some files are compiled with Now knowing what the issue is, I found this forum post where someone is running into a similar issue: which suggests that other people may find this warning to be helpful too. This is my first time contributing to llvm-project. I know adding new tests for a change is normally expected, but I'm not sure what test changes (if any) are expected for only an addition of a warning message, or where that test should be added. Please advise. Thanks! |
lenary
left a comment
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.
Please can you add a test?
Sure, happy to! This is my first contribution to llvm-project, so I'm not quite familiar with the testing infrastructure. I've read this page: If you could let me know where a test for an edit like this should go, that would be helpful! Thanks! |
|
Yeah, that guide is good at the higher level, but I can see why you need more guidance :) I think what we'd want is an assembly + linker test. This would consist of 1 (maybe 2) assembly files, which are assembled with The core assertion behind the test would be to invoke LLD, and then run It's fine to take a simple C example, and compile it on your own using |
|
@lenary Thanks for the help! I added a test. I also updated an existing test which I realized was failing because it was expecting no output from ld.lld but was now getting the warning. I wasn't sure if adding those the missing sections to that test could impact the test's intent, so I figured the best thing to do is to just make the test ignore that particular warning by stripping it out with sed. |
|
Thanks for adding the test. I think using I think it would be fine to update that test to also check for the warning, but I'm not entirely sure. |
MaskRay
left a comment
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.
Thanks for the motivation to improve things but I believe this will certainly break some corporate users. Left a comment on #34168 .
While warnings are generally acceptable in Linux distros and OSS environments, corporate users and large projects using custom toolchains often upgrade warnings to errors by specifying --fatal-warnings. Reporting a warning should probably be deferred to a lint tool. It's also quite difficult to ensure that all object files have .debug_gnu_pubnames - when you have prebuilt object files.
I was wondering that too, but I checked to see if other tests use sed, and found lots of them. Here's one in the same directory which only requires x86 and uses sed: llvm-project/lld/test/ELF/debug-names-different-aug-string.s Lines 1 to 3 in 1c19645
and here's one from llvm-objdump which specifically requires windows but still uses sed: llvm-project/llvm/test/tools/llvm-objdump/X86/source-interleave-prefix-windows.test Lines 2 to 7 in 39fbec0
So it seems fine to me.
For what it's worth, I am a corporate user, working towards enabling fatal warnings on our compilations :) That said, I understand a warning could cause trouble here, so what about making it a remark/info message instead of a warning? I just want the tool to provide some feedback that it's not doing what the user probably intended.
|
|
@MaskRay what do you think about something opt-in like I think this would solve the usecase of trying to improve a build enough use the gdb-index, without making life worse for people who don't care to make that improvement. @aury6623 ok, sed seems justified, it seems I'm not quite aligned with the status quo. I don't know the next steps forward for the issue you're facing with trying to get gdb working with dwarf 5 and C macros. It might be that you were just missing |
I'm not sure anyone would ever know to look for flag like that, if it's opt-in. I certainly wouldn't have known.
Right, I had forgotten the exact problem that I encountered when trying dwarf-5 with gdb. The problem is that gdb only supports You can see in the lastest gdb sources right now, it ignores That's the message I see when I try it myself. Here's the gdb bug ticket for supporting For my project's 600 MB debug binary (without an index), using I'd love to use standardized dwarf-5 features, but clearly they're not quite ready yet and I don't think we should dismiss .gdb_index as near end of life. |
|
If we have to add a diagnostic, an option like |
Issue a warning when lld is passed
--gdb-indexand some object files have general debug info but not the specific debug sections generated by-ggnu-pubnamesor-gsplit-dwarf. Missing those sections on all inputs leads to an empty index, which gdb ignores. Missing those sections on only some sources leads to a partially complete index, which confuses gdb.Addresses #156732 and #34168