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

[MaskRay]

Summary

-M output can be useful when diagnosing an "error: output file too large" problem (emitted in openFile()).

I just ran into such a situation where I had to debug an erronerous
Linux kernel linker script. It tried to create a file larger than
INT64_MAX bytes.

This patch could have helped https://bugs.llvm.org/show_bug.cgi?id=44715 as well.

[MaskRay]

Move -M/--cref before checkSections()

[grimar]

lld/test/ELF/linkerscript/output-too-large.s

7

I'd add comments for each test case.

8

MAP1,CHECK?

13

The "align" calues are misaligned vertically.

[psmith]

lld/ELF/Writer.cpp

598

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.

[MaskRay]

lld/ELF/Writer.cpp

598

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.

lld/test/ELF/linkerscript/output-too-large.s

8

error: output file too large is not printed for this test.

13

This is how it gets printed:/

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

[MaskRay]

I think Peter's suggestion is whether (1) we should do an automatic -M when the link fails (2) where we should do -M.

We could move -M earlier further, just after finalizeSections(), but these steps between finalizeSections() and checkSections rarely calls error.
I am still hesitant making -M an automatic thing. It can get in the way if I am diagnosing a problem with -t -y etc, then we will also need an option to turn off the automatic -M. We have --reproduce, so reproducing a link is really easy. I currently don't think an automatic -M is necessary.

[MaskRay]

Add comments to tests

[psmith]

I'm not suggesting we make -M display on every error. It was just mentioning experience of a linker that does do that when the map file is enabled, which approximates what you have here. I think this is good logical step to take, and if we need to alter it later after we gain experience we can do.

[MaskRay]

So, is the patch good to go as-is? :)

[psmith]

In D75966#1918075, @MaskRay wrote:

So, is the patch good to go as-is? :)

I'm happy, but better let George approve as he had some suggestions.

[grimar]

LGTM with 2 nits.

lld/test/ELF/linkerscript/output-too-large.s

7–23

Perhaps, "..if an address is greater..."?
I think an address always _can_, but we report the case when it _is_ :)

8

Then you should use --check-prefix, not --check-prefixes.

[MaskRay]

Address comments

lld/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::run() {
 
   for (Partition &part : partitions)
     setPhdrs(part);
 
   if (config->relocatable)
     for (OutputSection *sec : outputSections)
       sec->addr = 0;
 
+  // Handle --print-map(-M)/--Map and --cref. Dump them before checkSections()
+  // because the files may be useful in case checkSections() or openFile()
+  // fails, for example, due to an erroneous file size.
+  writeMapFile();
+  writeCrossReferenceTable();
+
   if (config->checkSections)
     checkSections();
 
   // It does not make sense try to open the file if we have error already.
   if (errorCount())
     return;
   // Write the result down to a file.
   openFile();
template <class ELFT> void Writer<ELFT>::run() {
   }
 
   // Backfill .note.gnu.build-id section content. This is done at last
   // because the content is usually a hash value of the entire output file.
   writeBuildId();
   if (errorCount())
     return;
 
-  // Handle -Map and -cref options.
-  writeMapFile();
-  writeCrossReferenceTable();
-  if (errorCount())
-    return;
-
   if (auto e = buffer->commit())
     error("failed to write to the output file: " + toString(std::move(e)));
 }
 
 static bool shouldKeepInSymtab(const Defined &sym) {
   if (sym.isSection())
     return false;
 

lld/test/ELF/linkerscript/output-too-large.s

 # REQUIRES: x86
 
-# RUN: llvm-mc -filetype=obj -triple=i686-unknown-linux %s -o %t.o
-# RUN: echo "SECTIONS { .text : { . = 0xffffffff; *(.text*); } }" > %t.script
-# RUN: not ld.lld --no-check-sections --script %t.script %t.o -o /dev/null 2>&1 | FileCheck %s
-
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
-# RUN: echo "SECTIONS { .text : { . = 0x8fffffffffffffff; *(.text*); } }" > %t.script
-# RUN: not ld.lld --no-check-sections --script %t.script %t.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: llvm-mc -filetype=obj -triple=i686 %s -o %t1.o
+# RUN: echo "SECTIONS { .text : { . = 0xffffffff; *(.text*); } }" > %t1.script
+# RUN: not ld.lld --no-check-sections -T %t1.script %t1.o -o /dev/null 2>&1 | FileCheck %s
+
+# RUN: not ld.lld -T %t1.script %t1.o -M -o /dev/null 2>&1 | \
+# RUN:   FileCheck --check-prefixes=MAP1 %s
+
+# MAP1:           VMA      LMA     Size Align Out     In      Symbol
+# MAP1-NEXT:        0        0 100000001     4 .text
+# 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
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t2.o
+# RUN: echo "SECTIONS { .text : { . = 0x8fffffffffffffff; *(.text*); } }" > %t2.script
+# RUN: not ld.lld -T %t2.script -M %t2.o -o /dev/null 2>&1 | \
+# RUN:   FileCheck --check-prefixes=MAP2,CHECK %s
+
+# MAP2:                   VMA              LMA     Size Align Out     In      Symbol
+# MAP2:      9000000000000000 9000000000000000        1     4         {{.*}}.o:(.text)
+# MAP2-NEXT: 9000000000000000 9000000000000000        0     1                 _start
 
 # CHECK: error: output file too large
 
 .global _start
 _start:
   nop