[PATCH] D56294: [ObjectYAML] [COFF] Support multiple symbols with the same name

Mon Jan 7 02:46:23 PST 2019

jhenderson accepted this revision.
jhenderson added a comment.

I'm giving a conditional LGTM, assuming that the mentioned YAML blob is as small as reasonably possible to test the relocations. Remember, we don't need to have a fully usable object file, we just need one that has the relevant symbols and relocations.

================
Comment at: include/llvm/ObjectYAML/COFFYAML.h:61
   uint16_t Type;
+  // Normally a Relocation can refer to the symbol via its name.
+  // It can also use a direct symbol table index instead (with no name
----------------
To clarify that this comment refers to the two following variables, please put a new line before it.

================
Comment at: test/tools/yaml2obj/coff-symbol-index.yaml:45
+
+--- !COFF
+header:          
----------------
Is this YAML blob as minimal as it can get whilst still testing the required properties? It's very verbose!

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

https://reviews.llvm.org/D56294