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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 3 inline comments as done.
mstorsjo added inline comments.


================
Comment at: include/llvm/ObjectYAML/COFFYAML.h:62
   StringRef SymbolName;
+  Optional<uint32_t> SymbolTableIndex;
 };
----------------
jhenderson wrote:
> SymbolTableIndex -> SymbolIndex
> 
> "SymbolTableIndex" to me implies the section index of the symbol table.
Well the field in the actual relocation also is called SymbolTableIndex, so I chose that as it's an exact match of what's stored on disk. In coff, the symbol table isn't stored in a section, so there shouldn't be any confusion with that (in a pure coff environment).


================
Comment at: tools/obj2yaml/coff2yaml.cpp:146
 
+  StringMap<size_t> SymbolOccurrances;
+  for (const auto &S : Obj.symbols()) {
----------------
jhenderson wrote:
> SymbolOccurrances -> SymbolOccurrences
Ok, will fix.

Later I though of using a StringMap<bool> SymbolUnique instead, to conserve space a little.


================
Comment at: tools/yaml2obj/yaml2coff.cpp:531-533
       uint32_t SymbolTableIndex = SymbolTableIndexMap[R.SymbolName];
+      if (R.SymbolTableIndex)
+        SymbolTableIndex = *R.SymbolTableIndex;
----------------
jhenderson wrote:
> It should probably be an error somewhere if both fields are set (possibly unless they indicate the same symbol). I'd also write this as a ternary, with the symbol index being the first case, to avoid the lookup if not necessary, unless the intention is to insert a default constructed symbol in the map when no symbol exists?
Ok, that sounds sensible. It's not intended to insert a dummy element in the map (although I doubt that it'd be harmful either).

I'll see if it making it an error here fits in.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56294