[PATCH] D54802: [LLD][COFF] Generate import modules in PDB

[Alexandre Ganea via Phabricator via llvm-commits]

aganea marked 2 inline comments as done.
aganea added a comment.

After further testing, in some scenarios this patch doesn't seem to be enough for Recode <http://www.indefiant.com/> to work. I'll pause this patch and follow up with Will (@lantictac) to ensure everything is covered.



================
Comment at: lld/trunk/COFF/Driver.cpp:1226
   // precedence, but we will emit a warning if there is a conflict.
-  parseMerge(".idata=.rdata");
   parseMerge(".didat=.rdata");
----------------
pcc wrote:
> rnk wrote:
> > Do you want to submit this separately? It seems to cause wide ranging mechanical changes to the tests. Feel free to submit it without review.
> Are you sure that this is what link.exe does? I'm pretty sure I added this here after noticing that this was what link.exe did, see D45737
{F7611855}

@pcc: Please see the zip file above for comparing the output from different linkers.

I figured out a few more things with `link.exe` along the way:

- `S_TRAMPOLINE` records are used for jump thunks when incremental linking is active. Of course this makes the produced EXE to execute slower because there are missed function inlining oportunities.
- The `* Linker *` module has a section contribution in `.idata` only when incremental linking is specified. Otherwise, there's no contrib section, only a debug stream.
- There's a lot more `.text` padding (CCCCCC) around functions when incremental linking is active.
- More importantly, `link.exe` folds `.idata` sections into `.rdata` **only** when `/INCREMENTAL:NO` is specified. Given that `/INCREMENTAL` is the default, I thought we should do the same in LLD. I would need to disable that behavior when providing `/INCREMENTAL:NO` (not in this patch yet).

Doc and defaults for /INCREMENTAL [[ https://docs.microsoft.com/en-us/cpp/build/reference/incremental-link-incrementally?view=vs-2017 | are here ]].


================
Comment at: lld/trunk/COFF/Writer.cpp:758-760
+    InputSection *ISec =
+        make<InputSection>(Pair.first.first, OutChars, Chunks, Sec);
+    InputSections.push_back(ISec);
----------------
rnk wrote:
> This seems like it's really just replicating `Map` into some longer lived data structure, and then creating a backreference from the output section to the concatenated input section. There are three other helper functions that take a reference to `Map`, and that type is ridiculously long. I think we could clean this code up significantly if we planned some NFC changes first to make `Map` a member of `Writer` and then give it a real key and value type. WDYT? I'm imagining that the type would become: `std::map<NameAndChars, InputSection>`
> 
> That would have the side effect of cleaning up a lot of unreadable `Pair.second` and `Pair.first.first` accesses in this code.
> 
> Honestly I started looking at this at first just because we copy the `Chunks` vector again, and I was wondering if that could be avoided, especially for a no-PDB build...
Agreed. Will do that.


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

https://reviews.llvm.org/D54802