[PATCH] D28617: Implement -Map

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Is there any reason we can't print out in the exact same format as ld.bfd? Because LLD is basically a drop-in replacement, compatible output would be generally appreciated.



================
Comment at: lld/ELF/MapFile.cpp:7
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
----------------
This needs a file comment. At least it should mention that this is for -Map and what kind of information is printed, at the granularity of the ld man page.


================
Comment at: lld/ELF/MapFile.cpp:24-27
+  OS << format_hex_no_prefix(Address, 8) << ' ';
+  OS << format_hex_no_prefix(Size, 8) << ' ';
+  OS << format("%5x ", Align);
+  OS << left_justify(Name, 7);
----------------
nit: maybe this style is more common?

  OS << ...
     << ...;

as opposed to

  OS << ...;
  OS << ...;


================
Comment at: lld/ELF/MapFile.cpp:99
+void elf::writeMapFile(ArrayRef<OutputSectionBase *> OutputSections) {
+  StringRef MapFile = Config->MapFile;
+  if (MapFile.empty())
----------------
Do you need this temporary variable?


================
Comment at: lld/ELF/MapFile.cpp:105-112
+  int FD;
+  std::error_code EC =
+      sys::fs::createUniqueFile(Twine(MapFile) + ".tmp%%%%%%%", FD, TempPath);
+  if (EC)
+    fatal(EC.message());
+  FileRemover RAII(TempPath);
+  writeMapFile2<ELFT>(FD, OutputSections);
----------------
This seems like a pretty common pattern. We probably should have a class that does this (open with a temporary file name and rename on close) automatically.


================
Comment at: lld/ELF/MapFile.h:1
+//===- MapFile.h ------------------------------------------------*- C++ -*-===//
+//
----------------
Not directly related to this patch, but we have many instances of this kind of "effectively one line" .h files in LLD. In some case, some declarations are piggy backed to a somewhat unrelated header (e.g. markLive's declaration is in Writer.h.)

I once wondered if we should create a header for miscellaneous functions, say, LLD.h, to aggregate them. What do you think?


https://reviews.llvm.org/D28617