[PATCH] D63001: [yaml2obj] - Do not assert when .dynsym is specified explicitly, but .dynstr is not present.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/yaml2obj/explicit-dynsym-no-dynstr.yaml:7-9
+## TODO: Check that .dynsym has Link field set to 0.
+##       GNU readelf is able to dump sections headers,
+##       but llvm-readelf report an error below too early.
----------------
Could you file/find a bug for this and include the bug URL here?

Also report -> reports


================
Comment at: tools/yaml2obj/yaml2elf.cpp:371
+
+  // When we describe the .dynsym section in document explicitly, it is allowed
+  // to omit "DynamicSymbols" tag. In this case, .dynstr is not added implicitly
----------------
in document -> in the document


================
Comment at: tools/yaml2obj/yaml2elf.cpp:372
+  // When we describe the .dynsym section in document explicitly, it is allowed
+  // to omit "DynamicSymbols" tag. In this case, .dynstr is not added implicitly
+  // and we should be able to leave the Link zeroed if .dynstr is not described.
----------------
omit "DynamicSymbols" -> omit the "DynamicSymbols"
You don't need the ',' after "case"


================
Comment at: tools/yaml2obj/yaml2elf.cpp:373
+  // to omit "DynamicSymbols" tag. In this case, .dynstr is not added implicitly
+  // and we should be able to leave the Link zeroed if .dynstr is not described.
+  unsigned Link = 0;
----------------
described -> defined?


================
Comment at: tools/yaml2obj/yaml2elf.cpp:375-379
+  if (IsStatic)
+    Link = SN2I.get(".strtab");
+  else
+    SN2I.lookup(".dynstr", Link);
+  SHeader.sh_link = Link;
----------------
It looks like this doesn't allow for a custom-specified sh_link field. It might be nice to add that later.


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

https://reviews.llvm.org/D63001