[PATCH] D45788: Mitigate relocation overflow [part 1 of 2]

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Ok, I reviewed the changed test cases and unfortunately, now I have many concerns about this patch.
Issues I found are in comments, I think they are critical and needs fix/investigation first of all.

But I also think that given the issues observed and the fact that it makes
output different with the default output from gnu linkers,
I probably want to suggest to hide this new functionality under some -z <option> flag.

Will be happy to see the opinions of other people.



================
Comment at: test/ELF/arm-exidx-shared.s:44
 // CHECK-EXTAB: Contents of section .ARM.extab:
 // 014c + 0ee4 = 0x1030 = __gxx_personality_v0(PLT)
+// CHECK-EXTAB-NEXT:  01d8 580e0000 b0b0b000 00000000
----------------
This calculation comment needs an update since values changed.


================
Comment at: test/ELF/dynsec-at-beginning.s:13
+# CHECK-NEXT: .hash    {{.*}}   A
+# CHECK-NEXT: .text    {{.*}}   A
+
----------------
Please address this comment (.text is `AX`, not just `A`)


================
Comment at: test/ELF/linkerscript/no-space.s:4
 
-# RUN: echo "SECTIONS {foo 0 : {*(foo*)} }" > %t.script
+# RUN: echo "SECTIONS {foo 30 : {*(foo*)} }" > %t.script
 # RUN: ld.lld --hash-style=sysv -o %t --script %t.script %t.o -shared
----------------
I do not think this is correct, sorry. This change broke the intention of the test case I believe.

You did that because LLD starts triggering an error:

```
error: section .dynsym file range overlaps with foo
>>> .dynsym range is [0x1000, 0x1017]
>>> foo range is [0x1000, 0x1007]
```

What happens because orphan section `.dynsym` is placed before `foo`. And then during
assigning of VA, `Dot` is explicitly set to 0 for `foo`, making it intersect with .dynsym.

I think the expected behavior is to not place these orphan sections before `foo`, since
the position of `foo` is explicitly requested by linker script. And sure I do not think
we should emit an error in that case.

This case needs some investigation.
I'll be happy to hear other people opinion on that too.


================
Comment at: test/ELF/linkerscript/out-of-order.s:3
 # RUN: llvm-mc -filetype=obj -triple=x86_64-linux %s -o %t.o
-# RUN: echo "SECTIONS { .data 0x4000 : { *(.data) } .text 0x2000 : { *(.text) } }" > %t.script
+# RUN: echo "SECTIONS { .data 0x4000 : { *(.data) } .dynsym 0x2000 : { *(.dynsym)} }" > %t.script
 # RUN: ld.lld --hash-style=sysv -o %t.so --script %t.script %t.o -shared
----------------
The same here. I do not think you should "fix" this script. It was valid by itself. It should just work.

With the original linker script, this patch makes LLD to produce invalid output (actually no output at all as FileSize is broken).


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45788