[PATCH] D57975: [ObjectYAML] Let dynamic entries use section names as values

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D57975#1400108 <https://reviews.llvm.org/D57975#1400108>, @amontanez wrote:

> Rebase, reduced patch contents to only include one feature addition. Rather than using a unified `Value` field, there's now `Value` and `String` fields. This was done because a unified `Value` field that could accept strings and numbers made it somewhat difficult for obj2yaml to output an appropriately formatted value. This is up for discussion, though, as I'm not sure what I've done is the best solution.


Honestly, I feel that a unified field would be better (note, I haven't thought much about how that would affect the implementation). I think that matches what we already do with other fields like Section's Info etc. It is also more intuitive, and you don't have to handle the case of both being specified. I also don't think it matters if yaml2obj -> obj2yaml produces the output identical to the input, as long as it is still valid. In this case, it could (should?) use the actual address of the referenced section, rather than trying to figure out the section name.



================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:666-667
     IO &IO, ELFYAML::ELF_DYNTAG &Value) {
-  assert(IO.getContext() && "The IO context is not initialized");
-
 // TODO: For simplicity we do not handle target specific flags. They are
----------------
Why has this assert disappeared? It seems unrelated to the change?


================
Comment at: llvm/test/tools/yaml2obj/dynamic-entries.yaml:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj -dynamic %t | FileCheck %s --check-prefix=DYNAMIC
+
----------------
Nit: Avoid mixing single and double dash long args in the same test. I prefer double-dash to avoid ambiguity with option grouping, but it's not a big deal.


================
Comment at: llvm/test/tools/yaml2obj/dynamic-entry-invalid-val.yaml:19
+
+# CHECK: error: String is not a valid section name
----------------
I'd specify the name of the section being searched for in this error message (i.e. "xxyyzz").


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:388
+    SHeader.sh_addralign = sizeof(Elf_Dyn)/2;
+  // TODO: Link .dynamic to .dynstr by default.
+}
----------------
Why can't this be done now by calling `getDotDynStrSecNo`?


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:417
+
+    // Otherwise, attempt to treat as a number.
+    } else if (YamlEntry.Val.hasValue()) {
----------------
Assuming you stick with different fields for string and value, I think this and the other comments and error messages in this area might need a bit of tweaking.


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:780
+template <class ELFT> bool ELFState<ELFT>::hasDynamicEntries() const {
+  return YamlDynamicSection;
+}
----------------
This is a personal thing, so feel free to ignore, but I find it easier to read if this explicitly compares against null.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57975