[PATCH 3/7][LoongArch] BFD support

[Alan Modra]

On Sat, Aug 14, 2021 at 04:14:27PM +0800, Paul Hua via Binutils wrote:
> +static void
> +loongarch_make_plt_header (bfd_vma got_plt_addr, bfd_vma plt_header_addr,
> +			   uint32_t *entry)
> +{
> +  int64_t pcrel = got_plt_addr - plt_header_addr;
> +  int64_t hi = (pcrel & 0x800 ? 1 : 0) + (pcrel >> 12);
> +  int64_t lo = pcrel & 0xfff;
> +  if ((hi >> 19) != 0 && (hi >> 19) != -1)
> +    abort ();

I think the abort can happen due to large input object files or user
scripts with large address differences between .got and .plt.  Rather
than abort you should instead call _bfd_error_handler and
bfd_set_error, and return a bool from this function to indicate
success.

According to the ISO C99 standard, left shifting negative values is
undefined behaviour and right shifting negative values is
implementation defined.  Avoid writing such code if you can.
Something like the following should work.

  bfd_vma pcrel = got_plt_addr - plt_header_addr;
  bfd_vma hi, lo;

  if (pcrel + 0x80000800 > 0xffffffff)
    {
       __bfd_error_handler ....
       bfd_set_error ...
       return false;
    }
  hi = ((pcrel + 0x800) >> 12) & 0xfffff;
  lo = pcrel & 0xfff;

> +loongarch_make_plt_entry (bfd_vma got_plt_entry_addr, bfd_vma plt_entry_addr,
> +			  uint32_t *entry)
> +{
> +  int64_t pcrel = got_plt_entry_addr - plt_entry_addr;
> +  int64_t hi = (pcrel & 0x800 ? 1 : 0) + (pcrel >> 12);
> +  int64_t lo = pcrel & 0xfff;
> +  if ((hi >> 19) != 0 && (hi >> 19) != -1)
> +    abort ();

Same here.

> +  s = bfd_make_section_anyway_with_flags (
> +    abfd, bed->rela_plts_and_copies_p ? ".rela.got" : ".rel.got",

Isn't loongarch always rela?

> +      _bfd_error_handler (_ ("Interl error: unreachable."));

Spelling.

> +    case R_LARCH_SOP_PUSH_DUP:
> +      r = bfd_reloc_outofrange;
> +      if (loongarch_pop (&opr1) != bfd_reloc_ok
> +	  || loongarch_push (opr1) != bfd_reloc_ok
> +	  || loongarch_push (opr1) != bfd_reloc_ok)
> +	break;
> +      r = bfd_reloc_ok;
> +      break;

Better written as

    case R_LARCH_SOP_PUSH_DUP:
      r = loongarch_pop (&opr1);
      if (r == bfd_reloc_ok)
	{
	  r = loongarch_push (opr1);
	  if (r == bfd_reloc_ok)
	     r = loongarch_push (opr1);
	}
      break;

which allows the push/pop routines to return other status values if
that should be necessary some time in the future (I think
bfd_reloc_dangerous would be better for stack over/underflow).
Similar changes should be applied to other cases too.

> +    case R_LARCH_SOP_POP_32_S_10_5:
> +      r = loongarch_pop (&opr1);
> +      if (r != bfd_reloc_ok)
> +	break;
> +      if ((opr1 & ~(uint64_t) 0xf) != 0x0
> +	  && (opr1 & ~(uint64_t) 0xf) != ~(uint64_t) 0xf)
> +	r = bfd_reloc_overflow;
> +      if (r != bfd_reloc_ok)
> +	break;
> +      insn1 = bfd_get (32, input_bfd, contents + rel->r_offset);
> +      insn1 = (insn1 & (~(uint32_t) 0x7c00)) | ((opr1 & 0x1f) << 10);
> +      bfd_put (32, input_bfd, insn1, contents + rel->r_offset);
> +      break;

Where do you check that rel->r_offset is in range of the section
contents before accessing?

> +#define LARCH_ASSERT(cond, bfd_fail_state, message)			      \

This is a rather horrible macro, affecting flow control.  Invoked all
over the place.  Why do only some "bfd_fail_state" values cause
control to break out of the reloc processing loop?  Why does it
handle bfd_reloc_ok and bfd_reloc_continue?

> +static bool
> +loongarch_elf_finish_dynamic_sections (bfd *output_bfd,
> +				       struct bfd_link_info *info)
> +{
> +  bfd *dynobj;
> +  asection *sdyn, *plt, *gotplt;
> +  struct loongarch_elf_link_hash_table *htab;
> +
> +  htab = loongarch_elf_hash_table (info);
> +  BFD_ASSERT (htab);
> +  dynobj = htab->elf.dynobj;
> +  sdyn = bfd_get_linker_section (dynobj, ".dynamic");
> +
> +  if (elf_hash_table (info)->dynamic_sections_created)
> +    {
> +      BFD_ASSERT (htab->elf.splt && sdyn);
> +
> +      if (!loongarch_finish_dyn (output_bfd, info, dynobj, sdyn))
> +	return false;
> +    }
> +
> +  if ((plt = htab->elf.splt))
> +    gotplt = htab->elf.sgotplt;
> +  else if ((plt = htab->elf.iplt))
> +    gotplt = htab->elf.igotplt;
> +
> +  if (plt && 0 < plt->size)
> +    {
> +      size_t i;
> +      uint32_t plt_header[PLT_HEADER_INSNS];
> +      loongarch_make_plt_header (sec_addr (gotplt), sec_addr (plt),
> +				 plt_header);

At low optimisation levels this might result in false warnings about
gotplt being used uninitialised.  (It did for me with gcc-10 -Og.)

-- 
Alan Modra
Australia Development Lab, IBM