[PATCH] D69895: [yaml2obj/obj2yaml] - Add support for SHT_LLVM_LINKER_OPTIONS sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I assume that this is in preparation for updating a test that uses this section?



================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1353-1354
+          dyn_cast<ELFYAML::LinkerOptionsSection>(Section.get())) {
+    if (!Sec->Options && !Sec->Content)
+      return "either \"Options\" or \"Content\" must be specified";
+    if (Sec->Options && Sec->Content)
----------------
Hmm... I think it should be okay to not have to specify anything to create an empty such section (I'd actually support this for other sections too, thinking about it, where an empty section is legitimate).


================
Comment at: llvm/test/tools/obj2yaml/linker-options.yaml:1
+## Check how obj2yaml produces SHT_LLVM_LINKER_OPTIONS sections descriptions.
+
----------------
sections -> section


================
Comment at: llvm/test/tools/obj2yaml/linker-options.yaml:35
+
+## Check we dump corrupted sections using "Content" key.
+
----------------
corrupted -> corrupt
using the "Content"


================
Comment at: llvm/test/tools/obj2yaml/linker-options.yaml:38
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: obj2yaml %t2 | FileCheck %s --check-prefix=CORRUPTED
+
----------------
CORRUPTED -> CORRUPT


================
Comment at: llvm/test/tools/obj2yaml/linker-options.yaml:61-64
+## 2) Non-null terminated content.
+  - Name: .linker-options-no-null
+    Type: SHT_LLVM_LINKER_OPTIONS
+    Content: "61"
----------------
Perhaps also a case combining 2) and 3), i.e. Content: "610062" or something like that.


================
Comment at: llvm/test/tools/yaml2obj/linker-options.yaml:7
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj --sections --section-data %t1 | FileCheck %s --check-prefix=OPTIONS
+
----------------
Rather than the --section-data switch, it might be clearer to use the --string-dump switch to dump the first section. That way, you can more easily match the original strings to their contents. (I assume you're deliberately not using the --elf-linker-options switch)


================
Comment at: llvm/test/tools/yaml2obj/linker-options.yaml:103
+
+## Check we should specify either "Options" or "Content".
+
----------------
should -> must (but see my earlier point)


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:626
+  S->Options = std::vector<ELFYAML::LinkerOption>();
+  for (size_t I = 0; I != Strings.size(); I += 2)
+    S->Options->emplace_back(Strings[I], Strings[I + 1]);
----------------
Isn't LLVM style to calculate the Strings size only once?

`for (size_t I = 0, E = Strings.size(); I != E; I += 2)`


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

https://reviews.llvm.org/D69895