Provisional patch for issue 11997 (Different behaviour of MEMORY regions)

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Provisional patch for issue 11997 (Different behaviour of MEMORY regions)

Nick Clifton
Hi Ian,

  I have attached a patch that I think addresses the problems raised by
  issue 11997 (the different behaviour of MEMORY regions in GOLD and GNU
  LD).  Would you mind reviewing it before I submit it to the issue
  please ?

  The patch is quite extensive, in the changes that it makes to the
  MEMORY handling code, as I found several other problems whilst working
  on getting compatibility between GOLD and GNU LD.

  I have also updated the linker documentation that describes how VMA
  and LMA addresses are computed, and extended the memory_test/rgn-at5
  test to take into account some problems that I found.

  There are still two discrepancies between GOLD's output and GNU LD's
  output:

    1)  GOLD creates a segment to hold the program headers, LD does
        not.

    2)  The ordering of the sections in the section header is
        different.  All the sections are present and correct, but LD
        orders them by name, whereas GOLD orders them by file offset.

  I am assuming that neither of these differences is important.  At
  least not for now.

  So, what do you think.  Is the patch OK for applying to GOLD ?

Cheers
  Nick

gold/ChangeLog
2010-10-04  Nick Clifton  <[hidden email]>

        * script-sections.cc(class Memory_region): Remove
        current_lma_offset_ field.  Rename current_vma_offset_ to
        current_offset_.  Add last_section_ field.
        (Memory_region::get_current_vma_address): Rename to
        get_current_address.
        (Memory_region::get_current_lma_address): Delete.
        (Memory_region::increment_vma_offset): Rename to
        increment_offset.
        (Memory_region::increment_lma_offset): Delete.
        (Memory_region::attributes_compatible): New method.  Returns
        true if the provided section is compatible with the region.
        (Memory_region::get_last_section): New method.  Returns the last
        section to use the region.
        (Memory_region::set_last_section): New method.  Stores the last
        section to use the region.
        (Script_sections::block_in_region): New method.  Returns true if
        a block of memory is contained within a region.
        (Script_sections::find_memory_region): New method.  Locates a
        memory region to be used to set a VMA or LMA address.
        (Output_section_definition::set_section_addresses): Add code to
        check for addresses set by memory regions.
        (Output_segment::set_section_addresses): Remove memory region
        walking code.
        (Script_sections::create_segment): Add a warning if a header
        segment is created outside of any region.
        * script-sections.h (class Script_sections): Add prototypes for
        find_memory_region and block_in_region methods.
       
        * testsuite/memory_test.s: Use .long instead of .word.
        * testsuite/memory_test.t: Add some more output sections.
        * testsuite/memory_test.sh: Update expected output.

ld/ChangeLog
2010-10-04  Nick Clifton  <[hidden email]>

        * ld.texinfo: Update description of computation of VMA and LMA
        addresses for output sections.
       
ld/testsuite/ChangeLog
2010-10-04  Nick Clifton  <[hidden email]>

        * ld-scripts/rgn-at5.t: Add some more output sections.
        * ld-scripts/rgn-at5.d: Update expected output.


