[PATCH] D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info.

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked 32 inline comments as done.
avl added inline comments.


================
Comment at: lld/ELF/DWARF.cpp:35
+      sectionNames.push_back({sec->name, true});
+      sectionAmountMap[sec->name]++;
+
----------------
grimar wrote:
> 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).
There should be three states here(seen, not seen, seen more than once). DWARFFormValue::dumpAddressSection() needs this to properly dump die address. If name seen more than once it prints section index.


```
  // Print section index if name is not unique.
  if (!SecRef.IsNameUnique)
    OS << format(" [%" PRIu64 "]", SectionIndex);
```


================
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)
----------------
grimar wrote:
> 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.
right. It assumes that relocations are sorted by r_offset.
It is sorted in scanRelocs(). But I missed that it is sorted not for all types of sections.
Would fix it , Thanks!


================
Comment at: lld/ELF/DWARF.h:84
 
+  ArrayRef<llvm::SectionName> getSectionNames() const override {
+    return sectionNames;
----------------
grimar wrote:
> Where it is used? I\ve not found it in this patch.
It is used at DWARFFormValue::dumpAddressSection(). When address is dumped, DWARFObject could be asked for section names(LLDDwarfObj is a child of DWARFObject). It could happen when Verbose mode is set for DWARFLinker.


================
Comment at: lld/ELF/DWARF.h:122
+
+  std::vector<llvm::SectionName> sectionNames;
 };
----------------
grimar wrote:
> Is it used somewhere outside the `template <class ELFT> LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *obj)` ?
> Can it be a local variable?
Yes, it could be used outside of LLDDwarfObj() constructor. When DWARFObject::getSectionNames() is called the sectionNames is used.
One such place is DWARFFormValue::dumpAddressSection().


================
Comment at: lld/ELF/InputSection.h:127
 
+  bool isDebug = false;
+
----------------
grimar wrote:
> Why it is needed to store this bit? The code calls `isDebugSection`, is there something wrong with the current approach?
To not do string compare in every usage. i.e. that bit is set in the constructor and at every usage place is checked. Otherwise, it would be necessary to compare strings at every usage place.


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:65
+      curIdx++;
+    }
+
----------------
grimar wrote:
> 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).
The idea was to not put error data into sectionIndexesMap. sec is indeed checked later for nullptr.
But it looks like not putting wrong data is better approach than relying on following checks. 


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:143
+            // Apply relocation.
+            assert(rel.offset - baseOffset < data.size());
+            getTarget()->relocate(
----------------
grimar wrote:
> Why this assert is useful? What scenario did you have in mind?
To check that relocation passed to the handler is from baseOffset, baseOffset + data.size() range.
i.e. to check that enumerateRelocations works correctly and pass proper relocations to the handler.


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:256
+          objectFiles[i]->getName(), obj->getDwarf()->getContext(),
+          addresssMapForLinking[i].get(), emptyWarnings);
+
----------------
grimar wrote:
> `emptyWarnings` is unused. Is it OK? You can inline it if so.
it is used only in std::make_unique<DwarfFile>. 

If emptyWarnings is used inline then it`s constructor would be called for each ObjectFile. Probably we could rely on compiler here...


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:266
+
+  // Create sections map.
+  DenseMap<StringRef, StringRef> sectionsMap;
----------------
grimar wrote:
> Map what to what?
Would add comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74169/new/

https://reviews.llvm.org/D74169