Re: [PATCH v3 02/44] bfd: add intelgt target to BFD

[Jan Beulich <jbeulich@suse.com>]

On 01.08.2025 11:37, Tankut Baris Aktemur wrote:
> --- a/bfd/config.bfd
> +++ b/bfd/config.bfd
> @@ -198,6 +198,7 @@ fido*)		 targ_archs=bfd_m68k_arch ;;
>  hppa*)		 targ_archs=bfd_hppa_arch ;;
>  i[3-7]86)	 targ_archs=bfd_i386_arch ;;
>  ia16)		 targ_archs=bfd_i386_arch ;;
> +intelgt)	 targ_archs=bfd_intelgt_arch ;;
>  kvx)		 targ_archs=bfd_kvx_arch ;;
>  loongarch*)	 targ_archs=bfd_loongarch_arch ;;
>  m6811*|m68hc11*) targ_archs="bfd_m68hc11_arch bfd_m68hc12_arch bfd_m9s12x_arch bfd_m9s12xg_arch" ;;
> @@ -705,12 +706,12 @@ case "${targ}" in
>      ;;
>    x86_64-*-linux-*)
>      targ_defvec=x86_64_elf64_vec
> -    targ_selvecs="i386_elf32_vec iamcu_elf32_vec x86_64_elf32_vec i386_pei_vec x86_64_pe_vec x86_64_pei_vec"
> +    targ_selvecs="i386_elf32_vec iamcu_elf32_vec x86_64_elf32_vec i386_pei_vec x86_64_pe_vec x86_64_pei_vec intelgt_elf64_vec"
>      want64=true
>      ;;
>    x86_64-*-mingw* | x86_64-*-pe | x86_64-*-pep | x86_64-*-cygwin)
>      targ_defvec=x86_64_pe_vec
> -    targ_selvecs="x86_64_pe_vec x86_64_pei_vec x86_64_pe_big_vec x86_64_elf64_vec i386_pe_vec i386_pei_vec i386_elf32_vec iamcu_elf32_vec pdb_vec"
> +    targ_selvecs="x86_64_pe_vec x86_64_pei_vec x86_64_pe_big_vec x86_64_elf64_vec i386_pe_vec i386_pei_vec i386_elf32_vec iamcu_elf32_vec pdb_vec intelgt_elf64_vec intelgt_legacy_elf64_vec"
>      want64=true
>      targ_underscore=no
>      ;;

I'm not convinced of forcing this as secondary target onto (about) everyone.

> @@ -781,7 +782,13 @@ case "${targ}" in
>      targ_defvec=i386_elf32_vec
>      targ_selvecs="i386_msdos_vec i386_aout_vec"
>      ;;
> -

Please don't remove the (visually) separating blank lines; instead add a new
one yourself.

> +#ifdef BFD64
> +  intelgt-*-elf)
> +    targ_defvec=intelgt_elf64_vec
> +    targ_selvecs="intelgt_elf64_vec"

Why would targ_selvecs need to hold what targ_defvec already has?

> --- /dev/null
> +++ b/bfd/elf64-intelgt.c
> @@ -0,0 +1,195 @@
> +/* Intel(R) Graphics Technology-specific support for ELF
> +   Copyright (C) 2022-2025 Free Software Foundation, Inc.
> +
> +   This file is part of BFD, the Binary File Descriptor library.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
> +   MA 02110-1301, USA.  */
> +
> +#include "sysdep.h"
> +#include "bfd.h"
> +#include "libbfd.h"
> +#include "elf-bfd.h"
> +
> +#include "elf/common.h"
> +#include "elf/intelgt.h"
> +
> +#define MINUS_ONE (~ (bfd_vma) 0)
> +
> +#define INTELGT_ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))

What's wrong with ARRAY_SIZE()?

