[ELF] - Do not use mergeable sections when LS is used.

[grimar]

Summary

After latest changes we combine input sections with different attributes into
single output section.
Problem here is that regular output sections does not
support adding mergeable input sections (and vise versa).
Patch just disables merging for now at the same way we do for -O0 for example.

This change helps for linking FreeBSD kernel.

[davide]

This seems to me just a way to avoid a bug, that needs to be fixed.

[grimar]

In D23447#513725, @davide wrote:

This seems to me just a way to avoid a bug, that needs to be fixed.

We use the same for -O0 currently. Of cource it should be implemented, but for now that is easy way to go forward in supporting LS.

[davide]

"This change helps for linking FreeBSD kernel."

Can you please elaborate? Have you tried booting and you are able to boot a running kernel with this change, and without you're not able to?

[davide]

In D23447#513726, @grimar wrote:

In D23447#513725, @davide wrote:

This seems to me just a way to avoid a bug, that needs to be fixed.

We use the same for -O0 currently. Of cource it should be implemented, but for now that is easy way to go forward in supporting LS.

That's a completely different story. -O0 has an explicit semantic which is "linker please do not optimize my code".
Here we're silently skipping the optimization because nobody implemented the correct support for it. So, sure, you can do this as temporary workaround (although I disagree because it hides a bug), but I wouldn't say that "we do this at -O0" is a justification for this or being sloppy.

[grimar]

In D23447#513727, @davide wrote:

"This change helps for linking FreeBSD kernel."

Can you please elaborate? Have you tried booting and you are able to boot a running kernel with this change, and without you're not able to?

I did not try booting yet, because there are still problems with linking. Without this change linker just crashes in

void OutputSection<ELFT>::addSection(InputSectionBase<ELFT> *C)
..
auto *S = cast<InputSection<ELFT>>(C); // Here

because we are trying to add MergeInputSection.

After that change in combination with other patches that are on review at least lld completes linking. And there are post-link issues still:

[grimar]

BFD: stdON5Bm: section `.eh_frame' can't be allocated in segment 4
objcopy: stdON5Bm: Bad value
BFD: stdON5Bm: section `.eh_frame' can't be allocated in segment 4

[grimar]

In D23447#513728, @davide wrote:

In D23447#513726, @grimar wrote:

In D23447#513725, @davide wrote:

This seems to me just a way to avoid a bug, that needs to be fixed.

We use the same for -O0 currently. Of cource it should be implemented, but for now that is easy way to go forward in supporting LS.

That's a completely different story. -O0 has an explicit semantic which is "linker please do not optimize my code".
Here we're silently skipping the optimization because nobody implemented the correct support for it. So, sure, you can do this as temporary workaround (although I disagree because it hides a bug), but I wouldn't say that "we do this at -O0" is a justification for this or being sloppy.

I think "we do that for -O0" is a justification for correctness of the change. Main reason for this is to let kernel to link. More complex change will need more time for reviews and I think we can use this one as temporarily solution.

[davide]

In D23447#513731, @grimar wrote:

In D23447#513728, @davide wrote:

In D23447#513726, @grimar wrote:

In D23447#513725, @davide wrote:

This seems to me just a way to avoid a bug, that needs to be fixed.

We use the same for -O0 currently. Of cource it should be implemented, but for now that is easy way to go forward in supporting LS.

That's a completely different story. -O0 has an explicit semantic which is "linker please do not optimize my code".
Here we're silently skipping the optimization because nobody implemented the correct support for it. So, sure, you can do this as temporary workaround (although I disagree because it hides a bug), but I wouldn't say that "we do this at -O0" is a justification for this or being sloppy.

I think "we do that for -O0" is a justification for correctness of the change. Main reason for this is to let kernel to link. More complex change will need more time for reviews and I think we can use this one as temporarily solution.

No, it isn't. If any, the kernel link line should be changed to contain -O0 until proper support is implemented and not the other way around.

[davide]

