Details
- Reviewers
dblaikie ruiu echristo clayborg • rafael aprantl - Commits
- rGec02b8d4c03e: [ELF] - Partial support of --gdb-index command line option (Part 3).
rLLD289810: [ELF] - Partial support of --gdb-index command line option (Part 3).
rL289810: [ELF] - Partial support of --gdb-index command line option (Part 3).
Diff Detail
- Repository
- rL LLVM
Event Timeline
Seems like the GdbIndex isn't specific to ELF. I know the file started off here and that some of the template classes you use are using ELF defines, but they don't really need to be in the ELF plug-in.
Would we want to move this functionality to the lib/DebugInfo/Dwarf/DWARFGdbIndex.cpp file so that other file formats like mach-o and COFF could create the gdb index section and be able to emit it? This code accesses existing DWARF sections and then creates the .gdb-index section contents.
ELF/GdbIndex.cpp | ||
---|---|---|
63 ↗ | (On Diff #80402) | Since we "do" not plan |
ELF/SyntheticSections.cpp | ||
1441 ↗ | (On Diff #80402) | use std::tie here with appropriate var names since you did it below? |
1442 ↗ | (On Diff #80402) | I am guessing we can guarantee that the StringRef is NULL terminated here since the string probably originated from a string table. |
1522 ↗ | (On Diff #80402) | use std::tie? |
If so I am not sure what is better ?
- Do that before landing this+D25821.
- Continue with landing above for ELF, complete and test functionality and then move it to lib/Debuginfo/Dwarf/* ?.
I would also create separate file for this like lib/DebugInfo/Dwarf/DWARFGdbIndexBuilder.cpp as DWARFGdbIndex.cpp is a parser and probably
better not to mix them.
Not sure either. Probably fine to land it here with this change then migrate it.
I would also create separate file for this like lib/DebugInfo/Dwarf/DWARFGdbIndexBuilder.cpp as DWARFGdbIndex.cpp is a parser and probably
better not to mix them.
If it is the same stuff, I don't see a problem with the class being able to parse and generate the gdb index. You would probably share a lot of the same data structures and it would be a shame to duplicate those.
ELF/GdbIndex.cpp | ||
---|---|---|
100–101 ↗ | (On Diff #80402) | What happens if there is no pubnames or pubtypes? Do we want a separate path that actually manually goes through the DWARF and generates the correct info when these sections are missing? Does the GDB index really live only on the pubnames and pubtypes? How useful is the GDB index if it doesn't include static functions and private types? |
Ah, I missed the extra "gnu" in the pubnames and pubtypes. If these sections don't exist, can we generate them on the fly using common code and then use the generated data?
ELF/SyntheticSections.cpp | ||
---|---|---|
1441 ↗ | (On Diff #80402) | I am not sure what you mean, sorry. Do you meant next form ? for (auto& p : someInitializingFunction()) { std::tie(a, b) = p; // do stuff; } If so I am probably missing for what. |
1442 ↗ | (On Diff #80402) | Yes, and hash() relies on that. Do you want to change hash() signature ? |
See inlined comments.
ELF/GdbIndex.cpp | ||
---|---|---|
105–118 ↗ | (On Diff #80402) | Might be worth putting this pubnames/pubtypes parser into the llvm/lib/DebugInfo/DWARF. We don't currently have a parser for this there. The more we duplicate code that is manually parsing DWARF sections outside of the DWARF parser the more fixes we need to make if the format gets updated. |
ELF/SyntheticSections.cpp | ||
1417 ↗ | (On Diff #80402) | Use StringRef here instead of "const char *"? |
1420 ↗ | (On Diff #80402) | Use the StringRef iteration here? |
1429 ↗ | (On Diff #80402) | Maybe add a: if (Finalized) return; So that this code works if asserts are not enabled? I would be great to reduce the amounts of places that use assert unconditionally in the llvm codebase. |
1441 ↗ | (On Diff #80402) | Never mind, I was thinking you can use std::tie in the for loop: for (std::tie(Name, Type) : NamesAndTypes) { but you can't. |
1442 ↗ | (On Diff #80402) | Might be safer to change the hash() signature if the only way we use it is with an llvm::StringRef. |
1522 ↗ | (On Diff #80402) | Never mind, can't use std::tie in for loop |
- Addressed review comments.
- Make StringPool to be Kind of llvm::StringTableBuilder::ELF, that is correct I think and after that output for testcase is equal to gold.
ELF/GdbIndex.cpp | ||
---|---|---|
63 ↗ | (On Diff #80402) | Done. |
105–118 ↗ | (On Diff #80402) | I wonder if that can be done in the following patch then ? |
ELF/SyntheticSections.cpp | ||
1417 ↗ | (On Diff #80402) | Done. |
1420 ↗ | (On Diff #80402) | Done. |
1429 ↗ | (On Diff #80402) | I removed this assert. readDwarf() is now called only from parseDebugSections, which is already protected by if (Finalized) return; parseDebugSections(); So actually it was bit excessive assert. If for some reason this method will be called when Finalized, we should catch it in testcase anyways I think. |
1442 ↗ | (On Diff #80402) | Done. |
So it looks like it is down to wether we need to actually do the pubnames parsing stuff in lib/DebugInfo/DWARF or not. I'll let other make that call.
ELF/GdbIndex.cpp | ||
---|---|---|
118 ↗ | (On Diff #80894) | I'll let others comment on what is right to do as I am just starting out reviewing things in LLVM and don't know all the ropes. David? |
ELF/GdbIndex.cpp | ||
---|---|---|
118 ↗ | (On Diff #80894) | Yeah, we go back and forth on stuff like this - in this case I think the code is short (granted, that also means the work to do it ahead of time isn't that big either) & George has been pretty responsive/quick on feedback/changes like this, so I'm OK with it going in after this patch. |
Sorry, I am pinging this again because I am just not sure who is that person for approving this then ? I just see no objections from reviewers currently and looks all comments are addressed. I am also will be happy to prepare a discussed patch for LLVM parsers right after this one landed.
Landing this one + D25821 looks allows us to include gdb-index functionality into upcoming release planned early in 2017 afaik.
I have a few more stylistic issues, but after fixing that, feel free to commit.
ELF/GdbIndex.cpp | ||
---|---|---|
157 ↗ | (On Diff #80894) | Coding guidelines want this to be a doxygen comment on the declaration in the header. |
ELF/SyntheticSections.cpp | ||
1416 ↗ | (On Diff #80894) | same here. |
Thank you for review Adrian !
I'll address all comments for this and D25821 with little general change:
we do not use Doxygen comments in tools/lld/ELF/* yet.
So I'll add regular ones for each place where it was requested. I am not sure why we do not use
Doxygen, but that moment already was discussed for these gdb_index patches before, so I hope it is still fine.
r289810
ELF/GdbIndex.cpp | ||
---|---|---|
157 ↗ | (On Diff #80894) | In LLD we often use the links in comments. Though in that case code itself just implements the formulas described in documentation. I replaced the link with short sentence about how it works in general. |
ELF/SyntheticSections.cpp | ||
1416 ↗ | (On Diff #80894) | I removed the link and added a short comment. We still have link in the file header, this comment refers to it. |
Now that's a really strange deviation from the LLVM coding guidelines! Why would we not want to use Doxygen comments there? Can someone explain the motivation behind this?
- adrian