[PATCH] D80203: [ObjectYAML][DWARF] Add DWARF entry in ELFYAML.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I think I'd like to see at least one DWARF section implemented to demonstrate that the concept works.



================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:26
 
+std::vector<std::string> DWARFYAML::Data::getNonEmptySectionNames() const {
+  std::vector<std::string> SecNames;
----------------
It can return the vector of `StringRef`s. Perhaps it can be called just "getSectionNames" for simplicity.


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:28
+  std::vector<std::string> SecNames;
+  if (DebugStrings.size() != 0)
+    SecNames.push_back("debug_str");
----------------
`DebugStrings.size() != 0` -> `!DebugStrings.empty()` here and below.


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:36
+    SecNames.push_back("debug_ranges");
+  if (PubNames.Entries.size() != 0)
+    SecNames.push_back("debug_pubnames");
----------------
Why this is `!DWARF.PubNames.Entries` and not `!DWARF.PubNames`?
(The same question for things around).


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:262
+  if (Doc.DWARF)
+    for (auto DebugSecName : Doc.DWARF->getNonEmptySectionNames())
+      ImplicitSections.push_back("." + DebugSecName);
----------------
Do not use `auto` when a type is not obvious.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:263
+    for (auto DebugSecName : Doc.DWARF->getNonEmptySectionNames())
+      ImplicitSections.push_back("." + DebugSecName);
   ImplicitSections.insert(ImplicitSections.end(), {".strtab", ".shstrtab"});
----------------
Doesn't this create a temporary `std::string` that is destroyed after the `push_back`
(and so `ImplicitSections` holds the dead string as a result)?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:396
     initStrtabSectionHeader(Header, SecName, DotDynstr, CBA, YAMLSec);
+  else if (SecName.startswith(".debug_") && !YAMLSec)
+    reportError(SecName + " section is not implemented");
----------------
Why do you need `&& !YAMLSec`?


================
Comment at: llvm/test/tools/yaml2obj/ELF/unimplemented-debug-sections.test:1
+## This file contains tests for unimplemented debug sections.
+
----------------
`unimplemented-debug-sections.test` is a name that assumes this file will be renamed/removed later.
I do not think it is common to do such things. It could be just "debug-sections.test".

Perhaps, we should create a "ELF/DWARF" subfolder and create a separate test for each section. I am not sure, lets hear from others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80203