[PATCH] D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR

[Peter Smith via llvm-commits]

I'm not going to get a chance to upload a new patch today, but I've
run some experiments. It looks like renaming the section to
.data.rel.ro.bss but making no other changes has the following effect
- The default ldscript from bfd is happy to match this in
.data.rel.ro, the whole of the contents of the section is emitted in
the file. The output at least for my simple examples is working.
- The OutputSection name for the non-linker script case is
.data.rel.ro as this matches the pattern .data.rel.ro.* . If this is
the only .rel.ro then it will be output as NOBITS (pt_memsz only)  if
there is any other .data.rel.ro produced by the compiler/assembler
then our data.rel.ro.bss will be merged with that and be output as
PROGBITS.

I'm planning on updating the patch on Monday.

There is one more oddity to report, although it isn't related to
relro. In the linker script after the bss there is the following:
  .lbss   :
  {
    *(.dynlbss)
    *(.lbss .lbss.* .gnu.linkonce.lb.*)
    *(LARGE_COMMON)
  }
  . = ALIGN(64 / 8);
  . = SEGMENT_START("ldata-segment", .);
  .lrodata   ALIGN(CONSTANT (MAXPAGESIZE)) + (. & (CONSTANT
(MAXPAGESIZE) - 1)) :
  {
    *(.lrodata .lrodata.* .gnu.linkonce.lr.*)
  }
  .ldata   ALIGN(CONSTANT (MAXPAGESIZE)) + (. & (CONSTANT (MAXPAGESIZE) - 1)) :
  {
    *(.ldata .ldata.* .gnu.linkonce.l.*)
    . = ALIGN(. != 0 ? 64 / 8 : 1);
  }

We output a 0 sized section for .ldata after the bss, I'm guessing
because of the . = ALIGN(. != 0 ? 64 / 8 : 1);
This is sufficient to make all the NOBITS in the image be output as
PROGBITS as it gets added to the PHDR containing the RW and ZI. I have
no idea what .ldata is, but it may be worth taking a look at this as
it could make lld look bad in size comparisons if projects are using
ld.bfd derived linker scripts.

Peter

On 17 November 2017 at 01:53, Rui Ueyama <ruiu at google.com> wrote:
> On Fri, Nov 17, 2017 at 3:11 AM, Rafael Avila de Espindola
> <rafael.espindola at gmail.com> wrote
>>
>> My opinions on this so far are
>>
>> There should be just one PT_GNU_RELRO. We should try to get the dynamic
>> linkers changed only if there is a direct advantaged to having multiple
>> versus having the static linker putting the data in the right order.
>>
>> I am OK with producing an error if a linker script would have caused
>> mulitple PT_GNU_RELRO to be created.
>
>
> I'm okay with that too. It doesn't quite right to create a
> partially-protected binary when we need multiple RELROs. I'd choose one of
>
>  - reporting an error when an output needs more than oen RELRO,
>  - emitting a warning that a RELRO segment is not contiguous and don't
> create RELRO at all, or
>  - creating multiple RELROs
>
>> We can probably ignore empty sections when decided the PT_GNU_RELRO
>> start and end. This means we would have an error only when there is a ro
>> copy relocation.
>>
>> Can we rename the section to .data.rel.ro.bss but keep the current
>> flags or would bfd error out with bss data not in the end of a PT_LOAD?
>>
>> Cheers,
>> Rafael
>>
>> Peter Collingbourne via Phabricator <reviews at reviews.llvm.org> writes:
>>
>> > pcc added a comment.
>> >
>> > In https://reviews.llvm.org/D40029#927193, @peter.smith wrote:
>> >
>> >> Adding pcc as the original author of https://reviews.llvm.org/D28272.
>> >>
>> >> Peter, in  https://reviews.llvm.org/D28272#637284 you mention that
>> >> you'd add a follow up patch that used .data.rel.ro rather than .bss.rel.ro
>> >> for the copy relocations. I can't work out if that ever landed. Can you let
>> >> me know if did? I've just ran into a case where the assumptions made by a
>> >> ld.bfd linker script cause problems with the .bss.rel.ro name.
>> >
>> >
>> > No, the use of NOBITS is deliberate. See
>> > http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170102/417025.html
>> >
>> > Could we instead rename .bss.rel.ro to .data.rel.ro only if the linker
>> > script has a SECTIONS clause?
>> >
>> >
>> > https://reviews.llvm.org/D40029
>
>