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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Thank you for the patch. Overall, I think it is a good thing to add link.exe-compatible /map file output to lld.



================
Comment at: lld/COFF/CMakeLists.txt:18-19
   LTO.cpp
   MapFile.cpp
+  MapFileMS.cpp
   MarkLive.cpp
----------------
Except a few features, everything in lld/COFF is Microsoft-compatible, so adding a MS suffix feels a bit odd to me. You probably should rename existing MapFile.cpp to LLDMapFile.cpp and then place your file as MapFile.cpp.


================
Comment at: lld/COFF/Config.h:185
   // Used for /lldmap.
   std::string mapFile;
 
----------------
Ditto -- rename this to lldMapFile and name yours mapFile


================
Comment at: lld/COFF/Driver.cpp:718
   StringRef outFile = config->outputFile;
+  return (outFile.substr(0, outFile.rfind('.')) + ".lldmap").str();
+}
----------------
We should avoid changing the suffix because it breaks backward compatibility. Instead, you may want to add a warning if /map and /lldmap has the same filename as an output file.


================
Comment at: lld/COFF/MapFileMS.cpp:9
+//
+// This file implements the /map option in a format close to the one generated
+// by link.exe (based on observations)
----------------
It is a bit odd that we intend to print out in a format that is not exactly the same as Microsoft's. What is the difference? Can't we output the exact same output?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70557