[ORC][COFF] Properly set weak flag to COMDAT symbols.

[sunho]

Summary

Properly set weak flag to COMDAT symbols so that no duplicate definition error will be generated. There is an inaccuracy in setting plain weak for largest selection type, which will be dealt with soon when largest type is properly implemented.

[sgraenitz]

Thanks!

llvm/lib/ExecutionEngine/Orc/ObjectFileInterface.cpp

165

I guess this could indeed fail for malformed objects. The referred section might not exist, right?

llvm/test/ExecutionEngine/JITLink/X86/COFF_comdat_weak_plus_strong.s

12

I never double-checked this with the Mircosoft toolchain. Did you?

llvm/test/ExecutionEngine/JITLink/X86/COFF_strong_duplicate.s

10

Can we use # RUN: not llvm-jitlink ... to check that we get a non-zero exit code instead of XFAILing the test? My impression was that XFAIL should be used specifically to mark bugs and todos.

[sunho]

Thanks for the review!

llvm/lib/ExecutionEngine/Orc/ObjectFileInterface.cpp

165

It shouldn't happen in valid object file, but yes it could happen in invalid ones, I'll change it to check error.

llvm/test/ExecutionEngine/JITLink/X86/COFF_comdat_weak_plus_strong.s

12

I just checked with msvc toolchain and it gets duplicate definition error. I guess we need to introduce new linkage type for this.

llvm/test/ExecutionEngine/JITLink/X86/COFF_strong_duplicate.s

10

I didn't know about not when I wrote this. Thanks for pointing out!

[sunho]

Address comments, strong + weak test will be corrected in follow up patch.

[lhames]

Otherwise LGTM. Thanks @sunho!

llvm/test/ExecutionEngine/JITLink/X86/COFF_strong_duplicate.s

10

Even better, can we check for the text of the error to verify that it failed for the expected reason? (And likewise in the other testcase, if possible)

llvm/lib/ExecutionEngine/Orc/ObjectFileInterface.cpp

getELFObjectFileSymbolInfo(ExecutionSession &ES,
 
   return I;
 }
 
 static Expected<MaterializationUnit::Interface>
 getCOFFObjectFileSymbolInfo(ExecutionSession &ES,
                             const object::COFFObjectFile &Obj) {
   MaterializationUnit::Interface I;
-
+  std::vector<Optional<object::coff_aux_section_definition>> ComdatDefs(
+      Obj.getNumberOfSections() + 1);
   for (auto &Sym : Obj.symbols()) {
     Expected<uint32_t> SymFlagsOrErr = Sym.getFlags();
     if (!SymFlagsOrErr)
       // TODO: Test this error.
       return SymFlagsOrErr.takeError();
 
+    // Handle comdat symbols
+    auto COFFSym = Obj.getCOFFSymbol(Sym);
+    bool IsWeak = false;
+    if (auto *Def = COFFSym.getSectionDefinition()) {
+      auto Sec = Obj.getSection(COFFSym.getSectionNumber());
+      if (!Sec)
+        return Sec.takeError();
+      if (((*Sec)->Characteristics & COFF::IMAGE_SCN_LNK_COMDAT) &&
+          Def->Selection != COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE) {
+        ComdatDefs[COFFSym.getSectionNumber()] = *Def;
+        continue;
+      }
+    }
+    if (!COFF::isReservedSectionNumber(COFFSym.getSectionNumber()) &&
+        ComdatDefs[COFFSym.getSectionNumber()]) {
+      auto Def = ComdatDefs[COFFSym.getSectionNumber()];
+      if (Def->Selection != COFF::IMAGE_COMDAT_SELECT_NODUPLICATES) {
+        IsWeak = true;
+      }
+      ComdatDefs[COFFSym.getSectionNumber()] = None;
+    } else {
       // Skip symbols not defined in this object file.
       if (*SymFlagsOrErr & object::BasicSymbolRef::SF_Undefined)
         continue;
+    }
 
     // Skip symbols that are not global.
     if (!(*SymFlagsOrErr & object::BasicSymbolRef::SF_Global))
       continue;
 
     // Skip symbols that have type SF_File.
     if (auto SymType = Sym.getType()) {
       if (*SymType == object::SymbolRef::ST_File)
         continue;
     } else
       return SymType.takeError();
 
     auto Name = Sym.getName();
     if (!Name)
       return Name.takeError();
 
     auto SymFlags = JITSymbolFlags::fromObjectSymbol(Sym);
     if (!SymFlags)
       return SymFlags.takeError();
     *SymFlags |= JITSymbolFlags::Exported;
-    auto COFFSym = Obj.getCOFFSymbol(Sym);
 
     // Weak external is always a function
-    if (COFFSym.isWeakExternal()) {
+    if (COFFSym.isWeakExternal())
       *SymFlags |= JITSymbolFlags::Callable;
-    }
+
+    if (IsWeak)
+      *SymFlags |= JITSymbolFlags::Weak;
 
     I.SymbolFlags[ES.intern(*Name)] = std::move(*SymFlags);
   }
 
   // FIXME: handle init symbols
 
   return I;
 }

