[PATCH] D56569: [ObjectYAML][yaml2obj][ELF] Add basic support for dynamic entries

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:94
+  ELF_DT Tag;
+  Optional<uint64_t> Val;
+  Optional<llvm::yaml::Hex64> Ptr;
----------------
jhenderson wrote:
> Somewhat moot if you follow @ruiu's suggestion (which I support), but this should be an int64_t.
Hmm, why is this of type `StringRef`? Isn't this `uint64_t`?


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:413
+    // .dynstr table may be populated with strings.
+    if (SHeader.sh_link == getDotDynStrSecNo()) {
+      switch (YamlEntry.Tag) {
----------------
Perhaps you should inverse the condition and continue early, to reduce the indentation depth.


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:420
+        DotDynstr.add(YamlEntry.Val);
+      break;
+      }
----------------
Stray break? (You don't need this because this is at the end of the last `case`, and if you really want it it is not indented correctly.)


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:455-457
+          return yamlError(
+              YamlEntry,
+              "value and linked string table address are inconsistent");
----------------
At first I thought that `yamlError(...)` creates a new object and you were returning it, but that's actually not the case. You are just returning `false`. Why don't you write it in two lines?

  printError(YamlEntry, "value and linked string table address are inconsistent");
  return false;

If you define a helper lambda function inside this function like this

  auto Report = [&](StringRef Msg) {
    yaml::Output Yout(errs());
    Yout << YamlObj;
    WithColor::error() << Msg << "\n";
  };

then you can write like this:

  Report("value and linked string table address are inconsistent");
  return false;



================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:516
+  // Ensure DT_NULL is present.
+  {
+    Elf_Dyn NullEnt;
----------------
I don't think you need to work that hard to minimize a scope of a variable.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56569