Re: [PATCH] gold: -Trodata-segment and fix -Ttext-segment for isolate_execinstr targets

[Cary Coutant <ccoutant at google dot com>]

> Second, it adds -Trodata-segment with the same meaning it has to BFD ld.
> This has no effect unless either --rosegment is also given, or
> target->isolate_execinstr() returns true.  I couldn't figure out where
> to add option sanity checking that would complain if you passed
> -Trodata-segment when it will be ignored, because it has to be done
> someplace where the target has already been selected.

You can put a check in Parameters::set_target_once, which will be
called as soon as we have a target. Notice how
check_target_endianness() is called both there and in
Parameters::set_options (where it will usually not run because the
target usually hasn't been set yet).

> gold/
> 2013-07-18  Roland McGrath  <mcgrathr@google.com>
>
>         * options.h (General_options): Add -Trodata-segment option.
>         * layout.cc (relaxation_loop_body): Honor -Trodata-segment option.
>         If target->isolate_execinstr(), don't expect executable segment to
>         come first.

Missing a ChangeLog entry for Layout::set_segment_offsets.

> --- a/gold/layout.cc
> +++ b/gold/layout.cc
> @@ -2424,8 +2424,12 @@ Layout::relaxation_loop_body(
>    // If the user set the address of the text segment, that may not be
>    // compatible with putting the segment headers and file headers into
>    // that segment.

Please update this comment.

> -  if (parameters->options().user_set_Ttext()
> -      && parameters->options().Ttext() % target->abi_pagesize() != 0)
> +  if (target->isolate_execinstr()
> +      ? (parameters->options().user_set_Trodata_segment()
> +        && (parameters->options().Trodata_segment() % target->abi_pagesize()
> +            != 0))
> +      : (parameters->options().user_set_Ttext()
> +        && parameters->options().Ttext() % target->abi_pagesize() != 0))

Putting a ?: operator inside a conditional expression has the same
effect on me as fingernails on a blackboard. Is there some better way
to express this? Perhaps you could factor that out to a separate
inline helper function:

// The first segment is normally the text segment, but the rodata segment
// will come first on targets where rodata is placed in a separate segment.
// This function returns false if the user has set the address of that segment
// such that the phdrs cannot be placed in it.

static inline bool
can_put_phdrs_in_first_segment(const Target* target)
{
  const General_options& options = parameters->options();
  if (target->isolate_execinstr())
    return (options.user_set_Trodata_segment()
            && options.Trodata_segment() % target->abi_pagesize() != 0);
  else
    return (options.user_set_Ttext()
            && options.Ttext() % target->abi_pagesize() != 0);
}

Then just test for
  if (!can_put_phdrs_in_first_segment(target))
    ...

>      {
>        load_seg = NULL;
>        phdr_seg = NULL;
> @@ -3411,10 +3415,18 @@ Layout::set_segment_offsets(const Target*
> target, Output_segment* load_seg,
>             }
>           else if (parameters->options().user_set_Ttext()
>                    && (parameters->options().omagic()
> -                      || ((*p)->flags() & elfcpp::PF_W) == 0))
> +                      || (target->isolate_execinstr()
> +                          ? ((*p)->flags() & elfcpp::PF_X) != 0
> +                          : ((*p)->flags() & elfcpp::PF_W) == 0)))

Same here:

// On targets where the rodata is placed in a separate segment, it is not
// sufficient to check the writable flag.

static inline bool
is_text_segment(const Target* target, Segment_list::iterator p)
{
  if (target->isolate_execinstr())
    return ((*p)->flags() & elfcpp::PF_X) != 0;
  else
    return ((*p)->flags() & elfcpp::PF_W) == 0;
}

Shouldn't we still require PF_W to be 0 in either case? I don't think
we'd want a PF_W|PF_X segment to qualify as the text segment.

-cary