C6X new PCR relocs

Joseph S. Myers joseph@codesourcery.com
Tue May 17 23:39:00 GMT 2011


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



More information about the Binutils mailing list