gold.i11997.patch.bz2 (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Provisional patch for issue 11997 (Different behaviour of MEMORY regions)

Ian Lance Taylor
Nick Clifton <[hidden email]> writes:

> gold/ChangeLog
> 2010-10-04  Nick Clifton  <[hidden email]>
>
> * script-sections.cc(class Memory_region): Remove
> current_lma_offset_ field.  Rename current_vma_offset_ to
>         current_offset_.  Add last_section_ field.
>         (Memory_region::get_current_vma_address): Rename to
> get_current_address.
>         (Memory_region::get_current_lma_address): Delete.
>         (Memory_region::increment_vma_offset): Rename to
> increment_offset.
>         (Memory_region::increment_lma_offset): Delete.
>         (Memory_region::attributes_compatible): New method.  Returns
> true if the provided section is compatible with the region.
>         (Memory_region::get_last_section): New method.  Returns the last
> section to use the region.
>         (Memory_region::set_last_section): New method.  Stores the last
>         section to use the region.
>         (Script_sections::block_in_region): New method.  Returns true if
> a block of memory is contained within a region.
>         (Script_sections::find_memory_region): New method.  Locates a
> memory region to be used to set a VMA or LMA address.
>         (Output_section_definition::set_section_addresses): Add code to
> check for addresses set by memory regions.
>         (Output_segment::set_section_addresses): Remove memory region
> walking code.
>         (Script_sections::create_segment): Add a warning if a header
> segment is created outside of any region.
> * script-sections.h (class Script_sections): Add prototypes for
> find_memory_region and block_in_region methods.
>        
>         * testsuite/memory_test.s: Use .long instead of .word.
>         * testsuite/memory_test.t: Add some more output sections.
>         * testsuite/memory_test.sh: Update expected output.
>
> ld/ChangeLog
> 2010-10-04  Nick Clifton  <[hidden email]>
>
> * ld.texinfo: Update description of computation of VMA and LMA
>         addresses for output sections.
>        
> ld/testsuite/ChangeLog
> 2010-10-04  Nick Clifton  <[hidden email]>
>
> * ld-scripts/rgn-at5.t: Add some more output sections.
>         * ld-scripts/rgn-at5.d: Update expected output.


> +  get_current_address(void) const

I didn't notice this earlier, but there is no need to say (void) in
C++.  You can just say (), and you might as well do that.

> +    return (this->current_offset_ + amount <
> +           this->length_->eval(symtab, layout, false));

Move '<' to the start of the second line.

> +// Returns true if the provide block of memory is contained
> +// within a memory region.

Here "provide" should probably be "provided", but even better might be
something like "Return whether the memory from START to LENGTH is
contained with a memory region."

> +Memory_region*
> +Script_sections::find_memory_region(Output_section_definition* section,
> +                                   bool find_vma_region,
> +                                   Output_section_definition**
> +                                   previous_section_return)

Rather than breaking the parameter name from the type, indent all the
parameters four spaces from the left margin.  See, e.g.,
Output_section_element_input::set_section_addresses.  Also, the comment
should say what *PREVIOUS_SECTION_RETURN is set to.

>   std::string
>   get_section_name(void) const
>   { return this->name_; }

This was actually in the last patch: besides using "()" instead of "(void)"q,
this should return "const std::string&" rather than "std::string".  That
will avoid the use of the copy constructor.

> +  // The /DISCARD/ region never gets assigned to any region.
> +  if (section->get_section_name().compare("/DISCARD/") == 0)
> +    return NULL;

Just write
  if (section->get_section_name() == "/DISCARD/")

The comment should probably say "The /DISCARD/ section" rather than
"region".

> +      if (first_match == NULL
> +         && (*mr)->attributes_compatible
> +         (section->get_output_section()->flags(),
> +          section->get_output_section()->type()))

The indentation looks wrong here.  Use temporary variables if
necessary.

> +    * previous_section_return = first_match->get_last_section();

s/* /*/

> +  //    region whoes attributes are compatible with this section and

s/whoes/whose/

> +             if (this->address_ != NULL || previous_section == this)
> +               // Either an explicit VMA address has been set, or an
> +               // explicit VMA region has been set, so set the LMA equal to
> +               // the VMA.
> +               laddr = address;

When there is a comment in a then block, I think it reads a little
better if the block uses curly braces even for a single statement.  The
same for the else block here, and for the else block a few lines below.

> +      uint64_t size = *dot_value -
> +       vma_region->get_current_address()->eval(symtab, layout, false);

Move the '-' to the start of the second line, and use parentheses.  Use
a temporary variable if necessary.

> +  // IF the LMA region is different from the VMA region, then increment the

s/IF/If/

> +  // using non-existant or protected memory.  We test LMA rather

s/existant/existent/

> +      && ! this->block_in_region (NULL, layout, lma - subtract, subtract))

s/! /!/

> +    gold_warning(_("Creating a segment to contain the file and program"
> +                  " headers outside of any MEMORY region."));

s/Creating/creating/
s/region./region/


This is OK with those changes.

Thanks.

Ian