[PATCH] D70557: [lld][COFF] Add support for /map

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking good.



================
Comment at: lld/COFF/Driver.cpp:1570-1571
+
+  if (!config->lldmapFile.empty() && !config->mapFile.empty() &&
+      config->lldmapFile == config->mapFile) {
+    warn("/lldmap and /map have the same output file '" + config->mapFile +
----------------
nit: You can omit `!config->mapFile.empty() &&` because if A==B and A=="", B must not be empty.

another nit: we usually do `A != ""` instead of `!A.empty` if A is StringRef.


================
Comment at: lld/COFF/Driver.cpp:1574
+         "'.\n>>> ignoring /lldmap");
+    config->lldmapFile.clear();
+  }
----------------
`config->lldmapFile == ""`


================
Comment at: lld/COFF/LLDMapFile.cpp:128
+} // namespace lld
\ No newline at end of file

----------------
nit: please make sure that a source file ends with `\n` instead of some other byte.


================
Comment at: lld/COFF/MapFile.cpp:76-77
+    // This can happen with symbols of Absolute kind
+    uint64_t rvaa = config->imageBase + a.first->getRVA();
+    uint64_t rvab = config->imageBase + b.first->getRVA();
+    return rvaa < rvab || (rvaa == rvab && a.second < b.second);
----------------
If you are adding `config->imageBase` to both A and B before comparing A < B, you don't have to add that at all, do you?


================
Comment at: lld/COFF/MapFile.cpp:225
+  } else {
+    time_t TDS = config->timestamp;
+    char FormattedTime[64] = {};
----------------
TDS -> tds, but I guess you can remove this local variable and use config->timestamp directly.


================
Comment at: lld/COFF/MapFile.cpp:226
+    time_t TDS = config->timestamp;
+    char FormattedTime[64] = {};
+    strftime(FormattedTime, 64, "%c", localtime(&TDS));
----------------
FormattedTime -> formattedTime


================
Comment at: lld/COFF/MapFile.cpp:227
+    char FormattedTime[64] = {};
+    strftime(FormattedTime, 64, "%c", localtime(&TDS));
+    os << " (" << FormattedTime << ")\n";
----------------
strftime's `%c` is locale-aware, so the exact output string of the function is not predictable. It may be in your local language (e.g. 2020年3月15日) depending on the language settings, and even for US English, I believe it's different depending on libc. That's not suitable for machine processing.

What is the output of MSVC link in the US English setting? We should always format a timestamp in that format whatever the user's language setting is.


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

https://reviews.llvm.org/D70557