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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Thanks for an update!

I tested check-lld both on windows and ubuntu and
there are still 6 test cases that are failing for me.
(I think that was true for previous iteration too).

50>  Failing Tests (6):
50>      lld :: ELF/aarch64-copy.s
50>      lld :: ELF/arm-copy.s
50>      lld :: ELF/map-file.s
50>      lld :: ELF/relocation.s
50>      lld :: ELF/version-script-extern.s
50>      lld :: ELF/x86-64-retpoline-znow-linkerscript.s

My suggestions about how we should adjust the test cases are also inline.
Let's discuss if anything I am suggesting does not look OK for some reason to you.



================
Comment at: test/ELF/linkerscript/no-space.s:10
+
+# RUN: echo "SECTIONS {.dynsym 0 : { *(.dynsym) } }" > %t.script
 # RUN: ld.lld --hash-style=sysv -o %t --script %t.script %t.o -shared
----------------
.dynstr is still an orphan.

Let's just list them explicitly here:
`SECTIONS {foo 0 : {*(foo*)} .dynsym : { *(.dynsym) } .dynstr : { *(.dynstr) } }`


================
Comment at: test/ELF/linkerscript/no-space.s:14
 
-# RUN: echo "SECTIONS {foo : {*(foo*)} }" > %t.script
+# RUN: echo "SECTIONS {.dynsym : {*(.synsym)} }" > %t.script
 # RUN: ld.lld --hash-style=sysv -o %t --script %t.script %t.o -shared
----------------
synsym -> dynsym

`SECTIONS {foo : {*(foo*)} .dynsym : { *(.dynsym) } .dynstr : { *(.dynstr) } }`


================
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
----------------
grimar wrote:
> 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).
I do not understand why you changed `.data` to `.dynamic` actually.
What your patch changed is the order of "a" sections. That breaks the current scripts
which previously would put dynsym/dynstr correctly, but now - don't.
And I think we should adjust only that part, no need to update anything else probably.

Lets do this:
`SECTIONS { .data 0x4000 : { *(.data) } .dynsym 0x2000 : { *(.dynsym ) } .dynstr : { *(.dynstr ) } }`

So what changed is that we explicitly start the second segment from `.dynsym` instead of `.text`,
so that '.dynsym'/`.dynstr` are not an orphans now, and that allows to place it correctly. Layout is then:

>   1 .data         00000008 0000000000004000 DATA
>   2 .dynamic      00000060 0000000000004008
>   3 .dynsym       00000018 0000000000002000
>   4 .dynstr       00000001 0000000000002018
>   5 .text         00000008 000000000000201c TEXT DATA




Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45788