RE: [Patch, ARM]Enable veneer routine for R_ARM_THM_JUMP19

["Tony Wang" <tony dot wang at arm dot com>]

> -----Original Message-----
> From: Will Newton [mailto:will.newton@linaro.org]
> Sent: Monday, August 04, 2014 5:14 PM
> To: Tony Wang
> Cc: binutils@sourceware.org; nickc@redhat.com
> Subject: Re: [Patch, ARM]Enable veneer routine for R_ARM_THM_JUMP19
> 
> On 4 August 2014 05:40, Tony Wang <tony.wang@arm.com> wrote:
> > Hi there,
> >
> > According to AAPCS-ELF specification R_ARM_THM_JUMP19 should also be
> > veneered if the target is outside the addressable span of the branch
> > instruction or where interworking happened.
> >
> > So the attached patch implements the veneer routine for
> > R_ARM_THM_JUMP19, and two new macros
> THM2_MAX_FWD_COND_BRANCH_OFFSET
> > and THM2_MAX_BWD_COND_BRANCH_OFFSET are introduced. Also
> updated the
> > conditional branch for both cases mentioned above.
> >
> > Tested with Binutils regression test, no new regressions. No
> > regression on gcc trunk with target cortex m4
> >
> > Ok for the trunk?
> >
> > bfd/ChangeLog:
> > 2014-08-4  Tony Wang <tony.wang@arm.com>
> >
> >      * elf32-arm.c (elf32_arm_final_link_relocate): Implement the
> > veneer routine for R_ARM_THM_JUMP19.
> >      (arm_type_of_stub): Add conditional clause for R_ARM_THM_JUMP19
> >      (elf32_arm_size_stub): Ditto.
> >
> > ld/testsuite/ChangeLog:
> > 2014-08-4  Tony Wang <tony.wang@arm.com>
> >
> >      * ld-arm/jump-reloc-veneers-cond.s: New test.
> >      * ld-arm/farcall-cond-thumb-arm.s: Ditto.
> >      * ld-arm/jump-reloc-veneers-cond-short.d: Expected output for
> > target without a veneer generation.
> >      * ld-arm/jump-reloc-veneers-cond-long.d: Expected output for
> > target with a veneer generation.
> >      * ld-arm/farcall-cond-thumb-arm.d: Expected output for inter
> > working veneer generation.
> >      * ld-arm/arm-elf.exp: Add tests for conditional branch veneer.
> 
> Looks ok to me (although I am not a maintainer). Small details but logical
> operations bind more loosely than almost every other operator so in some
> cases you can drop the brackets around assignments in conditionals and
> make them a bit easier to read. 

I agreed with your point.

>Also the casts to unsigned int in
> elf32_arm_size_stubs seem unnecessary.

>From my point of view, it might still needed here, as the enum type not always equals to unsigned int.  

> 
> --
> Will Newton
> Toolchain Working Group, Linaro