[PATCH] D128968: [JITLink][COFF] Initial COFF support.

Sat Jul 9 11:56:56 PDT 2022

sunho added inline comments.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:428-430
+// Since two symbols always come in a specific order, we store the first
+// symbol to ComdatLeaders table and emits it as a external symbol when
+// we encounter the COMDAT symbol.
----------------
lhames wrote:
> sunho wrote:
> > lhames wrote:
> > > Does this mean that the second symbol in the sequence will always follow immediately after the first?
> > > Do we need an array for `ComdatLeaders`, or just the most recent COMDAT leader symbol?
> > In the spec, it says "The first symbol having the section value of the COMDAT section." So technically, the second symbol might be separted in the global symbol list. Practically, it should be fine to store the only most recent one though. I went with array way ultimately to potentially deal with largest symbol later. lld does some interesting "combines" of multiple COMDAT symbols to a single leader symbol when it deals with largest selection type.
> Do we need to worry about combines? I've been assuming that COMDAT sections are unique within an individual `.o`.
I looked into this again and you're right -- it should be across object files. I've updated the code to only look at most recent one and made naming more clear. To detect failrue (e.g. MC somehow sneak in comdat symbol of another section), I've added a bunch of error checkings.

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

https://reviews.llvm.org/D128968