RFC: GOLD: Add support for MEMORY regions in linker scripts

RFC: GOLD: Add support for MEMORY regions in linker scripts

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

Hi Guys,

  Attached is a rough draft of a patch to add support for MEMORY regions
  in linker scripts to the GOLD linker.  Since I am new to GOLD and not
  much of a C++ programmer I thought that I would ask for any comments
  or corrections at this stage before I go too far.

  As it stands the patch appears to implement support for MEMORY
  regions, at least for the simple test case that I am using.  I am not
  at all sure that I have implemented it in the correct manner however,
  or if it will stand up to more robust testing.  Time will tell.  But
  if anyone has anything to say about it please let me know.

Cheers
  Nick


[-- Attachment #2: gold.MEMORY.patch.bz2 --]
[-- Type: application/x-bzip2, Size: 5705 bytes --]

Re: RFC: GOLD: Add support for MEMORY regions in linker scripts

[Ian Lance Taylor]

Nick Clifton <nickc@redhat.com> writes:

>   Attached is a rough draft of a patch to add support for MEMORY regions
>   in linker scripts to the GOLD linker.  Since I am new to GOLD and not
>   much of a C++ programmer I thought that I would ask for any comments
>   or corrections at this stage before I go too far.
>
>   As it stands the patch appears to implement support for MEMORY
>   regions, at least for the simple test case that I am using.  I am not
>   at all sure that I have implemented it in the correct manner however,
>   or if it will stand up to more robust testing.  Time will tell.  But
>   if anyone has anything to say about it please let me know.

Thanks for tackling this.


> + extern void
> + script_include_directive(void *, const char*, size_t);

s/void */void*/


> +   // Return true if <name,namelen> matches this region.
> +   bool
> +   name_match(const char* name, size_t namelen)
> +   {
> +     return this->name_.length() == namelen
> +       && strncmp (this->name_.c_str(), name, namelen) == 0;
> +   }

I usually use parentheses so that emacs indents correctly.

> + 
> +   uint64_t
> +   get_current_vma_address() const
> +   {
> +     return start_->eval(NULL, NULL, false) + current_vma_offset_;
> +   }
> +   
> +   uint64_t
> +   get_current_lma_address() const
> +   {
> +     return start_->eval(NULL, NULL, false) + current_lma_offset_;
> +   }

Passing NULL, NULL here certainly makes me nervous.  It would be better
to figure out a way to get the symbol table and layout in here.

Also, use explicit this-> when referring to members.

> +   void
> +   increment_vma_offset(uint64_t amount)
> +   { current_vma_offset_ += amount; }
> +   
> +   void
> +   increment_lma_offset(uint64_t amount)
> +   { current_lma_offset_ += amount; }

Use explicit this-> when referring to members.


> + // Print a memory region.
> + 
> + void
> + Memory_region::print(FILE* f) const
> + {
> +   fprintf(f, "  %s", this->name_.c_str());
> + 
> +   unsigned int attrs = this->attributes_;
> +   if (attrs != 0)
> +     {
> +       fprintf (f, " (");
> +       do
> + 	{
> + 	  switch (attrs & - attrs)
> + 	    {
> + 	    case MEM_EXECUTABLE:  fputc ('x', f); break;
> + 	    case MEM_WRITEABLE:   fputc ('w', f); break;
> + 	    case MEM_READABLE:    fputc ('r', f); break;
> + 	    case MEM_ALLOCATABLE: fputc ('a', f); break;
> + 	    case MEM_INITIALIZED: fputc ('i', f); break;
> + 	    default:
> + 	      gold_unreachable ();
> + 	    }
> + 	  attrs &= ~ (attrs & - attrs);
> + 	}
> +       while (attrs);

I tend to prefer an explicit != 0 when not dealing with a boolean.

> +       fputc (')', f);
> +     }
> + 
> +   fprintf(f, " : origin = ");
> +   this->start_->print (f);
> +   fprintf (f, ", length = ");
> +   this->length_->print (f);
> +   fprintf (f, "\n");
> + }

In C++ no space before left parenthesis.


> *************** class Sections_element
> *** 415,420 ****
> --- 527,537 ----
>     get_output_section() const
>     { return NULL; }
  
> +   // Set the section's memory regions.
> +   virtual void
> +   set_memory_region(Memory_region*, bool)
> +   { gold_unreachable(); }

Calling gold_unreachable here looks odd.  What prevents this method from
being called on a Sections_element which is not an
Output_section_definition?  Perhaps an error message would be more
appropriate?


> + void
> + Output_section_definition::set_memory_region(Memory_region* mr, bool set_vma)
> + {
> +   gold_assert (mr != NULL);

Is this assertion really needed?  Also, no space before left
parenthesis.


> +   Output_section* os = this->get_output_section();
> + 
> +   if (set_vma)
> +     {
> +       this->evaluated_address_ = mr->get_current_vma_address();
> +       this->address_ = script_exp_integer (this->evaluated_address_);
> +       if (os != NULL)
> + 	mr->increment_vma_offset (os->current_data_size());
> +     }
> +   else
> +     {
> +       this->evaluated_load_address_ = mr->get_current_lma_address ();
> +       this->load_address_ = script_exp_integer (this->evaluated_load_address_);
> +       if (os != NULL)
> + 	{
> + 	  os->set_load_address(this->evaluated_address_);
> + 	  mr->increment_lma_offset (os->current_data_size());
> + 	}
> +     }
> + }

This looks wrong to me.  This function is going to get called while
parsing the linker script.  At that point the size of the section is not
known, so the calls to increment_vma_offset and increment_lma_offset
aren't going to do the right thing.

I think that you need to build a list of sections attached to the memory
region, and then use them in Script_sections::set_section_addresses
which is called by Layout::relaxation_loop_body.

But I'm surprised this would work at all so maybe I'm missing something.


> + Memory_region*
> + Script_sections::find_memory_region(const char* name, size_t namelen)
> + {
> +   if (this->memory_regions_ == NULL)
> +     return NULL;
> + 
> +   for (Memory_regions::const_iterator m = this->memory_regions_->begin();
> +        m != this->memory_regions_->end();
> +        ++m)
> +     if ((*m)->name_match(name,namelen))
> +       return *m;
> + 
> +   return NULL;
> + }

Space after comma.


> + // Set the memory region to use for the current section.
> + 
> + void
> + Script_sections::set_memory_region(Memory_region* mr, bool set_vma)
> + {
> +   gold_assert(!this->sections_elements_->empty());
> +   Sections_elements::iterator this_section = this->sections_elements_->end();
> +   this_section --;
> +   (*this_section)->set_memory_region(mr, set_vma);
> + }

You can just write
    this->sections_elements_->back().set_memory_region(mr, set_vma);


> + // Functions for memory regions.
> + 
> + extern "C" Expression*
> + script_exp_function_origin(void* closurev, const char* name, size_t namelen)
> + {
> +   Parser_closure*   closure = static_cast<Parser_closure*>(closurev);
> +   Script_sections*  ss = closure->script_options()->script_sections();
> +   Expression*       origin = ss->find_memory_region_origin(name, namelen);

Please don't align the names.

> + 
> +   if (origin == NULL)    
> +     gold_error(_("undefined memory region '%s' referenced in ORIGIN expression"),
> + 	       name);
> + 
> +   return origin;
> + }

In the case where origin == NULL, I think you need to build a dummy
expression and return it, otherwise the linker will crash later.


Overall it looks good except for the issue about building a list of
sections while reading a script and then processing the list in
set_section_addresses.

Ian

Re: RFC: GOLD: Add support for MEMORY regions in linker scripts

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Hi Ian,

   Thanks for the review of my original patch.  Attached is a revised 
one which I hope addresses all of the issues that you raised.  I have 
also added some more error checking, and an entry in the testsuite.

   What do you think - is the patch suitable for committing to the sources ?

Cheers
   Nick

gold/ChangeLog
2010-09-02  Nick Clifton  <nickc@redhat.com>

	* README: Remove claim that MEMORY is not supported.
	* expression.cc (script_exp_function_origin)
	(script_exp_function_length): Move from here to ...
	* script.cc: ... here.
	(script_set_section_region, script_add_memory)
	(script_parse_memory_attr, script_include_directive): New
	functions.
	* script-sections.cc
	(class Memory_region): New class.
	(class Output_section_definition): Add set_memory_region,
	set_section_vma, set_section_lma and get_section_name methods.
	(class Script_Sections): Add add_memory_region,
	find_memory_region, find_memory_region_origin,
	find_memory_region_length and set_memory_region methods.
	Have set_section_addresses method walk the list of set memory
	regions.
	Extend the print methos to display memory regions.
	* script-sections.h: Add prototypes for new methods.
	Add enum for MEMORY region attributes.
	* yyscript.y: Add support for parsing MEMORY regions.
	* script-c.h: Add prototypes for new functions.
	* testsuite/Makefile.am: Add test of MEMORY region functionality.
	* testsuite/Makefile.in: Regenerate.
	* testsuite/memory_test.sh: New script.
	* testsuite/memory_test.s: New assembler source file.
	* testsuite/memory_test.t: New linker script.

[-- Attachment #2: gold.MEMORY.patch.bz2 --]
[-- Type: application/x-bzip, Size: 7503 bytes --]

Re: RFC: GOLD: Add support for MEMORY regions in linker scripts

[Ian Lance Taylor]

Nick Clifton <nickc@redhat.com> writes:

> gold/ChangeLog
> 2010-09-02  Nick Clifton  <nickc@redhat.com>
>
> 	* README: Remove claim that MEMORY is not supported.
> 	* expression.cc (script_exp_function_origin)
> 	(script_exp_function_length): Move from here to ...
> 	* script.cc: ... here.
> 	(script_set_section_region, script_add_memory)
> 	(script_parse_memory_attr, script_include_directive): New
> 	functions.
> 	* script-sections.cc
> 	(class Memory_region): New class.
> 	(class Output_section_definition): Add set_memory_region,
> 	set_section_vma, set_section_lma and get_section_name methods.
> 	(class Script_Sections): Add add_memory_region,
> 	find_memory_region, find_memory_region_origin,
> 	find_memory_region_length and set_memory_region methods.
> 	Have set_section_addresses method walk the list of set memory
> 	regions.
> 	Extend the print methos to display memory regions.
> 	* script-sections.h: Add prototypes for new methods.
> 	Add enum for MEMORY region attributes.
> 	* yyscript.y: Add support for parsing MEMORY regions.
> 	* script-c.h: Add prototypes for new functions.
> 	* testsuite/Makefile.am: Add test of MEMORY region functionality.
> 	* testsuite/Makefile.in: Regenerate.
> 	* testsuite/memory_test.sh: New script.
> 	* testsuite/memory_test.s: New assembler source file.
> 	* testsuite/memory_test.t: New linker script.


> + extern void
> + script_add_memory(void*, const char*, size_t, unsigned int, Expression_ptr, Expression_ptr);

Line too long.

> +   { gold_error (_("Attempt to set a memory region for a non-output section")); }

No space before left parenthesis.

> +       gold_error (_("region '%.*s' already defined"), namelen, name);

Likewise.

> +     switch (*attrs++)
> +       {
> +       case 'R':
> +       case 'r':
> +       attributes |= MEM_READABLE; break;

It's hard to tell in the diff format, but I'm not sure the indentation
is correct here.  The statements after the case labels should be
indented by two spaces.

This is OK with those changes.

Thanks.

Ian

Re: RFC: GOLD: Add support for MEMORY regions in linker scripts

[Nick Clifton]

Hi Ian,

>> > + extern void
>> > + script_add_memory(void*, const char*, size_t, unsigned int, Expression_ptr, Expression_ptr);
> Line too long.

Fixed.  I found a couple of other places where I had lines of over 80 
characters, so I fixed those as well.  There were two lines that I did 
not fix however:  script-sections.cc lines 3207 and 3219.  I am not sure 
what to do here.  Should the lines be split at a point of indirection, 
or should they be left as-is ?  For example this is line 3207:
					
       (*s)->get_output_section()->current_data_size(),
                                          ^
                    80-char-limit-is-here-^


>> > +       case 'R':
>> > +       case 'r':
>> > +       attributes |= MEM_READABLE; break;
> It's hard to tell in the diff format, but I'm not sure the indentation
> is correct here.  The statements after the case labels should be
> indented by two spaces.

They were like that - it was just the way that the tabs were behaving in 
the patch.

Committed.

Cheers
   Nick

Re: RFC: GOLD: Add support for MEMORY regions in linker scripts

[Ian Lance Taylor]

Nick Clifton <nickc@redhat.com> writes:

> There were two lines that I did
> not fix however:  script-sections.cc lines 3207 and 3219.  I am not
> sure what to do here.  Should the lines be split at a point of
> indirection, or should they be left as-is ?  For example this is line
> 3207:
> 					
>       (*s)->get_output_section()->current_data_size(),
>                                          ^
>                    80-char-limit-is-here-^

I normally introduce a temporary variable.

Ian

Re: RFC: GOLD: Add support for MEMORY regions in linker scripts

[H.J. Lu]

On Wed, Sep 8, 2010 at 9:15 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi Ian,
>
>>> > + extern void
>>> > + script_add_memory(void*, const char*, size_t, unsigned int,
>>> > Expression_ptr, Expression_ptr);
>>
>> Line too long.
>
> Fixed.  I found a couple of other places where I had lines of over 80
> characters, so I fixed those as well.  There were two lines that I did not
> fix however:  script-sections.cc lines 3207 and 3219.  I am not sure what to
> do here.  Should the lines be split at a point of indirection, or should
> they be left as-is ?  For example this is line 3207:
>
>      (*s)->get_output_section()->current_data_size(),
>                                         ^
>                   80-char-limit-is-here-^
>
>
>>> > +       case 'R':
>>> > +       case 'r':
>>> > +       attributes |= MEM_READABLE; break;
>>
>> It's hard to tell in the diff format, but I'm not sure the indentation
>> is correct here.  The statements after the case labels should be
>> indented by two spaces.
>
> They were like that - it was just the way that the tabs were behaving in the
> patch.
>
> Committed.

It failed to build on Fedora 13/x86-64:

cc1plus: warnings being treated as errors
/export/gnu/import/git/binutils/gold/script-sections.cc: In member
function ‘void gold::Script_sections::add_memory_region(const char*,
size_t, unsigned int, gold::Expression*, gold::Expression*)’:
/export/gnu/import/git/binutils/gold/script-sections.cc:2791: error:
field precision should have type ‘int’, but argument 2 has type
‘size_t’
make[6]: *** [script-sections.o] Error 1


-- 
H.J.

Re: RFC: GOLD: Add support for MEMORY regions in linker scripts

[H.J. Lu]

On Wed, Sep 8, 2010 at 9:15 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi Ian,
>
>>> > + extern void
>>> > + script_add_memory(void*, const char*, size_t, unsigned int,
>>> > Expression_ptr, Expression_ptr);
>>
>> Line too long.
>
> Fixed.  I found a couple of other places where I had lines of over 80
> characters, so I fixed those as well.  There were two lines that I did not
> fix however:  script-sections.cc lines 3207 and 3219.  I am not sure what to
> do here.  Should the lines be split at a point of indirection, or should
> they be left as-is ?  For example this is line 3207:
>
>      (*s)->get_output_section()->current_data_size(),
>                                         ^
>                   80-char-limit-is-here-^
>
>
>>> > +       case 'R':
>>> > +       case 'r':
>>> > +       attributes |= MEM_READABLE; break;
>>
>> It's hard to tell in the diff format, but I'm not sure the indentation
>> is correct here.  The statements after the case labels should be
>> indented by two spaces.
>
> They were like that - it was just the way that the tabs were behaving in the
> patch.
>
> Committed.
>

Gold generates different outputs from ld:

http://www.sourceware.org/bugzilla/show_bug.cgi?id=11997

-- 
H.J.