This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] gold: Process ARM relocation types used in Android.
- From: Ian Lance Taylor <iant at google dot com>
- To: Doug Kwan (éæå) <dougkwan at google dot com>
- Cc: binutils at sourceware dot org
- Date: Wed, 03 Jun 2009 00:24:52 -0700
- Subject: Re: [PATCH] gold: Process ARM relocation types used in Android.
- References: <498552560906021835y626c54f1l621404681245d68b@mail.gmail.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