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