Re: [PATCH] gold: Process ARM relocation types used in Android.

[Ian Lance Taylor <iant at google dot com>]

"Doug Kwan (éæå)" <dougkwan@google.com> writes:

> 2009-06-02  Doug Kwan  <dougkwan@google.com>
>
>         * gold/arm.cc (struct utils): New.
>         (Target_arm::reloc_is_non_pic): Define new method.
>         (class Arm_relocate_functions): New.
>         (Target_arm::Relocate::relocate): Handle relocation types used by
>         Android.


> +// Utilities for manipulating integers of up to 32-bits
> +
> +struct utils
> +{

This doesn't seem to be a struct--there aren't any fields.  So,
s/struct/namespace/.


> +  // sign extend an n-bit unsigned integer stored in an uint32_t into
> +  // an int32_t. NO_BITS must be between 1 to 32.

Capitalize "sign".  Two spaces after the period.

> +  template<int no_bits>
> +  static inline int32_t
> +  sign_extend(uint32_t bits)
> +  {
> +    if (no_bits < 1 || no_bits > 32)
> +      gold_unreachable();

Change this to
  gold_assert(no_bits >= 1 && no_bits <= 32);

> +  // Detects overflow of an NO_BITS integer stored in a uint32_t.
> +  template<int no_bits>
> +  static inline bool
> +  has_overflow(uint32_t bits)
> +  {
> +    if (no_bits < 1 || no_bits > 32)
> +      gold_unreachable();

Use gold_assert here.

> +  // Select bits from A and B using bits in MASk.  For each n in [0..31],
> +  // the n-th bit in the result is chosen from the n-th bits of A and B.
> +  // A zero selects A and a one selects B.

s/MASk/MASK/.


> +  // If this the symbol is a Thumb function, set thumb bit to 1.
> +  bool has_thumb_bit = (gsym != NULL) && (gsym->type() == elfcpp::STT_FUNC);

Do you need to check STT_ARM_TFUNC here?

> +
> +  // Pick the value to use for symbols defined in shared objects.
> +  Symbol_value<32> symval;
> +  if (gsym != NULL
> +      && gsym->use_plt_offset(reloc_is_non_pic(r_type)))
> +    {
> +      symval.set_output_value(target->plt_section()->address()
> +			      + gsym->plt_offset());
> +      psymval = &symval;
> +      has_thumb_bit = 0;
> +    }

s/has_thumb_bit = 0/has_thumb_bit = false/

> +	  printf("r_type %d r_sym %d\n", r_type, r_sym);

Remove this.

>    switch (r_type)
>      {
>      case elfcpp::R_ARM_NONE:
>        break;
>  
> +    case elfcpp::R_ARM_ABS32:
> +      if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, true,
> +				    output_section))
> +	reloc_status = Relocate_functions::abs32(view, object, psymval,
> +						 has_thumb_bit);
> +      break;

s/Relocate_functions/Arm_relocate_functions/ here and several following
cases.  I'm not sure why it works to use Relocate_functions.

> +    case elfcpp::R_ARM_BASE_PREL:
> +      {
> +	uint32_t origin;
> +	// Get the addressing origin of the output segment definiting the 
> +	// symbol gsym (AAELF 4.6.1.2 Relocation types)

s/definiting/defining/

> +	gold_assert(gsym != NULL); 
> +	if (gsym->source() == Symbol::IN_OUTPUT_SEGMENT)
> +	  origin = gsym->output_segment()->vaddr();
> +	else if (gsym->source () == Symbol::IN_OUTPUT_DATA)
> +	  origin = gsym->output_data()->address();
> +	else
> +	  gold_unreachable ();

I don't think gold_unreachable is appropriate here.  It seems like this
could happen with an erroneous input file.  It would probably be better
to print an error message and return.

> +  // Report any errors.
> +  switch (reloc_status)
> +    {
> +    case Relocate_functions::STATUS_OKAY:
> +      break;
> +    case Relocate_functions::STATUS_OVERFLOW:
> +      gold_error_at_location(relinfo, relnum, rel.get_r_offset(),
> +			     _("Overflow in reloc %u"), r_type);
> +      break;
> +    case Relocate_functions::STATUS_BAD_RELOC:
> +      gold_error_at_location(relinfo, relnum, rel.get_r_offset(),
> +			     _("Bad reloc %u"), r_type);
> +      break;

Let's make these error message a little better.  How about something
like

relocation overflow; address out of range

unexpected opcode while processing relocation

Note that GNU warnings and error messages never start with a capital
letter.


This patch is OK with those changes.

Thanks.

Ian