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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the patch. In general, it looks like the right approach.



================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:26
 
+std::vector<std::string> DWARFYAML::Data::getNonEmptySectionNames() const {
+  std::vector<std::string> SecNames;
----------------
grimar wrote:
> It can return the vector of `StringRef`s. Perhaps it can be called just "getSectionNames" for simplicity.
DWARFYAML is already used in Mach-O, so we might need to be careful with code like this that is ELF-specific. If we can add a property to the class that says what file format it is, that'll allow us to change the function name to `getSectionNames` as suggested, and provide a way to make this sort of function generic in the future, should the need arise.

Actually, I might suggest `getUsedSectionNames`, since it doesn't return all section names all the time.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:747
+
+  if (RawSec && (RawSec->Content || RawSec->Size)) {
+    SHeader.sh_size = writeContent(OS, RawSec->Content, RawSec->Size);
----------------
I don't know if it's needed in this patch, but we might want an error if a user tries to specify both Content/Size and DWARF data for a DWARF section.

An alternative, but I don't think it is necessary without a use-case, would be to do something like append the two.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:761
+  } else
+    reportError("the content of the " + Name + " section should be specified");
+
----------------
Maybe this can just result in an empty section rather than an error?


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:3
+
+## a) TODO: Generate the .debug_info section from the "DWARF" entry.
+# RUN: not yaml2obj --docnum=1 %s -o %t1.o 2>&1 | FileCheck %s --check-prefix="NOIMPL"
----------------
You can get rid of the "a) ". On its own, it doesn't really make sense.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:16-21
+    - Length:
+        TotalLength: 0x0
+      Version:       0
+      AbbrOffset:    0x0
+      AddrSize:      0x0
+      Entries:
----------------
Maybe worth just omitting the body here, since it isn't actually used, if I'm right.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:22
+      Entries:
+...
----------------
Here and in all other cases, you don't need the `...`.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:1
+## Test that yaml2obj emits .debug_str section.
+
----------------
In addition to the test cases in this file already, you need the following, I think:

a) Size is explicitly specified.
b) Both Size and DWARF->debug_str are specified
c) Both Content and DWARF->debug_str are specified
d) All properties that can be overridden by the section header (e.g. EntSize, Flags etc) when DWARF->debug_str is used.
e) The default values of Flags, EntSize etc when DWARF->debug_str is not used.
f) All properties that can be overridden by the section header (e.g. EntSize, Flags etc) when DWARF->debug_str is not used.
g) Something show that `assignSectionAddresses` is called.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:5
+# RUN: yaml2obj --docnum=1 %s -o %t1.o
+# RUN: llvm-dwarfdump --debug-str %t1.o | FileCheck %s
+
----------------
I'd like to hear other people's opinions on this:

Presumably one of the reasons we are adding this support is because in the future, we'd like to write llvm-dwarfdump tests by using yaml2obj, but if we do that, we end up in a bit of a circular dependency, as we are using llvm-dwarfdump to test yaml2obj. That could result in a bug in the common library that results in both appearing to produce the right data, but actually it not being correct.

For .debug_str, it would be trivial to use other options, like llvm-readobj's --section-data or --string-dump options. I don't think this would work as well for other sections though, so maybe the circular dependency is okay? Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80203