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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few more comments. They are mostly to give a feedback about the code style, I have not look deep into how it works yet.



================
Comment at: lld/ELF/DWARF.cpp:29
 template <class ELFT> LLDDwarfObj<ELFT>::LLDDwarfObj(ObjFile<ELFT> *obj) {
+  StringMap<unsigned> sectionAmountMap;
   for (InputSectionBase *sec : obj->getSections()) {
----------------
I'd call it `sectionNumber`, having `Map` in the name here is not useful.


================
Comment at: lld/ELF/DWARF.cpp:32
     if (!sec)
-      continue;
-
-    if (LLDDWARFSection *m =
-            StringSwitch<LLDDWARFSection *>(sec->name)
-                .Case(".debug_addr", &addrSection)
-                .Case(".debug_gnu_pubnames", &gnuPubnamesSection)
-                .Case(".debug_gnu_pubtypes", &gnuPubtypesSection)
-                .Case(".debug_info", &infoSection)
-                .Case(".debug_ranges", &rangesSection)
-                .Case(".debug_rnglists", &rnglistsSection)
-                .Case(".debug_str_offsets", &strOffsetsSection)
-                .Case(".debug_line", &lineSection)
-                .Default(nullptr)) {
-      m->Data = toStringRef(sec->data());
-      m->sec = sec;
-      continue;
-    }
+      sectionNames.push_back({"", true});
+    else {
----------------
To avoid nesting, you can use an early continue:

```
if (!sec) {
  sectionNames.push_back({"", true});
  continue;
}
...
```


================
Comment at: lld/ELF/DWARF.cpp:35
+      sectionNames.push_back({sec->name, true});
+      sectionAmountMap[sec->name]++;
+
----------------
avl wrote:
> 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);
> ```
Ah, sorry, I've misread the code slightly, I did not notice the codition is `sectionAmountMap[sec.Name] > 1` and not `sectionAmountMap[sec.Name] > 0`.



================
Comment at: lld/ELF/DWARF.cpp:63
+  for (SectionName &sec : sectionNames)
+    if (sec.Name.size() > 0 && sectionAmountMap[sec.Name] > 1)
+      sec.IsNameUnique = false;
----------------
grimar wrote:
> `sec.Name.size() > 0` -> `!sec.empty()`
It is better to avoid using `operator[]` in situations like this, because
it is not `const`, when a `sec.Name` is not found it creates a new map entry. This is not what you want here,
you can use `lookup()`.


================
Comment at: lld/ELF/InputSection.h:127
 
+  bool isDebug = false;
+
----------------
avl wrote:
> 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.
Yeah, but did you measure numbers, i.e. is there a visible benefit? The code in this diff still calls `isDebugSection` in many places. It looks like a premature optimization to me and/or something that could probably be moved to another patch (which could change all call sites at once).





================
Comment at: lld/ELF/LLDDwarfLinker.cpp:206
+      OutputFileType::Object, stream,
+      [](StringRef input) -> StringRef { return input; }, false,
+      [&](const Twine &err, StringRef context, const DWARFDie *) {
----------------
I think the returned type of this lambda can be deduced:

`[](StringRef S) { return S; }`


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:218
+          warn(warning);
+      });
+  if (!outStreamer.init(Triple(sys::getProcessTriple()))) {
----------------
This 2 lambdas looks similar. Also you have 2 more below.
It seems you could do something like the following?


```
auto ReportWarn = [] [&](const Twine &msg, StringRef ctx, const DWARFDie *) {
if (!context.empty())
  warn(ctx+ ": " + msg);
else
  warn(msg);
};

DwarfStreamer outStreamer(..., ReportWarn, ReportWarn);

```


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:256
+          objectFiles[i]->getName(), obj->getDwarf()->getContext(),
+          addresssMapForLinking[i].get(), emptyWarnings);
+
----------------
avl wrote:
> 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...
I'd inline to make the code shorter/cleaner. It doesn't look as hot path, so no need to optimize it until a benefit is proven.


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

https://reviews.llvm.org/D74169