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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

This patch looks okay to me though it is unfortunate that we need to support it. This patch overall lacks explanation -- at somewhere in the patch, I'd explain what is automatic import and how lld supports that functionality. Once you get an overview, details should hopefully become self-explanatory.



================
Comment at: COFF/Chunks.cpp:422
 
+static int getRuntimePseudoRelocSize(uint16_t Type) {
+  // Relocations that either contain an absolute address, or a plain
----------------
It's not clear to me what this function is supposed to return in the first place. Could you write a brief function comment?


================
Comment at: COFF/Chunks.cpp:493
+
+void SectionChunk::getRuntimePseudoRelocs(
+    std::vector<RuntimePseudoReloc> &Res) {
----------------
What this function is doing? Please write a brief function comment.


================
Comment at: COFF/Chunks.cpp:494
+void SectionChunk::getRuntimePseudoRelocs(
+    std::vector<RuntimePseudoReloc> &Res) {
+  for (const coff_relocation &Rel : Relocs) {
----------------
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.


================
Comment at: COFF/Chunks.cpp:631
 
+size_t PseudoRelocTableChunk::getSize() const {
+  if (Relocs.empty())
----------------
There's no explanation of "pseudo relocation". Please explain it somewhere.


================
Comment at: COFF/Chunks.h:424
 
+class PseudoRelocTableChunk : public Chunk {
+public:
----------------
Please add comment to explain what this is.


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


================
Comment at: COFF/Chunks.h:445
+
+class RuntimePseudoReloc {
+public:
----------------
Ditto


================
Comment at: COFF/SymbolTable.cpp:197
 
+    if (Config->MinGW && !Name.startswith("__imp_")) {
+      DefinedImportData *Imp =
----------------
This function is already too long. Please consider splitting to a new function.


================
Comment at: COFF/Writer.cpp:1148
 
+void Writer::createRuntimePseudoRelocs() {
+  for (Chunk *C : Symtab->getChunks()) {
----------------
Please add a comment.


https://reviews.llvm.org/D50917