> +static bool
> +elf64_intelgt_elf_object_p (bfd *abfd)
> +{
> +  return bfd_default_set_arch_mach (abfd, bfd_arch_intelgt, bfd_mach_intelgt);
> +}
> +
> +/* Map BFD relocs to the IntelGT relocs.  */
> +struct elf_reloc_map
> +{
> +  bfd_reloc_code_real_type bfd_reloc_val;
> +  unsigned char elf_reloc_val;
> +};
> +
> +static const struct elf_reloc_map elf64_intelgt_reloc_map[] =
> +{
> +  { BFD_RELOC_64, R_ZE_SYM_ADDR },
> +  { BFD_RELOC_32, R_ZE_SYM_ADDR_32 },
> +  { BFD_RELOC_ZE_SYM_ADDR32_HI, R_ZE_SYM_ADDR32_HI },
> +  { BFD_RELOC_ZE_PER_THREAD_PAYLOAD_OFFSET_32,
> +    R_PER_THREAD_PAYLOAD_OFFSET_32 },
> +};
> +
> +static reloc_howto_type elf64_intelgt_howto_table[] =
> +{
> +  HOWTO (R_ZE_NONE,		/* type */
> +	 0,			/* rightshift */
> +	 0,			/* size (0 = byte, 1 = short, 2 = long) */
> +	 0,			/* bitsize */
> +	 false,			/* pc_relative */
> +	 0,			/* bitpos */
> +	 complain_overflow_dont,/* complain_on_overflow */
> +	 NULL,			/* special_function */
> +	 "R_ZE_NONE",		/* name */
> +	 false,			/* partial_inplace */
> +	 0,			/* src_mask */
> +	 0,			/* dst_mask */
> +	 false),		/* pcrel_offset */
> +  HOWTO (R_ZE_SYM_ADDR,		/* type */
> +	 0,			/* rightshift */
> +	 2,			/* size (0 = byte, 1 = short, 2 = long) */
> +	 64,			/* bitsize */
> +	 false,			/* pc_relative */
> +	 0,			/* bitpos */
> +	 complain_overflow_unsigned,	/* complain_on_overflow */
> +	 bfd_elf_generic_reloc, /* special_function */
> +	 "R_ZE_SYM_ADDR",	/* name */
> +	 false,			/* partial_inplace */
> +	 MINUS_ONE,		/* src_mask */

Based on the arch apparently using RELA relocations, src_mask shouldn't be
non-zero anywhere aiui.

> +	 MINUS_ONE,		/* dst_mask */

While for a 64-bit reloc this looks right, ...

> +	 false),		/* pcrel_offset */
> +  HOWTO (R_ZE_SYM_ADDR_32,	/* type */
> +	 0,			/* rightshift */
> +	 2,			/* size (0 = byte, 1 = short, 2 = long) */
> +	 32,			/* bitsize */
> +	 false,			/* pc_relative */
> +	 0,			/* bitpos */
> +	 complain_overflow_unsigned,	/* complain_on_overflow */
> +	 bfd_elf_generic_reloc, /* special_function */
> +	 "R_ZE_SYM_ADDR_32",	/* name */
> +	 false,			/* partial_inplace */
> +	 MINUS_ONE,		/* src_mask */
> +	 MINUS_ONE,		/* dst_mask */

... I don't think it's right here. I'm also unconvinced of the use of
complain_overflow_unsigned, when ...

> +	 false),		/* pcrel_offset */
> +  HOWTO (R_ZE_SYM_ADDR32_HI,	/* type */
> +	 32,			/* rightshift */
> +	 2,			/* size (0 = byte, 1 = short, 2 = long) */
> +	 32,			/* bitsize */
> +	 false,			/* pc_relative */
> +	 0,			/* bitpos */
> +	 complain_overflow_unsigned,	/* complain_on_overflow */
> +	 bfd_elf_generic_reloc, /* special_function */
> +	 "R_ZE_SYM_ADDR32_HI",	/* name */
> +	 false,			/* partial_inplace */
> +	 MINUS_ONE,		/* src_mask */
> +	 MINUS_ONE,		/* dst_mask */
> +	 false),		/* pcrel_offset */

... the other half of the address can be represented this way.

> --- a/binutils/readelf.c
> +++ b/binutils/readelf.c
> @@ -124,6 +124,7 @@
>  #include "elf/kvx.h"
>  #include "elf/lm32.h"
>  #include "elf/iq2000.h"
> +#include "elf/intelgt.h"
>  #include "elf/m32c.h"
>  #include "elf/m32r.h"
>  #include "elf/m68k.h"
> @@ -2395,6 +2396,10 @@ dump_relocations (Filedata *          filedata,
>  	case EM_AMDGPU:
>  	  rtype = elf_amdgpu_reloc_type (type);
>  	  break;
> +
> +	case EM_INTELGT:
> +	  rtype = elf_intelgt_reloc_type (type);
> +	  break;
>  	}
>  
>        if (rtype == NULL)
> @@ -15589,6 +15594,8 @@ is_32bit_abs_reloc (Filedata * filedata, unsigned int reloc_type)
>        return reloc_type == 1; /* R_XTENSA_32.  */
>      case EM_Z80:
>        return reloc_type == 6; /* R_Z80_32.  */
> +    case EM_INTELGT:
> +      return reloc_type == 2; /* R_ZE_SYM_ADDR_32 */

Not sure here as well, when this can be the lower half of a 64-bit address.

> --- /dev/null
> +++ b/include/elf/intelgt.h
> @@ -0,0 +1,39 @@
> +/* Copyright (C) 2022-2025 Free Software Foundation, Inc.
> +
> +   This file is part of BFD, the Binary File Descriptor library.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
> +   MA 02110-1301, USA.  */
> +
> +/* This file holds definitions specific to the IntelGT ABI.  */
> +
> +#ifndef __INTELGT_H_
> +#define __INTELGT_H_
> +
> +#include "elf/reloc-macros.h"
> +
> +START_RELOC_NUMBERS (elf_intelgt_reloc_type)
> +  RELOC_NUMBER (R_ZE_NONE,			     0)
> +  /* 64-bit address.  */
> +  RELOC_NUMBER (R_ZE_SYM_ADDR,			     1)
> +  /* 32-bit address or lower 32-bit of a 64-bit address.  */
> +  RELOC_NUMBER (R_ZE_SYM_ADDR_32,		     2)
> +  /* Higher 32bits of a 64-bit address.  */
> +  RELOC_NUMBER (R_ZE_SYM_ADDR32_HI,		     3)
> +  /* 32-bit field of payload offset of per-thread data.  */
> +  RELOC_NUMBER (R_PER_THREAD_PAYLOAD_OFFSET_32,      4)

Why is this not R_ZE_PER_THREAD_PAYLOAD_OFFSET_32?

Jan