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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks. I've mostly got test comments for you.



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:269
+    for (StringRef DebugSecName : Doc.DWARF->getUsedSectionNames()) {
+      std::string SecName = Twine("." + DebugSecName).str();
+      ImplicitSections.push_back(StringRef(SecName).copy(StringAlloc));
----------------
If I'm not mistaken, `StringRef + const char *` results in a `Twine`, so you probably can just do `("." + DebugSecName).str()` without the explicit `Twine`.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:787
+  else
+    llvm_unreachable("initDWARFSectionHeader() failed");
+
----------------
Would this be better if it were more explicit? Something like: "initDWARFSectionHeader() should only be called if something describes the section".


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:16
+
+# RUN: llvm-readelf --section-headers %t1.o | FileCheck %s --check-prefix=READELF
+
----------------
Let's be more preceise with our prefix names, e.g. "SHDRS" here, "DWARF-DEFAULT" above.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:36
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
+# RUN: llvm-readelf --string-dump=.debug_str %t2.o | FileCheck %s
+
----------------
I think you should get rid of the Flags property below and show the section header details here. That way, you'll show what the default flag properties are if the DWARF tag is not specified.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:53
+# RUN: yaml2obj --docnum=3 %s -o %t3.o
+# RUN: llvm-readelf --section-headers %t3.o | FileCheck %s --check-prefix=SIZE
+
----------------
You probably want to show that the content of the section is all zero too. You can use the `--hex-dump` switch if needed for that, or the `od` tool.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:109
+
+## f) Test that all the properties can be overriden by the section header when
+## the "debug_str" entry is used.
----------------
overriden -> overridden

Same elsewhere.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:132-133
+    AddressAlign: 2           # 1 by default.
+DWARF:
+  debug_str:
+    - a
----------------
I think another set of interesting tests would be when the `DWARF` tag exists, but no `debug_str` entry is specified. I also think it would be interesting to show that no .debug_str section is generated if the `DWARF` tag is used, but no `debug_str` entry is specified within it or the `Sections` tag.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:159-160
+
+## h) Check the default values of sh_entsize, sh_flags, sh_info and sh_addralign when
+## the "debug_str" entry doesn't exist.
+
----------------
I think you can merge this test case with case b) above (if you drop the flags bit).


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:178
+
+## i) Set the address for the .debug_str section when the "debug_str" entry exists.
+
----------------
Fold this in with the similar test cases for sh_entsize etc.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str.yaml:200
+
+## j) Set the address for the .debug_str section when the "debug_str" entry doesn't exist.
+
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80203