Re: [PATCH] [RFC] [GOLD] s390 -fsplit-stack support.

[Cary Coutant <ccoutant at gmail dot com>]

> The problem here is that I have to get to the proper place in .rodata
> (and mutate it), starting from the text section by parsing the
> relocation of the larl instruction aiming at it.  The parameters
> currently passed into do_calls_non_split unfortunately don't seem to
> allow this.

For the relocations, the Track_relocs class in reloc.h can simplify
the relocation tracking that you're doing. The initialize() method
requires an Object* and a section index. Unfortunately, we only have
the shndx of the data section, so we could also pass down the
reloc_shndx from do_relocate_sections, but Track_relocs::initialize()
is going to duplicate some work we've already done, so it would
probably make more sense to do what you've done and pass prelocs and
reloc_count down. We can add an overload for
Track_relocs::initialize() to bypass the overhead of fetching the
relocation section contents, like this:

template<int size, bool big_endian>
bool
Track_relocs<size, big_endian>::initialize(
    const unsigned char* prelocs,
    size_t reloc_count,
    unsigned int reloc_type)
{
  this->prelocs_ = prelocs;

  if (reloc_type == elfcpp::SHT_REL)
    this->reloc_size_ = elfcpp::Elf_sizes<size>::rel_size;
  else if (reloc_type == elfcpp::SHT_RELA)
    this->reloc_size_ = elfcpp::Elf_sizes<size>::rela_size;
  else
    gold_unreachable();

  this->len_ = this->reloc_size_ * reloc_count;
  return true;
}

That will allow you to replace this loop:

+      // Find out larl's operand.  It should be a local symbol in .rodata
+      // section.
+      const unsigned char *pr = prelocs;
+      for (size_t i = 0; i < reloc_count; ++i, pr += reloc_size)
+        {
...
+        }

with something like this:

       Track_relocs<size, big_endian> track_relocs;
       track_relocs.initialize(prelocs, reloc_count, elfcpp::SHT_RELA);
       track_relocs.advance(curoffset);
       ...

To get the view of the section containing the parameter block, I'd
suggest adding a new Object method:

[public:]
const unsigned char*
Object::get_output_view(Output_file* of, unsigned int shndx,
section_size_type* plen)
{ return this->do_get_output_view(of, shndx, plen); }

[protected:]
virtual const unsigned char*
Object::do_get_output_view(Output_file* of, unsigned int shndx,
  section_size_type* plen)
{ gold_unreachable(); }

const unsigned char*
Sized_relobj_file<size, big_endian>::do_get_output_view(
    Output_file* of,
    unsigned int shndx,
    section_size_type* plen)
{
  const Output_section* os = out_sections[shndx];
  if (os == NULL)
    return NULL;
  Address output_offset = out_offsets[shndx];

  // Assume we don't have to support post-processed sections.
  gold_assert(!os->requires_postprocessing());

  off_t output_section_offset = os->offset();
  gold_assert(output_section_offset != invalid_address);

  // Rather than look up the section header to fetch the sh_size field
  // of the input section, we'll punt and just return a view that extends
  // to the end of the output section.
  section_size_type view_size =
      convert_to_section_size_type(os->data_size() - output_offset);

  // Assume we don't have to support compressed sections.
  section_size_type uncompressed_size;
  gold_assert(!this->section_is_compressed(shndx, &uncompressed_size));

  *plen = view_size;
  return of->get_output_view(output_section_offset + output_offset, view_size);
}

To use this method, we'll need the Output_file* pointer, so we'll need
to pass that down from do_relocate_sections() as well.

I think it's reasonable to expect that a target implementation of
do_calls_non_split may need prelocs, reloc_count, and Output_file*, so
I don't have a problem adding those to the signatures of
Sized_relobj_file::split_stack_adjust,
Sized_relobj_file::split_stack_adjust_reltype,
Target::calls_non_split, and Target::do_calls_non_split.

> I have managed to get it working, but only by making a lot of private
> or protected things public, and changing some types.  In summary:
>
> - the private Views class used in reloc.cc is made public and routed
>   to do_calls_non_split.
> - Relobj's output_sections method is likewise made public.

With the changes suggested above, these should not be necessary.

> - text section's relocation pointer and count are also routed to
>   do_calls_non_split.

This is OK (along with the Output_file* parameter).

> - since calls_non_split now takes a templated-type parameter (the reloc
>   pointer), it's moved to Sized_target class, and a few ugly dynamic
>   casts are inserted.

This should not be necessary. We don't use dynamic casts anywhere in
gold, so let's keep it that way.

> diff --git a/configure b/configure
> index 3bb1c03..91fd098 100755
> --- a/configure
> +++ b/configure
> @@ -2972,7 +2972,7 @@ case "${ENABLE_GOLD}" in
>        # Check for target supported by gold.
>        case "${target}" in
>          i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
> -        | aarch64*-*-* | tilegx*-*-*)
> +        | aarch64*-*-* | tilegx*-*-* | s390*-*-*)
>    configdirs="$configdirs gold"
>    if test x${ENABLE_GOLD} = xdefault; then
>      default_ld=gold

Please remove this change from the patch. I'll get the top-level
configure changes in separately for s390 and mips.

> diff --git a/gold/Makefile.in b/gold/Makefile.in

It's not necessary to include regenerated files in the patch you send.

+      if (static_cast<section_size_type>(curoffset + 2) > view_size)

Use convert_to_section_size_type().

+  // A function for targets to call.  Return whether BYTES/LEN matches
+  // VIEW/VIEW_SIZE at OFFSET.
+  bool
+  match_view(const unsigned char* view, section_size_type view_size,
+     section_offset_type offset, const unsigned char* bytes, size_t len) const
+    {
+      return this->match_view(view, view_size, offset,
+      reinterpret_cast<const char *>(bytes), len);
+    }

Rather than create two copies of the same routine, one of which calls
the other, I suggest making this version of match_view() an inline
private to the Target_s390 class.

Should be <const char*> (no space before *), and watch the line lengths.

Don't forget a ChangeLog entry!

-cary