Re: [patch][gold] Don't put rodata and text in the same segment.

[Ian Lance Taylor <iant at google dot com>]

Rafael Espindola <espindola@google.com> writes:

>> I think it would be OK to have an option to put read-only data in a
>> different segment.
>
> I see. The attached patch moves the it behind an option.
>
> 2010-09-01  Rafael Espindola  <espindola@google.com>
>
> 	* layout.cc (Layout::attach_allocated_section_to_segment): Add support
> 	for --rosegment.
> 	(Layout::find_first_load_seg): Search all segments to find the first
> 	one.
> 	(Layout::set_segment_offsets): Align the address of a read only segment
> 	that follows a read only one.
> 	* Makefile.am: Add a --rosegment bootstrap test.
> 	* Makefile.in: Regenerate.
> 	* options.h (rosegment): New.


> +# --rosegment bootstrap
> +ld1_ro_SOURCES = $(sources_var)
> +ld1_ro_DEPENDENCIES = $(deps_var) gcctestdir1/ld
> +ld1_ro_LDADD = $(ldadd_var)
> +ld1_ro_LDFLAGS = -Bgcctestdir1/ -Wl,--rosegment
> +
> +gcctestdir2-ro/ld: ld1-ro
> +	test -d gcctestdir2-ro || mkdir -p gcctestdir2-ro
> +	rm -f gcctestdir2/ld-ro
> +	(cd gcctestdir2-ro && $(LN_S) ../ld1-ro ld)
> +
> +ld2_ro_SOURCES = $(sources_var)
> +ld2_ro_DEPENDENCIES = $(deps_var) gcctestdir2-ro/ld
> +ld2_ro_LDADD = $(ldadd_var)
> +ld2_ro_LDFLAGS = -Bgcctestdir2-ro/ -Wl,--rosegment
> +
> +bootstrap-test-ro: ld2-ro
> +	rm -f $@
> +	echo "#!/bin/sh" > $@
> +	echo "cmp ld1-ro ld2-ro" > $@
> +	chmod +x $@
> +
> +# -r bootstrap

To be honest I don't think I want this additional test.  The bootstrap
tests are not the fastest.

> -	      if (was_readonly && ((*p)->flags() & elfcpp::PF_W) != 0)
> +	      if (was_readonly)
>  		{
> -		  if ((addr & (abi_pagesize - 1)) != 0)
> -		    addr = addr + abi_pagesize;
> +		  if (((*p)->flags() & elfcpp::PF_W) != 0)
> +		    {
> +		      if ((addr & (abi_pagesize - 1)) != 0)
> +			addr = addr + abi_pagesize;
> +		    }
> +		  else
> +		    addr = align_address(addr, abi_pagesize);

This does not look correct to me.  I think the existing computation is
always the one you want: addr = addr + abi_pagesize.  What you should
change here is to do that if any of the PF_R|PF_W|PF_X segment flags
change.  That is, change was_readonly to hold the flags of the previous
segment, and adjust addr if the new segment has different flags.

> +              N_(" Use a distinct segment for ro but not executable sections.")

How about

"Put read-only non-executable sections in their own segment"

Note that there should not be a period; the existing messages with
periods are incorrect.

Ian