[PATCH] D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 6 04:50:14 PDT 2020
grimar added a comment.
I've put a few first general comments and suggestions inline.
side note: I've noticed you have a build time 75m15s, what looks to me too much for your configuration. Do you know about "-DLLVM_OPTIMIZED_TABLEGEN=1"? It should give an instant significant boost.
================
Comment at: lld/ELF/DWARF.cpp:35
+ sectionNames.push_back({sec->name, true});
+ sectionAmountMap[sec->name]++;
+
----------------
Doesn't seem you really need to know the number of sections?
Hence you can use `DenseSet<StringRef> seen;` (btw, 'seen' is the common name for sets used in a few similar situations in LLD code).
================
Comment at: lld/ELF/DWARF.cpp:63
+ for (SectionName &sec : sectionNames)
+ if (sec.Name.size() > 0 && sectionAmountMap[sec.Name] > 1)
+ sec.IsNameUnique = false;
----------------
`sec.Name.size() > 0` -> `!sec.empty()`
================
Comment at: lld/ELF/DWARF.cpp:161
+ relHandler) const {
+ // check if there are no more relocations.
+ if (nextReloc >= rels.size())
----------------
Comments should be started from an uppercase letter: check -> Check.
(applies here and for other comments)
================
Comment at: lld/ELF/DWARF.cpp:166
+ // skip relocations, started from nextReloc until relocPos < startPos.
+ uint64_t relocPos = rels[nextReloc].r_offset;
+ while (relocPos < startPos && nextReloc < rels.size() - 1)
----------------
Seems this assumes that relocations are sorted by r_offset? This is probably a wrong assumpting.
We have the code, for example, which uses the same thing (and perhaps could be reused somehow, not sure):
```
// Returns the index of the first relocation that points to a region between
// Begin and Begin+Size.
template <class IntTy, class RelTy>
static unsigned getReloc(IntTy begin, IntTy size, const ArrayRef<RelTy> &rels,
unsigned &relocI) {
// Start search from RelocI for fast access. That works because the
// relocations are sorted in .eh_frame.
for (unsigned n = rels.size(); relocI < n; ++relocI) {
const RelTy &rel = rels[relocI];
if (rel.r_offset < begin)
continue;
```
But it says that relocations are sorted in .eh_frame and I believe it is true,
but generally there is no requirement in ELF specification about that relocations must be sorted by offset I think.
================
Comment at: lld/ELF/DWARF.cpp:183
+ if (referencedSection && referencedSection->isLive()) {
+ Relocation reloc = {R_ABS, rel.getType(config->isMips64EL), rel.r_offset,
+ getAddend<ELFT>(rel), &file->getRelocTargetSym(rel)};
----------------
Why exactly `R_ABS`/is it important? (Perhaps needs a comment).
================
Comment at: lld/ELF/DWARF.h:84
+ ArrayRef<llvm::SectionName> getSectionNames() const override {
+ return sectionNames;
----------------
Where it is used? I\ve not found it in this patch.
================
Comment at: lld/ELF/DWARF.h:122
+
+ std::vector<llvm::SectionName> sectionNames;
};
----------------
Is it used somewhere outside the `template <class ELFT> LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *obj)` ?
Can it be a local variable?
================
Comment at: lld/ELF/Driver.cpp:347
+ error("--gc-debuginfo may not be used without --gc-sections");
+ }
+
----------------
You do not need to have curly bracers around a single code line (the rule to look around to see the style often works good).
================
Comment at: lld/ELF/InputSection.h:127
+ bool isDebug = false;
+
----------------
Why it is needed to store this bit? The code calls `isDebugSection`, is there something wrong with the current approach?
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:58
+
+ // Create map InputSection -> SectionIndex.
+ DenseMap<InputSectionBase *, uint64_t> sectionIndexesMap;
----------------
It should be `InputSectionBase`.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:59
+ // Create map InputSection -> SectionIndex.
+ DenseMap<InputSectionBase *, uint64_t> sectionIndexesMap;
+ uint64_t curIdx = 0;
----------------
I'd suggest something shorter, like `secToIndex`
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:64
+ sectionIndexesMap[sec] = curIdx;
+ curIdx++;
+ }
----------------
Prefer ++foo over foo++.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:65
+ curIdx++;
+ }
+
----------------
btw, what is so wrong with the following version?
```
for (InputSectionBase *sec : objFile->getSections())
sectionIndexesMap[sec] = curIdx++;
```
(i.e. I probably see no issue with the case when `sec == nullptr`, as the code below handes this case anyways).
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:74
+
+ if (sec && sec->isLive() && sectionIndexesMap.count(sec)) {
+ hasValidRelocations = true;
----------------
Continue early?
```
if (!sec || !sec->isLive() || !sectionIndexesMap.count(sec))
continue;
```
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:103
+
+ bool result = false;
+
----------------
We often call such things `ret`. Up to you.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:140
+ uint64_t Value = Addr.inputSectionAddress + Addr.objectAddress;
+ Value += rel.addend;
+
----------------
This could be a single line, no need toadd an addend later.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:143
+ // Apply relocation.
+ assert(rel.offset - baseOffset < data.size());
+ getTarget()->relocate(
----------------
Why this assert is useful? What scenario did you have in mind?
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:145
+ getTarget()->relocate(
+ (uint8_t *)(data.data() + rel.offset - baseOffset), rel, Value);
+ result = true;
----------------
I think you can just inline `Value`.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:193
+template <class ELFT> void linkDebugInfo(DebugDataBits &outDebugInfoBytes) {
+
+ // Calculate estimated size of resulting debug info.
----------------
An excessive empty line.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:194
+
+ // Calculate estimated size of resulting debug info.
+ uint64_t estimatedSize = 0;
----------------
This comment needs expanding I think. "Estimation" is something that usually sounds tricky and requires an explanation of your algorithm implemented.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:196
+ uint64_t estimatedSize = 0;
+ for (const auto sec : inputSections) {
+ if (sec->isDebug && sec->isLive())
----------------
Don't use `auto` when the type is not obvious.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:199
+ estimatedSize += sec->getSize() * 0.5f;
+ }
+ outDebugInfoBytes.reserve(estimatedSize);
----------------
No need to have curly bracers here.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:230
+ [&](const Twine &err, StringRef context, const DWARFDie *) {
+ if (context.size() > 0)
+ warn(context + ": " + err);
----------------
`foo.size() > 0` -> `!foo.empty()` when `foo` is `StringRef`
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:256
+ objectFiles[i]->getName(), obj->getDwarf()->getContext(),
+ addresssMapForLinking[i].get(), emptyWarnings);
+
----------------
`emptyWarnings` is unused. Is it OK? You can inline it if so.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:266
+
+ // Create sections map.
+ DenseMap<StringRef, StringRef> sectionsMap;
----------------
Map what to what?
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:271
+ for (const SectionRef &Sec : MemFile->sections()) {
+ if (Expected<StringRef> name = Sec.getName()) {
+ if (!isDebugSection(*name))
----------------
The way you work with `Expected<>` is incorrect. It might fail on error with `LLVM_ENABLE_ABI_BREAKING_CHECKS=1`.
You need to handle the error or to ignore it explicitly. E.g.:
```
Expected<GlobPattern> pat = GlobPattern::create(arg);
if (!pat) {
error("--undefined-glob: " + toString(pat.takeError()));
return;
}
```
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:283
+ // Set linked debug info to corresponding output section.
+ for (auto &sec : outputSections) {
+ if (!isDebugSection(sec->name))
----------------
Don't use `auto` here.
================
Comment at: lld/ELF/LLDDwarfLinker.cpp:292
+ else
+ sec->setDebugData(StringRef());
+ }
----------------
`StringRef()` -> `""`
But I think it can be just:
```
for (auto &sec : outputSections)
if (isDebugSection(sec->name))
sec->setDebugData(sectionsMap.lookup(sec->name));
```
================
Comment at: lld/ELF/Options.td:188
+ "Enable garbage collection of unused debug information",
+ "Disable garbage collection of unused debug information (default)">;
+
----------------
When you add an LLD option, you should also update the documentation (`lld/docs/ld.lld.1`)
================
Comment at: lld/ELF/OutputSections.cpp:314
+ if (hasDebugData()) {
+ memcpy(buf, debugData.data(), debugData.size());
----------------
Needs a comment.
================
Comment at: lld/ELF/OutputSections.h:129
+ StringRef debugData;
+ bool debugDataNonEmpty = false;
};
----------------
Did you consider to use `llvm::Optional<StringRef>`?
================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:456
+ OutFile);
+ else
+ return Streamer->finish(), true;
----------------
You do not need to use `else` after return:
```
if (Map.getTriple().isOSDarwin() && !Map.getBinaryPath().empty() &&
Options.FileType == OutputFileType::Object)
return MachOUtils::generateDsymCompanion(
Map, Options.Translator, *Streamer->getAsmPrinter().OutStreamer,
OutFile);
return Streamer->finish(), true;
```
================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:457
+ else
+ return Streamer->finish(), true;
}
----------------
Something is wrong with this line. `, true` is a part of something else it seems.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74169/new/
https://reviews.llvm.org/D74169
More information about the llvm-commits
mailing list