[ELF] Bypass section type check #2

[evgeny777]

Summary

Addressed comments from Rafael

[bd1976llvm]

FWIW, I liked the spirit, if not the implementation, of your original change: https://reviews.llvm.org/D28761. To me it seems that LLD should do it's best to obey the linker script even if that means producing an elf that doesn't meet the elf standard (although I think it is a dubious call as to if the elf standard is really meant to be this strict). By all means issue a warning if the types or flags of sections don't match - but don't fail the link! This makes the tool simpler and able to cope with non-conforming input effectively.

[evgeny777]

I see. I'm not sure about the motivation behind those flag/type checks. @grimar has just found out that they prevent linux kernel from linking ....

[grimar]

In D29278#660494, @evgeny777 wrote:

I see. I'm not sure about the motivation behind those flag/type checks. @grimar has just found out that they prevent linux kernel from linking ....

Yes I tried to fix it in a some nice way at first, but found my solution will not work. So issue is next:

# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
# RUN: echo "SECTIONS { .notes : { *(.note.*) } }" > %t.script
# RUN: ld.lld --script %t.script --build-id %t.o -o %t

## Check that synthetic .note.gnu.build-id links fine with non-alloc note.
.section .note.1
.quad 0

Testcase above should work. Linux kernel contains script:

.notes : AT(ADDR(.notes) - 0xffffffff80000000) { __start_notes = .; *(.note.*) __stop_notes = .; } :text :note

and called with --build-id.

We create allocatable synthetic section .note.gnu.build-id and fail to combine it with other non allocatable sections .note.*
At first I tried to ignore check for synthetic input sections, but that will not work in general case.

Now I am not sure what is good solution for that.

[bd1976llvm]

I think these checks exist, because of this line from the gabi.

In the first phase, input sections that match in name, type and attribute flags should be concatenated into single sections.

However, it should be noted that this is followed by an note that states that linkers are only recommended to follow this approach.

AFAIR: at least gnu does not follow this rule. Also, I am aware of many codebases (games) with hand written asm which does not set the section flags correctly. This is not always easy to fix. Perhaps this asm is in a third party library etc..

Being permissive on this can be very helpful to users.

[bd1976llvm]

discussion continued via email:

Rafael Avila de Espindola <[e-mail]>
5:49 PM (15 hours ago)

to reviews+D29278., eleviant, ruiu, seifsta, me, llvm-commits, ikudrin.dev, grimar, adhemerval.zan. 
ben via Phabricator <[e-mail]> writes:

> bd1976llvm added a comment.
>
> I think these checks exist, because of this line from the gabi.
>
>   In the first phase, input sections that match in name, type and attribute flags should be concatenated into single sections.
>
> However, it should be noted that this is followed by an note that states that linkers are only **recommended ** to follow this approach.
>
> AFAIR: at least gnu does not follow this rule. Also, I am aware of many codebases (games) with hand written asm which does not set the section flags correctly. This is not always easy to fix. Perhaps this asm is in a third party library etc..
>
> Being permissive on this can be very helpful to users.

And that is the idea of the white list. It makes us as permissive as we
need. I also don't see the point for different levels depending on
linker scripts being used or not.

Cheers,
Rafael

bd1976 llvm <[e-mail]>
6:19 PM (15 hours ago)

to Rafael 
Unless your not permissive enough! Most permissive is just to obey the linker script and warn if doing so might cause issues. The tool gains more functionality and doesn't sacrifice safety. Linker script primacy FTW :)


Rafael Avila de Espindola
6:25 PM (14 hours ago)

to me 
That means we would never find out if something we are allowing is
needed and way. We should not extend the white list unless there is a
good reason for it.

lld/trunk/ELF/OutputSections.cpp

OutputSectionFactory<ELFT>::create(InputSectionBase<ELFT> *C,
   SectionKey Key = createKey(C, OutsecName);
   return create(Key, C);
 }
 
 static uint64_t getIncompatibleFlags(uint64_t Flags) {
   return Flags & (SHF_ALLOC | SHF_TLS);
 }
 
+// We allow sections of types listed below to merged into a
+// single progbits section. This is typically done by linker
+// scripts. Merging nobits and progbits will force disk space
+// to be allocated for nobits sections. Other ones don't require
+// any special treatment on top of progbits, so there doesn't
+// seem to be a harm in merging them.
+static bool canMergeToProgbits(unsigned Type) {
+  return Type == SHT_NOBITS || Type == SHT_PROGBITS || Type == SHT_INIT_ARRAY ||
+         Type == SHT_PREINIT_ARRAY || Type == SHT_FINI_ARRAY ||
+         Type == SHT_NOTE;
+}
+
 template <class ELFT>
 std::pair<OutputSectionBase *, bool>
 OutputSectionFactory<ELFT>::create(const SectionKey &Key,
                                    InputSectionBase<ELFT> *C) {
   uintX_t Flags = getOutFlags(C);
   OutputSectionBase *&Sec = Map[Key];
   if (Sec) {
     if (getIncompatibleFlags(Sec->Flags) != getIncompatibleFlags(C->Flags))
       error("Section has flags incompatible with others with the same name " +
             toString(C));
-    // Convert notbits to progbits if they are mixed. This happens is some
-    // linker scripts.
-    if (Sec->Type == SHT_NOBITS && C->Type == SHT_PROGBITS)
+    if (Sec->Type != C->Type) {
+      if (canMergeToProgbits(Sec->Type) && canMergeToProgbits(C->Type))
         Sec->Type = SHT_PROGBITS;
-    if (Sec->Type != C->Type &&
-        !(Sec->Type == SHT_PROGBITS && C->Type == SHT_NOBITS))
+      else
         error("Section has different type from others with the same name " +
               toString(C));
+    }
     Sec->Flags |= Flags;
     return {Sec, false};
   }
 
   uint32_t Type = C->Type;
   switch (C->kind()) {
   case InputSectionBase<ELFT>::Regular:
   case InputSectionBase<ELFT>::Synthetic:

lld/trunk/test/ELF/compatible-section-types.s

+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: ld.lld -shared %t.o -o %t
+// RUN: llvm-objdump -section-headers %t | FileCheck %s
+
+// CHECK: .foo {{0*}}28
+
+.section .foo, "aw", @progbits, unique, 1
+.quad 0
+
+.section .foo, "aw", @init_array, unique, 2
+.quad 0
+
+.section .foo, "aw", @preinit_array, unique, 3
+.quad 0
+
+.section .foo, "aw", @fini_array, unique, 4
+.quad 0
+
+.section .foo, "aw", @note, unique, 5
+.quad 0

lld/trunk/test/ELF/incompatible-section-types.s

-// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
-// RUN: not ld.lld -shared %t.o -o %t 2>&1 | FileCheck %s
-
-// CHECK: error: Section has different type from others with the same name {{.*}}incompatible-section-types.s.tmp.o:(.foo)
-
-.section .foo, "aw", @progbits, unique, 1
-.quad 0
-
-.section .foo, "aw", @init_array, unique, 2
-.quad 0