[PATCH] D55423: [LLD][ELF] - A fix for "linker script assignment loses relative nature of section" bug.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: ELF/Relocations.cpp:387-388
+  // They always can be computed as a link time constant.
+  if (Sym.ScriptDefined)
+    return true;
+
----------------
peter.smith wrote:
> grimar wrote:
> > nickdesaulniers wrote:
> > > @peter.smith suggested that just this part of this patch is what I need in order to link an arm64 Linux kernel with KASLR enabled with LLD (https://bugs.llvm.org/show_bug.cgi?id=39810#c11).
> > > 
> > > I was able to confirm that this change was all that I needed to link a bootable kernel image.
> > > 
> > > Can I please have just this change split out of the patch?  Surely this part is less controversial?
> > If nobody will so that earlier, I'll take a look at how to extract this part with an appropriate test case tomorrow.
> > 
> > (Though I still think we just need to land D55550, and then this part. Having a different behavior of linker script on different architectures is just weird).
> The pr contains an AArch64 version of the test. It should fail with a link error with -pie. FWIW I agree with George that D55550 is worth landing as well. Linker scripts are often generated from a large set of macros and it isn't always easy to hand edit the files to avoid forward references.
So I ended up with the following test case:

```
# REQUIRES: aarch64
# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %s -o %t.o
# RUN: echo "aliasto__text = __text; SECTIONS { .text 0x1000 : { __text = . ; *(.text) } }" > %t.script
# RUN: ld.lld -pie -o %t --script %t.script %t.o
# RUN: llvm-readobj -symbols %t | FileCheck %s

## Check that alias 'aliasto__text' has the correct value.
## (It should belong to the section .text and point to it's start).

## TODO: This could be a test for x86 platform, but currently, LLD
## has an inconsistent behavior of the linker script between x86 and aarch64.
## Under x86, `aliasto__text` would not get the correct value and test would fail.

# CHECK:      Symbol {
# CHECK:        Name: __text
# CHECK-NEXT:   Value: 0x1000
# CHECK-NEXT:   Size: 0
# CHECK-NEXT:   Binding: Global
# CHECK-NEXT:   Type: None
# CHECK-NEXT:   Other: 0
# CHECK-NEXT:   Section: .text
# CHECK-NEXT: }

# CHECK:      Symbol {
# CHECK:        Name: aliasto__text
# CHECK-NEXT:   Value: 0x1000
# CHECK-NEXT:   Size: 0
# CHECK-NEXT:   Binding: Global
# CHECK-NEXT:   Type: None
# CHECK-NEXT:   Other: 0
# CHECK-NEXT:   Section: .text
# CHECK-NEXT: }

.text
.globl _start
.type _start, %function
_start:
.globl aliasto__text
  bl __text
  bl aliasto__text
```

I.e. it still uses forward declaration for `aliasto__text`. I found no way to make this piece of the code be useful without
doing that.

Rui's comment above says:
"I think I'm not still convinced that we should support forward declaration. How often do you really need that? Also, how is it hard/easy to detect if something is a forward reference? How much easy/hard to fix your linker scripts if yours is already using that pattern?
I believe we should at least ban circular definitions such as foo=bar; bar=foo; because it doesn't make any sense."

I.e. he was against doing it that time. My conclusion remains the same: if you want to support such forward declarations we want to land D55550 + rebased version of this patch to avoid mess about different behavior between aarch64 and x86 and fix it all at once. I do not see for what another case this piece can be useful. (If it is useful for some other case, please let me know, then it would be probably possible to justify such change). 


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

https://reviews.llvm.org/D55423