[ObjectYAML] [COFF] Support multiple symbols with the same name

[mstorsjo]

Summary

This is necessary for YAML based tests for D55881.

[jhenderson]

I think a composite test (or tests) would makes sense, but check the intermediate output as well as the final output.

include/llvm/ObjectYAML/COFFYAML.h

62 ↗

(On Diff #180151)

SymbolTableIndex -> SymbolIndex

"SymbolTableIndex" to me implies the section index of the symbol table.

tools/obj2yaml/coff2yaml.cpp

146 ↗

(On Diff #180151)

SymbolOccurrances -> SymbolOccurrences

tools/yaml2obj/yaml2coff.cpp

531–533 ↗

(On Diff #180151)

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?

[mstorsjo]

include/llvm/ObjectYAML/COFFYAML.h

62 ↗

(On Diff #180151)

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).

tools/obj2yaml/coff2yaml.cpp

146 ↗

(On Diff #180151)

Ok, will fix.

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

tools/yaml2obj/yaml2coff.cpp

531–533 ↗

(On Diff #180151)

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.

[jhenderson]

include/llvm/ObjectYAML/COFFYAML.h

62 ↗

(On Diff #180151)

Okay, fair enough. Keep it as is.

tools/obj2yaml/coff2yaml.cpp

146 ↗

(On Diff #180151)

Yes, that would make sense.

[mstorsjo]

Added a testcase, applied @jhenderson's suggestions (erroring out if both SymbolName and SymbolTableIndex are present, avoiding touching SymbolTableIndexMap if direct index is set, changed the (misspelled) SymbolOccurrances map into a SymbolUnique map).

[ruiu]

tools/obj2yaml/coff2yaml.cpp

208 ↗

(On Diff #180311)

Doesn't this insert a new key if *SymbolNameOrErr does not exist in the map? I don't think that affects correctness of the program, but that's probably a waste of time and memory. I'd use count instead.

[mstorsjo]

tools/obj2yaml/coff2yaml.cpp

208 ↗

(On Diff #180311)

Yes, but every symbol will exist in the map already. We're not interested in whether the key exists in the map or not (it will always exist), but whether the map entry actually is set to true or false.

[ruiu]

tools/obj2yaml/coff2yaml.cpp

208 ↗

(On Diff #180311)

Ah OK, then this is fine. I'd still use count to not surprise careless readers like me though. :)

[mstorsjo]

tools/obj2yaml/coff2yaml.cpp

208 ↗

(On Diff #180311)

Why count? Wouldn't that just return 1 saying that the symbol indeed is present in the map? We're not interested in whether the key is present or not (it always is), we're interested in the value of the map element, and count won't help with that.

[mstorsjo]

tools/obj2yaml/coff2yaml.cpp

208 ↗

(On Diff #180311)

You're imagining a StringSet that only contains the symbols we know are unique. It's not that (building such a data structure would be tricky) but a StringMap<bool> which could be called StringIsUnique, which stores all symbols and whether they are unique or not, as a bool.

[ruiu]

tools/obj2yaml/coff2yaml.cpp

208 ↗

(On Diff #180311)

Sorry, what I wanted to say is lookup.

[mstorsjo]

tools/obj2yaml/coff2yaml.cpp

208 ↗

(On Diff #180311)

Oh, ok - I can change it to that.

[mstorsjo]

Changed [] into .lookup(), as requested by @ruiu.

[alexander-shaposhnikov]

include/llvm/ObjectYAML/COFFYAML.h

62 ↗

(On Diff #180151)

I'd probably add a comment somewhere explaining that this "duplication" (the presence of both Index and Name) are for convenience (that i.e. in YAML one can specify either the name or the index) + handling of non-unique names. P.S. if i understood the rationale for this change correctly, of course

[mstorsjo]

include/llvm/ObjectYAML/COFFYAML.h

62 ↗

(On Diff #180151)

Sure, I can add a comment.

[mstorsjo]

Added a comment to the COFFYAML.h header.

[alexander-shaposhnikov]

LGTM with one minor nit

tools/obj2yaml/coff2yaml.cpp

151 ↗

(On Diff #180329)

side note: double lookup (1st - find, second - operator []) can be avoided here if we switch to using the method "insert" instead (it returns the corresponding iterator).

[mstorsjo]

tools/obj2yaml/coff2yaml.cpp

151 ↗

(On Diff #180329)

Oh, nice, will do.

[mstorsjo]

Using the insert method, to avoid double lookup. Will push next week after giving @jhenderson a chance to comment, unless there's more things to change.

[jhenderson]

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.

include/llvm/ObjectYAML/COFFYAML.h

61 ↗

(On Diff #180363)

To clarify that this comment refers to the two following variables, please put a new line before it.

test/tools/yaml2obj/coff-symbol-index.yaml

45 ↗

(On Diff #180363)

Is this YAML blob as minimal as it can get whilst still testing the required properties? It's very verbose!

[mstorsjo]

Added a newline before the comment in the header, minimized the yaml testcase.

[jhenderson]

Okay, looking much better. I'm happy for this to go in, but if any of the other bits I highlighted can be removed, even better.

test/tools/yaml2obj/coff-symbol-index.yaml

34 ↗

(On Diff #180455)

Are all these required? Same comment for the other sections.

59–61 ↗

(On Diff #180455)

Are these fields required? Same question for further below.

78 ↗

(On Diff #180455)

Is this necessary?

80 ↗

(On Diff #180455)

Is this needed?

[mstorsjo]

test/tools/yaml2obj/coff-symbol-index.yaml

34 ↗

(On Diff #180455)

Yes, Characteristics is mandatory, otherwise it fails like this:

YAML:33:5: error: missing required key 'Characteristics'

59–61 ↗

(On Diff #180455)

Yes, these fields are necessary.

78 ↗

(On Diff #180455)

The whole SectionDefinition part can be omitted while keeping the test functioning.

80 ↗

(On Diff #180455)

Can be omitted while keeping the test working.

[mstorsjo]

Stripped the test yaml further, with empty Characteristics and removed SectionDefinitions.

llvm/trunk/include/llvm/ObjectYAML/COFFYAML.h

 
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, COMDATType)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, WeakExternalCharacteristics)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, AuxSymbolType)
 
 struct Relocation {
   uint32_t VirtualAddress;
   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
+  // specified), allowing disambiguating between multiple symbols with the
+  // same name or crafting intentionally broken files for testing.
   StringRef SymbolName;
+  Optional<uint32_t> SymbolTableIndex;
 };
 
 struct Section {
   COFF::section Header;
   unsigned Alignment = 0;
   yaml::BinaryRef SectionData;
   std::vector<CodeViewYAML::YAMLDebugSubsection> DebugS;
   std::vector<CodeViewYAML::LeafRecord> DebugT;

llvm/trunk/lib/ObjectYAML/COFFYAML.cpp

struct NDLLCharacteristics {
   COFF::DLLCharacteristics Characteristics;
 };
 
 } // end anonymous namespace
 
 void MappingTraits<COFFYAML::Relocation>::mapping(IO &IO,
                                                   COFFYAML::Relocation &Rel) {
   IO.mapRequired("VirtualAddress", Rel.VirtualAddress);
-  IO.mapRequired("SymbolName", Rel.SymbolName);
+  IO.mapOptional("SymbolName", Rel.SymbolName, StringRef());
+  IO.mapOptional("SymbolTableIndex", Rel.SymbolTableIndex);
 
   COFF::header &H = *static_cast<COFF::header *>(IO.getContext());
   if (H.Machine == COFF::IMAGE_FILE_MACHINE_I386) {
     MappingNormalization<NType<COFF::RelocationTypeI386>, uint16_t> NT(
         IO, Rel.Type);
     IO.mapRequired("Type", NT->Type);
   } else if (H.Machine == COFF::IMAGE_FILE_MACHINE_AMD64) {
     MappingNormalization<NType<COFF::RelocationTypeAMD64>, uint16_t> NT(

llvm/trunk/test/tools/yaml2obj/coff-symbol-index.yaml

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj -relocations %t | FileCheck %s --check-prefix=RELOCS
+# RUN: obj2yaml %t | FileCheck %s --check-prefix=YAML
+
+# RELOCS:      Relocations [
+# RELOCS-NEXT:   Section (1) .text {
+# RELOCS-NEXT:     0x3 IMAGE_REL_AMD64_REL32 .rdata (0)
+# RELOCS-NEXT:     0xA IMAGE_REL_AMD64_REL32 .rdata (1)
+# RELOCS-NEXT:     0x11 IMAGE_REL_AMD64_REL32 foo (2)
+# RELOCS-NEXT:   }
+# RELOCS-NEXT: ]
+
+# Check that we usually output relocations with SymbolName.
+# For relocations with a non-unique symbol name, output
+# SymbolTableIndex instead.
+
+# YAML:          Relocations:     
+# YAML-NEXT:       - VirtualAddress:  3
+# YAML-NEXT:         SymbolTableIndex: 0
+# YAML-NEXT:         Type:            IMAGE_REL_AMD64_REL32
+# YAML-NEXT:       - VirtualAddress:  10
+# YAML-NEXT:         SymbolTableIndex: 1
+# YAML-NEXT:         Type:            IMAGE_REL_AMD64_REL32
+# YAML-NEXT:       - VirtualAddress:  17
+# YAML-NEXT:         SymbolName:      foo
+# YAML-NEXT:         Type:            IMAGE_REL_AMD64_REL32
+
+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     488B0500000000488B0500000000488B0500000000
+    Relocations:     
+      - VirtualAddress:  3
+        SymbolTableIndex: 0
+        Type:            IMAGE_REL_AMD64_REL32
+      - VirtualAddress:  10
+        SymbolTableIndex: 1
+        Type:            IMAGE_REL_AMD64_REL32
+      - VirtualAddress:  17
+        SymbolName:      foo
+        Type:            IMAGE_REL_AMD64_REL32
+  - Name:            .rdata
+    Characteristics: [  ]
+    Alignment:       1
+    SectionData:     '00'
+  - Name:            .rdata
+    Characteristics: [  ]
+    Alignment:       1
+    SectionData:     '01'
+symbols:         
+  - Name:            .rdata
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            .rdata
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            foo
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

llvm/trunk/tools/obj2yaml/coff2yaml.cpp

 //===------ utils/obj2yaml.cpp - obj2yaml conversion tool -------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "obj2yaml.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/DebugInfo/CodeView/DebugChecksumsSubsection.h"
 #include "llvm/DebugInfo/CodeView/DebugStringTableSubsection.h"
 #include "llvm/DebugInfo/CodeView/StringsAndChecksums.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/ObjectYAML/COFFYAML.h"
 #include "llvm/ObjectYAML/CodeViewYAMLTypes.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/YAMLTraits.h"
initializeFileAndStringTable(const llvm::object::COFFObjectFile &Obj,
   }
 }
 
 void COFFDumper::dumpSections(unsigned NumSections) {
   std::vector<COFFYAML::Section> &YAMLSections = YAMLObj.Sections;
   codeview::StringsAndChecksumsRef SC;
   initializeFileAndStringTable(Obj, SC);
 
+  StringMap<bool> SymbolUnique;
+  for (const auto &S : Obj.symbols()) {
+    object::COFFSymbolRef Symbol = Obj.getCOFFSymbol(S);
+    StringRef Name;
+    Obj.getSymbolName(Symbol, Name);
+    StringMap<bool>::iterator It;
+    bool Inserted;
+    std::tie(It, Inserted) = SymbolUnique.insert(std::make_pair(Name, true));
+    if (!Inserted)
+      It->second = false;
+  }
+
   for (const auto &ObjSection : Obj.sections()) {
     const object::coff_section *COFFSection = Obj.getCOFFSection(ObjSection);
     COFFYAML::Section NewYAMLSection;
     ObjSection.getName(NewYAMLSection.Name);
     NewYAMLSection.Header.Characteristics = COFFSection->Characteristics;
     NewYAMLSection.Header.VirtualAddress = COFFSection->VirtualAddress;
     NewYAMLSection.Header.VirtualSize = COFFSection->VirtualSize;
     NewYAMLSection.Header.NumberOfLineNumbers =
for (const auto &Reloc : ObjSection.relocations()) {
       Expected<StringRef> SymbolNameOrErr = Sym->getName();
       if (!SymbolNameOrErr) {
        std::string Buf;
        raw_string_ostream OS(Buf);
        logAllUnhandledErrors(SymbolNameOrErr.takeError(), OS);
        OS.flush();
        report_fatal_error(Buf);
       }
+      if (SymbolUnique.lookup(*SymbolNameOrErr))
         Rel.SymbolName = *SymbolNameOrErr;
+      else
+        Rel.SymbolTableIndex = reloc->SymbolTableIndex;
       Rel.VirtualAddress = reloc->VirtualAddress;
       Rel.Type = reloc->Type;
       Relocations.push_back(Rel);
     }
     NewYAMLSection.Relocations = Relocations;
     YAMLSections.push_back(NewYAMLSection);
   }
 }

llvm/trunk/tools/yaml2obj/yaml2coff.cpp

 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/DebugInfo/CodeView/DebugStringTableSubsection.h"
 #include "llvm/DebugInfo/CodeView/StringsAndChecksums.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/ObjectYAML/ObjectYAML.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <vector>
 
 using namespace llvm;
 
 /// This parses a yaml stream that represents a COFF object file.
 /// See docs/yaml2obj for the yaml scheema.
 struct COFFParser {
for (const COFFYAML::Section &S : CP.Obj.Sections) {
     if (!S.Header.SizeOfRawData)
       continue;
     assert(S.Header.PointerToRawData >= OS.tell());
     OS << num_zeros(S.Header.PointerToRawData - OS.tell());
     S.SectionData.writeAsBinary(OS);
     assert(S.Header.SizeOfRawData >= S.SectionData.binary_size());
     OS << num_zeros(S.Header.SizeOfRawData - S.SectionData.binary_size());
     for (const COFFYAML::Relocation &R : S.Relocations) {
-      uint32_t SymbolTableIndex = SymbolTableIndexMap[R.SymbolName];
+      uint32_t SymbolTableIndex;
+      if (R.SymbolTableIndex) {
+        if (!R.SymbolName.empty())
+          WithColor::error()
+              << "Both SymbolName and SymbolTableIndex specified\n";
+        SymbolTableIndex = *R.SymbolTableIndex;
+      } else {
+        SymbolTableIndex = SymbolTableIndexMap[R.SymbolName];
+      }
       OS << binary_le(R.VirtualAddress)
          << binary_le(SymbolTableIndex)
          << binary_le(R.Type);
     }
   }
 
   // Output symbol table.