llvm/test/ExecutionEngine/JITLink/X86/COFF_comdat_weak_duplicate.s

+# RUN: rm -rf %t && mkdir -p %t
+# RUN: yaml2obj %S/Inputs/COFF_comdat_weak_def.yaml -o %t/COFF_weak_1.o
+# RUN: yaml2obj %S/Inputs/COFF_comdat_weak_def.yaml -o %t/COFF_weak_2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc %s -o %t/COFF_main.o
+# RUN: 
+# RUN: llvm-jitlink -noexec %t/COFF_main.o %t/COFF_weak_1.o %t/COFF_weak_2.o \
+# RUN: -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
+# RUN: -show-graph -noexec 2>&1 | FileCheck %s
+#
+# Check that duplicate comdat any definitions don't generate duplicate definition error.
+#
+# CHECK: section weakfunc:
+# CHECK-EMPTY:
+# CHECK-NEXT:  block 0xfff02000 size = 0x00000001, align = 16, alignment-offset = 0
+# CHECK-NEXT:    symbols:
+# CHECK-NEXT:      0xfff02000 (block + 0x00000000): size: 0x00000001, linkage: weak, scope: default, live  -   func
+# CHECK-NEXT:    no edges
+
+  .text
+
+  .def  main;
+  .scl  2;
+  .type 32;
+  .endef
+  .globl  main
+  .p2align  4, 0x90
+main:
+    callq func
+  retq
 No newline at end of file

llvm/test/ExecutionEngine/JITLink/X86/COFF_comdat_weak_plus_strong.s

+# FIXME: Comdat any + ordinary strong symbol should generate duplicate section error
+# XFAIL: *
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: yaml2obj %S/Inputs/COFF_comdat_weak_def.yaml -o %t/COFF_weak_1.o
+# RUN: yaml2obj %S/Inputs/COFF_strong_def.yaml -o %t/COFF_strong.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc %s -o %t/COFF_main.o
+# RUN: 
+# RUN: not llvm-jitlink -noexec %t/COFF_main.o %t/COFF_weak_1.o %t/COFF_strong.o \
+# RUN: -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
+# RUN: -show-graph -noexec 2>&1 | FileCheck %s
+#
+# Check that a combination of comdat any definition and strong definition generate 
+# duplicate definition error.
+#
+# CHECK: section strongfunc:
+# CHECK-EMPTY:
+# CHECK-NEXT:  block 0xfff02000 size = 0x00000001, align = 16, alignment-offset = 0
+# CHECK-NEXT:    symbols:
+# CHECK-NEXT:      0xfff02000 (block + 0x00000000): size: 0x00000001, linkage: strong, scope: default, live  -   func
+# CHECK-NEXT:    no edges
+
+  .text
+
+  .def  main;
+  .scl  2;
+  .type 32;
+  .endef
+  .globl  main
+  .p2align  4, 0x90
+main:
+    callq func
+  retq
 No newline at end of file

llvm/test/ExecutionEngine/JITLink/X86/COFF_strong_duplicate.s

+# RUN: rm -rf %t && mkdir -p %t
+# RUN: yaml2obj %S/Inputs/COFF_strong_def.yaml -o %t/COFF_strong_1.o
+# RUN: yaml2obj %S/Inputs/COFF_strong_def.yaml -o %t/COFF_strong_2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc %s -o %t/COFF_main.o
+# RUN: 
+# RUN: not llvm-jitlink -noexec %t/COFF_main.o %t/COFF_strong_1.o %t/COFF_strong_2.o \
+# RUN: -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
+# RUN: -show-graph
+#
+# Check that duplicate strong definitions cause llvm-jitlink to terminate with error.
+#
+
+  .text
+
+  .def  main;
+  .scl  2;
+  .type 32;
+  .endef
+  .globl  main
+  .p2align  4, 0x90
+main:
+    callq func
+  retq
 No newline at end of file

llvm/test/ExecutionEngine/JITLink/X86/Inputs/COFF_comdat_weak_def.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            weakfunc
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     C3
+symbols:
+  - Name:            weakfunc
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          1
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        40735498
+      Number:          1
+      Selection:       IMAGE_COMDAT_SELECT_ANY
+  - Name:            func
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

llvm/test/ExecutionEngine/JITLink/X86/Inputs/COFF_strong_def.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            strongfunc
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     C3
+symbols:
+  - Name:            strongfunc
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          1
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        40735498
+      Number:          1
+  - Name:            func
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...