Also, please note that I'm not entirely opposed with the workaround (although I don't agree with that either), so I'll defer the judgment to others. What kinda bothers me is your justification for this change, which does make absolutely no sense to me.

[grimar]

In D23447#513734, @davide wrote:

Also, please note that I'm not entirely opposed with the workaround (although I don't agree with that either), so I'll defer the judgment to others. What kinda bothers me is your justification for this change, which does make absolutely no sense to me.

Ok. Just in case I am also working on a real patch for this at this moment.
My point was that LS is still feature heavely under development and latest commits shows that sometimes we can use temporarily workarounds when we know it is not harmfull.

[ruiu]

Because currently our linker script support is incomplete and broken, it is okay to break something to fix something temporarily as long as we are making a progress. Speaking of this patch, it is okay to submit this and remove later, so please check this in if you want.

[ruiu]

But the problem you are trying to fix is a hard one in my opinion. Currently, we instantiate input sections when we read them from files, so the decision whether the sections are to be string-merged or not is made when we read files. That's too early if you allow linker scripts to combine non-mergeable sections with mergeable ones. I do not have a clear idea how to deal with this. If you have an idea, and if it's going to be a large change, please discuss before writing a patch.

[grimar]

In D23447#514076, @ruiu wrote:

But the problem you are trying to fix is a hard one in my opinion. Currently, we instantiate input sections when we read them from files, so the decision whether the sections are to be string-merged or not is made when we read files. That's too early if you allow linker scripts to combine non-mergeable sections with mergeable ones. I do not have a clear idea how to deal with this. If you have an idea, and if it's going to be a large change, please discuss before writing a patch.

Yep, I also today faced it is not trivial task. At first I thought it is possible to convert mergable section to regular, but probably better would be to create regular ones at the begining.
I have no good ideas how to do that without early sections scan. Will revisit this later.

lld/trunk/ELF/InputFiles.cpp

 //===- InputFiles.cpp -----------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "InputFiles.h"
 #include "Driver.h"
 #include "Error.h"
 #include "InputSection.h"
+#include "LinkerScript.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/CodeGen/Analysis.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/raw_ostream.h"

 
 template <class ELFT>
 bool elf::ObjectFile<ELFT>::shouldMerge(const Elf_Shdr &Sec) {
   // We don't merge sections if -O0 (default is -O1). This makes sometimes
   // the linker significantly faster, although the output will be bigger.
   if (Config->Optimize == 0)
     return false;
 
+  // We don't merge if linker script has SECTIONS command. When script
+  // do layout it can merge several sections with different attributes
+  // into single output sections. We currently do not support adding
+  // mergeable input sections to regular output ones as well as adding
+  // regular input sections to mergeable output.
+  if (ScriptConfig->HasContents)
+    return false;
+
   // A mergeable section with size 0 is useless because they don't have
   // any data to merge. A mergeable string section with size 0 can be
   // argued as invalid because it doesn't end with a null character.
   // We'll avoid a mess by handling them as if they were non-mergeable.
   if (Sec.sh_size == 0)
     return false;
 
   uintX_t Flags = Sec.sh_flags;

lld/trunk/test/ELF/linkerscript/linkerscript-merge-sections.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+# RUN: echo "SECTIONS { .foo : { *(.foo.*) } }" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -s %t1 | FileCheck %s
+# CHECK:      Contents of section .foo:
+# CHECK-NEXT:  0158 01000000 02000000 00000000 73686f72  ............shor
+# CHECK-NEXT:  0168 7420756e 7369676e 65642069 6e7400    t unsigned int.
+
+.global _start
+_start:
+  nop
+
+.section .foo.1, "aw"
+writable:
+ .long 1
+
+.section .foo.2, "aM",@progbits,1
+readable:
+ .long 2
+
+.section .foo.3, "awx"
+ .long 0
+
+.section .foo.4, "MS",@progbits,1
+.LASF2:
+ .string "short unsigned int"