[PATCH] D50917: [LLD] [COFF] Support MinGW automatic dllimport of data

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/Chunks.cpp:422
 
+static int getRuntimePseudoRelocSize(uint16_t Type) {
+  // Relocations that either contain an absolute address, or a plain
----------------
ruiu wrote:
> It's not clear to me what this function is supposed to return in the first place. Could you write a brief function comment?
Sure, will do - with this explanation:
```
// Check whether a static relocation of type Type can be deferred and
// handled at runtime as a pseudo relocation (for references to a module
// local variable, which turned out to actually need to be imported from
// another DLL) This returns the size the relocation is supposed to update,  
// in bits, or 0 if the relocation cannot be handled as a runtime pseudo  
// relocation.
```


================
Comment at: COFF/Chunks.cpp:493
+
+void SectionChunk::getRuntimePseudoRelocs(
+    std::vector<RuntimePseudoReloc> &Res) {
----------------
ruiu wrote:
> What this function is doing? Please write a brief function comment.
Will do, like this:
```
// Append information to the provided vector about all relocations that
// need to be handled at runtime as runtime pseudo relocations (references
// to a module local variable, which turned out to actually need to be
// imported from another DLL).
```


================
Comment at: COFF/Chunks.cpp:494
+void SectionChunk::getRuntimePseudoRelocs(
+    std::vector<RuntimePseudoReloc> &Res) {
+  for (const coff_relocation &Rel : Relocs) {
----------------
ruiu wrote:
> If `Res` is always empty at the entry of this function, return a new instance of a `std::vector` instead of taking an instance of `std::vector` as a reference.
It isn't empty at entry; we append entries to an existing vector.


================
Comment at: COFF/Chunks.cpp:631
 
+size_t PseudoRelocTableChunk::getSize() const {
+  if (Relocs.empty())
----------------
ruiu wrote:
> There's no explanation of "pseudo relocation". Please explain it somewhere.
Adding a comment like this above here:
```
// MinGW specific, for the "automatic import of variables from DLLs" feature.
```
I added a slightly longer comment about the same class in Chunks.h as well.


================
Comment at: COFF/Chunks.h:424
 
+class PseudoRelocTableChunk : public Chunk {
+public:
----------------
ruiu wrote:
> Please add comment to explain what this is.
I'll add a comment like this:
```
// MinGW specific, for the "automatic import of variables from DLLs" feature.
// This provides the table of runtime pseudo relocations, for variable
// references that turned out to need to be imported from a DLL even though
// the reference didn't use the dllimport attribute. The MinGW runtime will
// process this table after loading, before handling control over to user
// code.
```


================
Comment at: COFF/Chunks.h:436-437
+
+// This is a placeholder Chunk, to allow attaching a DefinedSynthetic to a
+// specific place in a section, without any data.
+class EmptyChunk : public Chunk {
----------------
ruiu wrote:
> Please explain the motivation of defining these classes in the first place. In particular, I'd mention that we don't really need this for MS compatibility but this is for MinGW.
Well the EmptyChunk isn't inherently anything MinGW specific at all, I could imagine that it would be useful for any case where you want to attach a Symbol to a specific place in an OutputSection, without actually producing any content.

I can mention that it is used for the MinGW specific `__RUNTIME_PSEUDO_RELOC_LIST_END__` symbol if that makes it clearer?


================
Comment at: COFF/Chunks.h:445
+
+class RuntimePseudoReloc {
+public:
----------------
ruiu wrote:
> Ditto
Added a comment like this:
```
// MinGW specific; information about one individual location in the image
// that needs to be fixed up at runtime after loading. This represents
// one individual element in the PseudoRelocTableChunk table.
```


================
Comment at: COFF/SymbolTable.cpp:197
 
+    if (Config->MinGW && !Name.startswith("__imp_")) {
+      DefinedImportData *Imp =
----------------
ruiu wrote:
> This function is already too long. Please consider splitting to a new function.
Done, split the extra new added code to a separate method.


================
Comment at: COFF/Writer.cpp:1148
 
+void Writer::createRuntimePseudoRelocs() {
+  for (Chunk *C : Symtab->getChunks()) {
----------------
ruiu wrote:
> Please add a comment.
I wrote:
```
// MinGW specific. Gather all relocations that are imported from a DLL even
// though the code didn't expect it to, produce the table that the runtime
// uses for fixing them up, and provide the synthetic symbols that the
// runtime uses for finding the table.
```


https://reviews.llvm.org/D50917