Re: [PATCH] kvx: fix 32-bit build and validation

[Paul Iannetta <piannetta@kalrayinc.com>]

Thank you very much for your comments!

On Wed, Aug 23, 2023 at 09:57:26AM +0930, Alan Modra wrote:
> On Tue, Aug 22, 2023 at 06:01:42PM +0200, Paul Iannetta via Binutils wrote:
> >    kvx-*-*)
> > -    targ_defvec=kvx_elf32_vec
> > -#ifdef BFD64
> > -    targ64_selvecs=kvx_elf64_vec
> > -#endif
> > +    targ_defvec=kvx_elf64_vec
> > +    targ_selvecs="kvx_elf64_vec kvx_elf32_vec"
> > +    want64=true
> 
> It isn't necessary to put kvx_elf64_vec into targ_selvecs, but doesn't
> hurt.  I mention this only because..

I'll remove it then.

> 
> > -    kvx_elf64_vec)		 tb="$tb elf64-kvx.lo elfxx-kvx.lo elf64.lo $elf $ipa" ;;
> > -    kvx_elf64_linux_vec)	 tb="$tb elf64-kvx.lo elfxx-kvx.lo elf64.lo $elf $ipa" ;;
> > +    kvx_elf64_vec)		 tb="$tb elf64-kvx.lo elfxx-kvx.lo elf64.lo $elf $ipa"; target_size=64 ;;
> > +    kvx_elf64_linux_vec)	 tb="$tb elf64-kvx.lo elfxx-kvx.lo elf64.lo $elf $ipa"; target_size=64 ;;
> 
> ..I don't see kvx_elf64_linux_vec defined anywhere.  Delete it?

Indeed, it somehow got missing from bfd/targets.c, it is mostly
similar to kvx_elf64_vec, and only exists to match what is done in the
linker with the emulation elf64kvx_linux.

> 
> >    if (class == classes->imm_classes)
> >      {
> > -      unsigned long long uval = token_val_p
> > +      uint64_t uval = token_val_p
> >  	? token->val
> >  	: strtoull (tok + (tok[0] == '-') + (tok[0] == '+'), NULL, 0);
> > -      long long val = uval;
> > -      long long pval = val < 0 ? -val : val;
> > -      int neg_power2_p = val < 0 && !(pval & (pval - 1));
> > +      uint64_t sign_bit = ((~0ULL >> 1) ^ ~0ULL);
> > +      int64_t pval = sign_bit & uval ? 1 + (~0ULL >> 1) - (~sign_bit & uval) : (~sign_bit & uval);
> > +      int neg_power2_p = (sign_bit & uval) && !(pval & (pval - 1));
> >        unsigned int len = 8 * sizeof (pval) - __builtin_clzll (pval);
> >        for (; class[cur].class_id != -1
> >  	  && ((unsigned int) (class[cur].sz < 0 ? - class[cur].sz - !neg_power2_p : class[cur].sz) < len
> 
> I'm about to commit a patch I wrote yesterday that fixes ubsan
> warnings in the above, which will make this change unnecessary (except
> of course you might like to make the variables {u,}int64_t).  The
> expressions you're using for sign_bit and pval to avoid signed integer
> overflow will no doubt work (unless unsigned long long is larger than
> uint64_t!), but are more complicated than the following.
> 
>  int64_t val = uval;
>  int64_t pval = val < 0 ? -uval : uval;
>  int neg_power2_p = val < 0 && !(uval & (uval - 1));

Thanks, I'll much prefer keeping something readable.

> 
> > index 3cf6b27976d..00aec6c858e 100644
> > --- a/gas/config/tc-kvx.c
> > +++ b/gas/config/tc-kvx.c
> > @@ -1419,6 +1419,9 @@ kvx_set_cpu (void)
> >    if (env.params.core == -1)
> >        env.params.core = kvx_core_info->elf_core;
> >  
> > +  if (!strcmp (TARGET_OS, "linux-gnu"))
> > +      env.params.osabi = ELFOSABI_GNU;
> > +
> >    int kvx_bfd_mach;
> >    print_insn = kvx_print_insn;
> >  
> 
> Do you really need this?  Other targets automatically use ELFOSABI_GNU
> when the ELF bfd code detects known ABI extensions.  See
> has_gnu_osabi.  Hmm, likely you shouldn't be setting e_ident in
> tc-kvx.c:kvx_end.

I thought I needed this because we may emit STB_GNU_UNIQUE symbols but
I was not aware of the fact that has_gnu_osabi updates itself
depending on the features used.  I'll fix that part accordingly.

Thanks,
Paul