[PATCH] D28617: Implement -Map

[Rafael Espíndola via llvm-commits]

On 12 January 2017 at 15:43, Rui Ueyama via Phabricator
<reviews at reviews.llvm.org> wrote:
> 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.

The bfd format seems pretty attached to the fact that bfd ld always
works with a linker script. I have attached what it produces for this
testcase. If you ignore the linker script dump, what this prints is
similar to the "Linker script and memory map" section. We can add more
(like the LOAD segments in a followup patch).

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

Will add.

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

Will do.

>
> ================
> 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?

It is used in 3 places in the function, so it is convenient.

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

Yes, FileOutputBuffer should probably be split in two. One just
provides the temp file management, the other add the resize+mmap.

>
> ================
> 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?

My preference would be to make the llvm headers less verboses if
possible, but aggregating the small ones is probably also fine and far
easier.

Cheers,
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: map
Type: application/octet-stream
Size: 9359 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170112/569cc323/attachment.obj>