[PATCH] D75966: [ELF] Move --print-map(-M)/--cref before checkSections() and openFile()

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/Writer.cpp:598
 
+  // Handle --print-map(-M)/--Map and --cref. Dump them before checkSections()
+  // because the files may be useful in case checkSections() or openFile()
----------------
psmith wrote:
> In general I think moving the map file before writing is a good thing, especially when trying to debug problems. Arm's proprietary linker has done something similar to this for a while, it will attempt to print the map file whenever an error has been detected, once a certain stage in the link has passed. 
> My experience from that:
> - There are sometimes problems when the map file crashes due some information not being available at the time, or not being valid. I don't think the risk is high with this implementation. Is it possible checkSections() might find something that would crash the map file printer? Does the map file writing code need to guard against invalid sections for example?
> - It sometimes gives the impression that the link has succeeded even when it has failed, especially if the error message comes before the map file. If it isn't obvious the link has failed we may need to make it so.  
Thanks for sharing the information about Arm's proprietary linker!

> There are sometimes problems when the map file crashes due some information not being available at the time, or not being valid.

After finalizeAddressDependentContext() (addresses and symbol assignments) and assignFileOffsets (file offsets), all the information for -M is available. test/ELF/map-file.s and test/ELF/linkerscript/map-file.s  make sure we don't run into a missing information problem.

checkSections() just checks whether sections overlap or exceed the address space limit for ELFCLASS32. It does not make any field of a section/symbol assignment invalid. It is safe to move -M before checkSections().

> It sometimes gives the impression that the link has succeeded even when it has failed, especially if the error message comes before the map file. If it isn't obvious the link has failed we may need to make it so.

The error status makes it clear the the link has failed. In addition, the error message is still kept.


================
Comment at: lld/test/ELF/linkerscript/output-too-large.s:8
+# RUN: not ld.lld -T %t1.script %t1.o -M -o /dev/null 2>&1 | \
+# RUN:   FileCheck --check-prefixes=MAP1 %s
+
----------------
grimar wrote:
> `MAP1,CHECK`?
`error: output file too large` is not printed for this test.


================
Comment at: lld/test/ELF/linkerscript/output-too-large.s:13
+# MAP1-NEXT:        0        0 ffffffff     1         . = 0xffffffff
+# MAP1-NEXT: 100000000 100000000        1     4         {{.*}}.o:(.text)
+# MAP1:      error: section .text at 0x0 of size 0x100000001 exceeds available address space
----------------
grimar wrote:
> The "align" calues are misaligned vertically.
This is how it gets printed:/

For an ELFCLASS32 output, lld thinks 8 characters are sufficient for an address. Here the address gets overflowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75966