This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] gold/arm: define $a/$d markers in .plt


Roland McGrath <mcgrathr@google.com> writes:

> The Symbol_table changes are all based on Cary's advice (on the binutils
> list).  I really can't explain or vouch for this being the best way to
> enable the missing functionality: defining synthetic local symbols
> without regard to preexisting symbols of the same name.  Cary thinks
> that do_define_in_output_segment and do_define_as_constant should get
> the same treatment (though there are so far no uses of those that need
> it); he also agrees with me that the boilerplate chunk (21 lines that
> becomes 32 lines) that calls define_special_symbol (or doesn't) should
> be factored out rather than repeated three times, for maintainability;
> we both agree that those improvements can come after this change goes in
> (and I think Cary has implicitly volunteered to do that cleanup).

The three functions (do_define_in_output_data,
do_define_in_output_segment, and do_define_as_constant) all need to act
the same way.  You are keying off of binding == STB_LOCAL to omit adding
the name to the global symbol table, but as you've discovered that mixes
together two unrelated cases.  Your case is a symbol that is always
local and is never referenced by name.  The other case, the one
currently supported, is a symbol that is global but is forced local by a
version script.  A symbol like _GLOBAL_OFFSET_TABLE_ is the latter.

So I think your code needs to add a new value to the Defined enum,
PREDEFINED_UNREFERENCED or something, to add a new local symbol with no
globally visible name.

Then you can handle that in define_special_symbol if you like, or via
some other helper function that calls define_special_symbol.


> -      layout->add_output_section_data(".plt", elfcpp::SHT_PROGBITS,
> +
> +      Output_section* os =
> +          layout->add_output_section_data (".plt", elfcpp::SHT_PROGBITS,
>  				      (elfcpp::SHF_ALLOC
>  				       | elfcpp::SHF_EXECINSTR),
>  				      this->plt_, ORDER_PLT, false);

No space before '('.


> diff --git a/gold/symtab.cc b/gold/symtab.cc
> index 1edb88d..804ea09 100644
> --- a/gold/symtab.cc
> +++ b/gold/symtab.cc
> @@ -1,6 +1,6 @@
>  // symtab.cc -- the gold symbol table
>  
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2006,2007,2008,2009,2010,2011,2012 Free Software Foundation, Inc.

In general GCC and BFD keep the spaces between years and instead break
the line before "Free Software Foundation," and gold should do the
same.  Same in symtab.h.



> +  // Next do all the symbols which have been generated as local.

Seems to me that this loop should be before the loop over the entire
symbol table, not after.


> -  // The type of the list of symbols which have been forced local.
> -  typedef std::vector<Symbol*> Forced_locals;
> +  // The type of a simple vector of symbols.
> +  typedef std::vector<Symbol*> Symbol_vector;

I don't like using words like "vector" as part of a type, because they
imply a specific container implementation when the point should be what
the container holds.  Just call this type Symbols, which happens to
match the type Object::Symbols.

Ian


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]