This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: C6X new PCR relocs
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Bernd Schmidt <bernds at codesourcery dot com>
- Cc: binutils at sourceware dot org
- Date: Tue, 17 May 2011 23:38:53 +0000 (UTC)
- Subject: Re: C6X new PCR relocs
- References: <4DD29C46.5090808@codesourcery.com>
On Tue, 17 May 2011, Bernd Schmidt wrote:
> * elf32-tic6x.c (elf32_tic6x_howto_table): Add entries for
> R_C6000_PCR_H16 and R_C6000_PCR_L16.
I think elf32_tic6x_howto_table_rel should have the corresponding
EMPTY_HOWTO entries changed to use the relocation names instead of the
numbers (similar to how other RELA-only relocations are handled).
> *************** elf32_tic6x_relocate_section (bfd *outpu
> *** 2378,2383 ****
> --- 2402,2421 ----
> unresolved_reloc = FALSE;
> break;
>
> + case R_C6000_PCR_H16:
> + case R_C6000_PCR_L16:
> + off = (input_section->output_section->vma
> + + input_section->output_offset
> + + rel->r_offset);
> + /* These must be calculated as R = S - FP(FP(PC) - A).
> + PC, here, is the value we just computed in OFF. RELOCATION
> + has the address of S + A. */
> + relocation -= rel->r_addend;
> + off2 = ((off & ~0x1f) - rel->r_addend) & ~0x1f;
I think it would be safer to use ~(bfd_vma) 0x1f in both places on this
line instead of ~0x1f. Truncation to 32 bits (in the 64-bit-BFD case) may
well happen elsewhere, but having everything of type bfd_vma in such
expressions is more obviously correct.
> + case BFD_RELOC_C6000_ABS_S16:
> + case BFD_RELOC_C6000_ABS_L16:
> + new_reloc = BFD_RELOC_C6000_PCR_L16;
> + break;
> +
> + case BFD_RELOC_C6000_ABS_H16:
> + new_reloc = BFD_RELOC_C6000_PCR_H16;
> + break;
> +
> + default:
> + as_bad (_("$PCR_OFFSET not supported in this context"));
> + return;
Should have a testcase for the error here (for each context in which
$PCR_OFFSET isn't allowed, like reloc-bad-2.s). Also, since the
relocations are L16 and H16 (with, correspondingly, no overflow checking
possible) are you sure you want to allow the BFD_RELOC_C6000_ABS_S16 case?
> *************** md_apply_fix (fixS *fixP, valueT *valP,
> *** 3823,3828 ****
> --- 3881,3904 ----
> abort ();
> break;
>
> + case BFD_RELOC_C6000_PCR_H16:
> + case BFD_RELOC_C6000_PCR_L16:
> + if (fixP->fx_done || !seg->use_rela_p)
> + {
> + offsetT newval = md_chars_to_number (buf, 4);
> + int shift = fixP->fx_r_type == BFD_RELOC_C6000_PCR_H16 ? 16 : 0;
> +
> + MODIFY_VALUE (newval, value, shift, 7, 16);
> + if ((value < -0x8000 || value > 0x7fff)
> + && (fixP->fx_r_type == BFD_RELOC_C6000_ABS_S16
> + || fixP->fx_r_type == BFD_RELOC_C6000_SBR_S16))
> + as_bad_where (fixP->fx_file, fixP->fx_line,
> + _("immediate offset out of range"));
This overflow checking is clearly wrong. It's unreachable because the
relocation can never be one of the two in the condition, and the nature of
the L16/H16 pair of relocations is that any value can be represented by
the pair, so no overflow checking.
> *************** typedef struct
> *** 142,147 ****
> --- 142,150 ----
> instruction, whereas a non-constant represents a DP-relative
> value counting in the appropriate units). */
> bfd_boolean fix_adda;
> + /* The symbol to be subtracted in case of a PCR_H16 or PCR_L16
> + reloc. */
> + symbolS *fix_subsy;
> } tic6x_fix_info;
I think the new field should be initialized in tic6x_init_fix_data.
--
Joseph S. Myers
joseph@codesourcery.com