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

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:123
+    // Get the section's memory protection flags.
+    MemProt Prot = {};
+    if ((*Sec)->Characteristics & COFF::IMAGE_SCN_MEM_EXECUTE)
----------------
sunho wrote:
> lhames wrote:
> > `MemProt Prot;` will default-initialize -- no need to be explicit here. 
> Looks like it doesn't default initialize, but I made it more explicit by using MemProt::None.
Quite right -- my mistake!


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:230
+             "IMAGE_WEAK_EXTERN_SEARCH_NOLIBRARY is not supported.");
+      WeakAliasRequests.push_back({SymIndex, TagIndex, SymbolName});
+    } else {
----------------
sunho wrote:
> lhames wrote:
> > I'm not very familiar with COFF, but just took a look at https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#auxiliary-format-3-weak-externals.
> > 
> > It sounds like `IMAGE_SYM_CLASS_WEAK_EXTERNAL` behaves differently depending on the value of the `Characteristics` field:
> > 
> > * "`IMAGE_WEAK_EXTERN_SEARCH_NOLIBRARY` indicates that no library search for sym1 should be performed". That sounds a bit vague, but additional conversation (e.g. [[ https://sourceforge.net/p/mingw/mailman/mingw-users/thread/000a01c5a5e8%24b0737700%24de6d65da%40DANNY/ | here ]]) helps to clarify:
> > > The SVR4 ABI uses (and bfd currently supports)
> > > the equivalent of IMAGE_WEAK_EXTERN_SEARCH_NOLIBRARY semantics (a
> > > library member does not resolve a weak symbol unless a strong symbol
> > > cause the member to be linked in)
> > 
> > Some thoughts off the top of my head:
> > 
> > ORC can't currently support this. ORC lookups have the related concept of a `LookupKind` (static linking context, or a dynamic lookup context) which we use to decide whether to pull in definitions from static archives: -- static links pull in archive defs, dynamic lookups don't.
> > 
> > This would be something new: "don't pull this symbol in at all, but bind it if it's already present", and it's a per-symbol behavior, rather than per-lookup.
> > 
> > We could support this by adding a new "ReallyWeakLinkage" type to the LinkGraph, and `SymbolLookupFlags::ReallyWeaklyReferencedSymbol` value. 
> >  
> > It's worth filing a GitHub issue for this, but for now I think this patch is right to treat it as unsupported and return an error.
> > 
> > * "`IMAGE_WEAK_EXTERN_SEARCH_LIBRARY` indicates that a library search for sym1 should be performed". We can probably implement this as a plain weak external symbol in the LinkGraph. This should be added to COFFLinkGraphBuilder in this patch or a follow-up.
> > 
> > * "`IMAGE_WEAK_EXTERN_SEARCH_ALIAS` indicates that sym1 is an alias for sym2". We should add an explicit symbol alias representation to the LinkGraph -- MachO and ELF both need it too (though actual use is rare). For now the approach used in this patch is good.
> Let me do it in a follow up patch -- it will require an additional test case. Only IMAGE_WEAK_EXTERN_SEARCH_ALIAS is used in all the testcases added in this patch. Supporting alias natively in LinkGraph level is indeed interesting to look at.
Sounds good to me.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:406
+    if (Definition->Selection == COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE) {
+      // FIXME: don't dead strip this when parent section is alive
+      return &G->addDefinedSymbol(
----------------
sunho wrote:
> lhames wrote:
> > I guess we should be able to implement these semantics in the future with a new `LinkGraph` `Section` flags and a custom dead-stripping pass?
> Yes. We can dead strip these sections when parent section was dead stripped. So, we might add "parent" relationship between sections too.
COFF sections are single-block, right? If so, in the short term we should use KeepAlive edges for this.

Longer term we can think about if/how to introduce COMDAT concepts into `LinkGraph` itself.




================
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.
----------------
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`.


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

https://reviews.llvm.org/D128968