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

[Ian Lance Taylor <ian at airs dot com>]

Nick Clifton <nickc@redhat.com> writes:

> gold/ChangeLog
> 2010-10-04  Nick Clifton  <nickc@redhat.com>
>
> 	* 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  <nickc@redhat.com>
>
> 	* ld.texinfo: Update description of computation of VMA and LMA
>         addresses for output sections.
>        
> ld/testsuite/ChangeLog
> 2010-10-04  Nick Clifton  <nickc@redhat.com>
>
> 	* 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