arm: Handle undefweak with ST_BRANCH_UNKNOWN

  A previous patch made ld fail early on Thumb-only where branch_type is
ST_BRANCH_UNKNOWN.

However, this fails erroneously when the target is undefweak: in that
case the branch should be replaced by a branch to the next instruction
(or nop.w on thumb2).  This patch accepts this case and restores the
previous behaviour in such cases.

This was reported by failures in the GCC testsuite, where we fail to
link executables because __deregister_frame_info is undefweak:

(__deregister_frame_info): Unknown destination type (ARM/Thumb) in ...crtbegin.o
crtbegin.o: in function `__do_global_dtors_aux':
crtstuff.c:(.text+0x52): dangerous relocation: unsupported relocation
---
 bfd/elf32-arm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
  

On 06/09/2024 18:21, Christophe Lyon wrote:
> A previous patch made ld fail early on Thumb-only where branch_type is
> ST_BRANCH_UNKNOWN.
> 
> However, this fails erroneously when the target is undefweak: in that
> case the branch should be replaced by a branch to the next instruction
> (or nop.w on thumb2).  This patch accepts this case and restores the
> previous behaviour in such cases.
> 
> This was reported by failures in the GCC testsuite, where we fail to
> link executables because __deregister_frame_info is undefweak:
> 
> (__deregister_frame_info): Unknown destination type (ARM/Thumb) in ...crtbegin.o
> crtbegin.o: in function `__do_global_dtors_aux':
> crtstuff.c:(.text+0x52): dangerous relocation: unsupported relocation
> ---
>  bfd/elf32-arm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index 7441ee2cc38..17df8b30eb6 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -10512,7 +10512,12 @@ elf32_arm_final_link_relocate (reloc_howto_type *	    howto,
>    if (using_thumb_only (globals)
>        && (r_type == R_ARM_THM_CALL
>  	  || r_type == R_ARM_THM_JUMP24)
> -      && branch_type == ST_BRANCH_UNKNOWN)
> +      && branch_type == ST_BRANCH_UNKNOWN
> +      /* Exception to the rule above: a branch to an undefined weak
> +	 symbol is turned into a jump to the next instruction unless a
> +	 PLT entry will be created (see below).  */
> +      && !(h && h->root.type == bfd_link_hash_undefweak
> +	   && plt_offset == (bfd_vma) -1))
>      {
>        if (sym_sec != NULL
>  	  && sym_sec->owner != NULL)

Did you look at the other case in elf32_arm_get_plt_info where we handle (I think) i-realtive relocs?  It might be that these must never be weak, but this is not really my area of expertise.  It might be worth a comment here if they don't need to be covered.  CCing Richard Sandiford as he did the original i-relative support for Arm (though some time ago now).

A testcase would also be useful, so that we don't need to run GCC to find validate this.

R.
  

On Tue, 10 Sept 2024 at 14:49, Richard Earnshaw (lists)
<Richard.Earnshaw@arm.com> wrote:
>
> On 06/09/2024 18:21, Christophe Lyon wrote:
> > A previous patch made ld fail early on Thumb-only where branch_type is
> > ST_BRANCH_UNKNOWN.
> >
> > However, this fails erroneously when the target is undefweak: in that
> > case the branch should be replaced by a branch to the next instruction
> > (or nop.w on thumb2).  This patch accepts this case and restores the
> > previous behaviour in such cases.
> >
> > This was reported by failures in the GCC testsuite, where we fail to
> > link executables because __deregister_frame_info is undefweak:
> >
> > (__deregister_frame_info): Unknown destination type (ARM/Thumb) in ...crtbegin.o
> > crtbegin.o: in function `__do_global_dtors_aux':
> > crtstuff.c:(.text+0x52): dangerous relocation: unsupported relocation
> > ---
> >  bfd/elf32-arm.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> > index 7441ee2cc38..17df8b30eb6 100644
> > --- a/bfd/elf32-arm.c
> > +++ b/bfd/elf32-arm.c
> > @@ -10512,7 +10512,12 @@ elf32_arm_final_link_relocate (reloc_howto_type *        howto,
> >    if (using_thumb_only (globals)
> >        && (r_type == R_ARM_THM_CALL
> >         || r_type == R_ARM_THM_JUMP24)
> > -      && branch_type == ST_BRANCH_UNKNOWN)
> > +      && branch_type == ST_BRANCH_UNKNOWN
> > +      /* Exception to the rule above: a branch to an undefined weak
> > +      symbol is turned into a jump to the next instruction unless a
> > +      PLT entry will be created (see below).  */
> > +      && !(h && h->root.type == bfd_link_hash_undefweak
> > +        && plt_offset == (bfd_vma) -1))
> >      {
> >        if (sym_sec != NULL
> >         && sym_sec->owner != NULL)
>
> Did you look at the other case in elf32_arm_get_plt_info where we handle (I think) i-realtive relocs?
> It might be that these must never be weak, but this is not really my area of expertise.
>  It might be worth a comment here if they don't need to be covered.
>  CCing Richard Sandiford as he did the original i-relative support for Arm (though some time ago now).
>
I did not look specifically at that, but the code which converts the
branch into nop says:
"A branch to an undefined weak symbol is turned into a jump to
   the next instruction unless a PLT entry will be created."
If elf32_arm_get_plt_info returns true, plt_offset != -1, so we
wouldn't convert the branch into a nop anyway?

So, in presence of i-relative & undefweak, we would now  enter "Handle
calls via the PLT"
because we now have stub_type == arm_stub_none, and thus try to jump
directly to the PLT in thumb mode,
and the normal code flow will catch any overflow.   An overflow means
that a long-branch stub is needed,
so if we still have branch_type == ST_BRANCH_UNKNOWN at this point,
it's good that the user
is informed that something is wrong?


> A testcase would also be useful, so that we don't need to run GCC to find validate this.
>
Sure I'll add one.

Thanks,

Christophe

> R.