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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:788-789
+    reportError("section '" + Name +
+                "' can only be initialized from the 'DWARF' entry or "
+                "the raw content section");
+
----------------
Probably I'd say "can only be initialized via the 'DWARF' entry or a section's 'Content' or 'Size' fields"


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:1
+## Test that yaml2obj emits .debug_str section.
+
----------------
jhenderson wrote:
> 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.
>  g) Something show that assignSectionAddresses is called.

I'm not sure I see which test case demonstrates this?


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:138
+    EntSize:      2                  # 1 by default.
+    Info:         1                  # 0 by default.
+    AddressAlign: 2                  # 1 by default.
----------------
We probably want a "Link:" reference too (e.g. "Link: .symtab").


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:148
+
+## g) Test that all the properties can be overridden by the section header when
+## the "debug_str" entry doesn't exist.
----------------
It might make slightly more sense for f) and g) to be switched around (have the "base" case before the ones which include more data).


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:185-186
+Sections:
+  - Name: .debug_str
+    Type: SHT_DYNAMIC
+
----------------
My instinct says that this should only be an error if we start using something like "Entries:" (for dynamic sections) etc. Otherwise, we cannot test the case where the type of a debug section is one of those kinds of sections (dynamic, relocation etc). Thus the cases below should not be errors:
```
  - Name: .debug_str
    Type: SHT_DYNAMIC
  - Name: .debug_str
    Type: SHT_DYNAMIC
    Size: 42
  - Name: .debug_str
    Type: SHT_DYNAMIC
    Content: '006f00'
```
whereas the following would be:
```
  - Name: .debug_str
    Type: SHT_DYNAMIC
    Entries:
     - Tag: DT_NULL
       Value: 0
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80203