[ELF] - Disable emiting multiple output sections when merging is disabled.

[grimar]

Summary

When -O0 is specified, we do not do section merging.
Though before this patch several sections were generated instead
of single, what is useless I think.

This is important for current testing of FreeBSD bootstrap.
Particulaty for EFI loader. Since allows to get closer to output of
gnu linkers when needed.

[ruiu]

I think I do not understand what this patch is trying to address. Did you mean that FreeBSD EFI loader passes -O0 to linker?

[grimar]

In D27041#604490, @ruiu wrote:

I think I do not understand what this patch is trying to address. Did you mean that FreeBSD EFI loader passes -O0 to linker?

No it doesn't. My point is next:

For linkerscript we take "M" and "S" flags in account:

uintX_t Flags = C->Flags & (SHF_MERGE | SHF_STRINGS);

That is done because currently our implementation requires one output section per "kind",
what is different from what gnu linkers do.

With -O0 sections metging is disabled:

bool elf::ObjectFile<ELFT>::shouldMerge(const Elf_Shdr &Sec) {
  if (Config->Optimize == 0)
    return false;

In that case there is no point to create more output sections, we can combine them as regular into single section.
Currently that is the only way to get output similar to ld/gold. And currently that way broken.

Returning o EFI loader. Change above is useful for investigation of broken files, including the loader.
Currently LLD output for loader is:

[ 4] .data             PROGBITS         0000000000058000  00059000
      0000000000006355  0000000000000001 AMS       0     0     1
 [ 5] .got              PROGBITS         0000000000068520  0005f520
      00000000000004a0  0000000000000000  WA       0     0     8
 [ 6] .data             PROGBITS         000000000005e360  00055360
      000000000000a142  0000000000000000  WA       0     0     16
 [ 7] .data             PROGBITS         00000000000684b0  0005f4b0
      0000000000000051  0000000000000001 AMS       0     0     16
 [ 8] .data             PROGBITS         0000000000068502  0005f502
      0000000000000016  0000000000000002 AMS       0     0     2
 [ 9] .data             PROGBITS         0000000000068518  0005f518

.got inside .data is fixed by D27040, but there are still multiple .data sections.
That might be a problem and might be not. It is unclear now if EFI firmware
accepts file with multiple data or if we can have any other problems at other points like
if objcopy fine to convert this to EFI PE format.

So I believe the behavior of this patch is:

1. Fixes bug in output (no need to produce more sections that we can and take in account flags that are unused).
2. Might be helpfull for investigation such issues like above in FreeBSD at least.

[grimar]

To be more clear currently lld linked loader just hangs under QEMU. After D27040 situation changes, so now QEMU crashes when trying to use it.
It is unclear if this patch changes anything yet, just adds a one more button to trigger during investigation.

[ruiu]

ELF/LinkerScript.cpp

280 ↗

(On Diff #79060)

I don't think this is the right place to fix it.

If shouldMerge() returns false, we should override Flags so that it doesn't have SHF_MERGE|SHF_STRINGS.

Then naturally they'll be linked as if they were not mergeable.

[grimar]

ELF/LinkerScript.cpp

280 ↗

(On Diff #79060)

Ok, that sounds reasonable.

[grimar]

• Addressed review comments.

[ruiu]

ELF/InputSection.cpp

60–62 ↗

(On Diff #79289)

We shouldn't implement the same logic again here. You want to change this line https://github.com/llvm-mirror/lld/blob/d93aa5ab9ac56831d625a0233a0e17f482822d07/ELF/InputSection.cpp#L208to turn off SHF_MERGE and SHF_STRINGS flags unconditionally.

[grimar]

ELF/InputSection.cpp

60–62 ↗

(On Diff #79289)

That will not work unfortunately.
Next code is involved here and not that line:

if (shouldMerge(Sec))
  return make<MergeInputSection<ELFT>>(this, &Sec, Name);
return make<InputSection<ELFT>>(this, &Sec, Name);

calls:

template <class ELFT>
InputSection<ELFT>::InputSection(elf::ObjectFile<ELFT> *F,
                                 const Elf_Shdr *Header, StringRef Name)
    : InputSectionBase<ELFT>(F, Header, Name, Base::Regular) {}

So the suggested change will not take any affect on .data

[ruiu]

ELF/InputSection.cpp

60–62 ↗

(On Diff #79289)

But implementing the same logic here is not right.

91 ↗

(On Diff #79289)

How about this. You could add code here to drop the flag.

// If it is not a mergeable section, overwrite the flag so that the flag
// is consistent with the class. This inconsistency could occur when
// string merging is disabled using -O0 flag.
if (!isa<MergeInputSection<ELFT>>(this))
  Flags &= ~(SHF_MERGE | SHF_STRINGS);

[grimar]

• Addressed comments.

ELF/InputSection.cpp

91 ↗

(On Diff #79289)

This works for me with minor modification, thanks.

[ruiu]

LGTM

lld/trunk/ELF/InputSection.cpp

if (!isPowerOf2_64(V))
     fatal(toString(File) + ": section sh_addralign is not a power of 2");
 
   // We reject object files having insanely large alignments even though
   // they are allowed by the spec. I think 4GB is a reasonable limitation.
   // We might want to relax this in the future.
   if (V > UINT32_MAX)
     fatal(toString(File) + ": section sh_addralign is too large");
   Alignment = V;
+
+  // If it is not a mergeable section, overwrite the flag so that the flag
+  // is consistent with the class. This inconsistency could occur when
+  // string merging is disabled using -O0 flag.
+  if (!Config->Relocatable && !isa<MergeInputSection<ELFT>>(this))
+    this->Flags &= ~(SHF_MERGE | SHF_STRINGS);
 }
 
 template <class ELFT>
 InputSectionBase<ELFT>::InputSectionBase(elf::ObjectFile<ELFT> *File,
                                          const Elf_Shdr *Hdr, StringRef Name,
                                          Kind SectionKind)
     : InputSectionBase(File, Hdr->sh_flags, Hdr->sh_type, Hdr->sh_entsize,
                        Hdr->sh_link, Hdr->sh_info, Hdr->sh_addralign,

lld/trunk/test/ELF/merge-string.s

 // NOTAIL-NEXT: SectionData (
 // NOTAIL-NEXT:   0000: 61626300 626300                     |abc.bc.|
 // NOTAIL-NEXT: )
 
 // NOMERGE:      Name:    .rodata
 // NOMERGE-NEXT: Type:    SHT_PROGBITS
 // NOMERGE-NEXT: Flags [
 // NOMERGE-NEXT:   SHF_ALLOC
-// NOMERGE-NEXT:   SHF_MERGE
-// NOMERGE-NEXT:   SHF_STRINGS
 // NOMERGE-NEXT: ]
 // NOMERGE-NEXT: Address:         0x1C8
 // NOMERGE-NEXT: Offset:  0x1C8
 // NOMERGE-NEXT: Size:    16
 // NOMERGE-NEXT: Link: 0
 // NOMERGE-NEXT: Info: 0
 // NOMERGE-NEXT: AddressAlignment: 2
 // NOMERGE-NEXT: EntrySize: 0

lld/trunk/test/ELF/no-merge.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS { .data : {*(.data.*)} }" > %t0.script
+# RUN: ld.lld %t.o -o %t0.out --script %t0.script
+# RUN: llvm-objdump -s %t0.out | FileCheck %s --check-prefix=OPT
+# OPT:      Contents of section .data:
+# OPT-NEXT:   0000 01
+# OPT-NEXT: Contents of section .data:
+# OPT-NEXT:   0001 6100
+# OPT-NEXT: Contents of section .data:
+# OPT-NEXT:   0003 03
+
+# RUN: ld.lld -O0 %t.o -o %t1.out --script %t0.script
+# RUN: llvm-objdump -s %t1.out | FileCheck %s --check-prefix=NOOPT
+# NOOPT:      Contents of section .data:
+# NOOPT-NEXT:   0000 01610003
+
+.section .data.aw,"aw",@progbits
+.byte 1
+
+.section .data.ams,"aMS",@progbits,1
+.asciz "a"
+
+.section .data.am,"aM",@progbits,1
+.byte 3