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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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
----------------
shenhan wrote:
> grimar wrote:
> > shenhan wrote:
> > > grimar wrote:
> > > > 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.
> > > Thanks for pointing this out, yes, I agree we need to have consensus on this (and the "out-of-order.s" test case below).
> > > 
> > > Here is my thoughts:
> > > 1. The test writer assume the fact that "foo" must be the first section in its segment (which is a group of output sections), otherwise, its address intersects with the first output section in the segment, because DOT is set to 0 for "foo" by linkerscript.
> > > 
> > > 2. There is no fact in the test case supports this assumption. 
> > > 
> > > 3. So this test case might cause linker error whenever output sections change relative order in the segment.
> > > 
> > > So my suggestion:
> > > Honor the assumption, so instead of change the DOT for foo, change it for .dynsym (the first output section of segment):
> > > - # RUN: echo "SECTIONS {foo 0 : {*(foo*)} }" > %t.script
> > > + # RUN: echo "SECTIONS {.dynsym 0 : {*(.dynsym)} }" > %t.script 
> > > This reserves the origin test intention.
> > > 
> > > .OR.
> > > Always layout output sections with explicit address assignment first, then layout those without. So for this particular case, the origin output section order is:
> > >   .dynsym .dynstr foo
> > > We layout it like below:
> > >   foo .dynsym .dynstr
> > > 
> > > .OR.
> > > Report error on rewinding "." to a conflict address.
> > > 
> > > [This also applies to out-of-order.s test case below.]
> > > 
> > > I'm happy to go any direction.
> > > 
> > > Comments?
> > > 
> > > 
> > > 
> > > 
> > > .OR.
> > > Always layout output sections with explicit address assignment first
> > 
> > I believe it is the correct way. A script should specify the layout, any orphans
> > should be placed after the explicitly specified sections. That seems logically correct
> > and consistent with gnu linkers behavior (I think).
> > 
> > I would really want to hear from other people before proceeding this place probably.
> Thanks, George.
> 
> @ruiu Can you comment?
Can you update the test cases you had to change so that these sections be not the orphans (will be listed explicitly)?
Let's see if we can try to go with that change then and watch how it works in practice.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45788