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

[Sunho Kim via Phabricator via llvm-commits]

sunho added a comment.

Thanks for the review!



================
Comment at: llvm/lib/ExecutionEngine/Orc/ObjectFileInterface.cpp:165
+    if (auto *Def = COFFSym.getSectionDefinition()) {
+      auto Sec = cantFail(Obj.getSection(COFFSym.getSectionNumber()));
+      if ((Sec->Characteristics & COFF::IMAGE_SCN_LNK_COMDAT) &&
----------------
sgraenitz wrote:
> I guess this could indeed fail for malformed objects. The referred section might not exist, right?
It shouldn't happen in valid object file, but yes it could happen in invalid ones, I'll change it to check error.


================
Comment at: llvm/test/ExecutionEngine/JITLink/X86/COFF_comdat_weak_plus_strong.s:11
+# Check that a combination of comdat any definition and strong definition don't generate 
+# duplicate definition error.
+#
----------------
sgraenitz wrote:
> I never double-checked this with the Mircosoft toolchain. Did you?
I just checked  with msvc toolchain and it gets duplicate definition error. I guess we need to introduce new linkage type for this.


================
Comment at: llvm/test/ExecutionEngine/JITLink/X86/COFF_strong_duplicate.s:9
+# RUN: -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
+# RUN: -show-graph -noexec 2>&1 | FileCheck %s
+#
----------------
sgraenitz wrote:
> 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.
I didn't know about not when I wrote this. Thanks for pointing out!


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

https://reviews.llvm.org/D129764