[PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Fangrui Song]

There is no assembler syntax for the two relocation types and therefore
they appear to be never used. Remove related ld code.

Link: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/323
---
 bfd/elfnn-riscv.c   | 32 +-------------------------------
 bfd/elfxx-riscv.c   | 32 +++-----------------------------
 include/elf/riscv.h |  2 --
 3 files changed, 4 insertions(+), 62 deletions(-)

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 8f9f0d8a86a..86cb207b573 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -932,16 +932,6 @@ riscv_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
 
 	  break;
 
-	case R_RISCV_GNU_VTINHERIT:
-	  if (!bfd_elf_gc_record_vtinherit (abfd, sec, h, rel->r_offset))
-	    return false;
-	  break;
-
-	case R_RISCV_GNU_VTENTRY:
-	  if (!bfd_elf_gc_record_vtentry (abfd, sec, h, rel->r_addend))
-	    return false;
-	  break;
-
 	default:
 	  break;
 	}
@@ -950,24 +940,6 @@ riscv_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
   return true;
 }
 
-static asection *
-riscv_elf_gc_mark_hook (asection *sec,
-			struct bfd_link_info *info,
-			Elf_Internal_Rela *rel,
-			struct elf_link_hash_entry *h,
-			Elf_Internal_Sym *sym)
-{
-  if (h != NULL)
-    switch (ELFNN_R_TYPE (rel->r_info))
-      {
-      case R_RISCV_GNU_VTINHERIT:
-      case R_RISCV_GNU_VTENTRY:
-	return NULL;
-      }
-
-  return _bfd_elf_gc_mark_hook (sec, info, rel, h, sym);
-}
-
 /* Adjust a symbol defined by a dynamic object and referenced by a
    regular object.  The current definition is in some section of the
    dynamic object, but we're not including those sections.  We have to
@@ -2027,8 +1999,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
       char *msg_buf = NULL;
       bool resolved_to_zero;
 
-      if (howto == NULL
-	  || r_type == R_RISCV_GNU_VTINHERIT || r_type == R_RISCV_GNU_VTENTRY)
+      if (howto == NULL)
 	continue;
 
       /* This is a final link.  */
@@ -5260,7 +5231,6 @@ riscv_elf_merge_symbol_attribute (struct elf_link_hash_entry *h,
 #define elf_backend_relocate_section		riscv_elf_relocate_section
 #define elf_backend_finish_dynamic_symbol	riscv_elf_finish_dynamic_symbol
 #define elf_backend_finish_dynamic_sections	riscv_elf_finish_dynamic_sections
-#define elf_backend_gc_mark_hook		riscv_elf_gc_mark_hook
 #define elf_backend_plt_sym_val			riscv_elf_plt_sym_val
 #define elf_backend_grok_prstatus		riscv_elf_grok_prstatus
 #define elf_backend_grok_psinfo			riscv_elf_grok_psinfo
diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
index 0b2021f5cc7..c9636bcbdc8 100644
--- a/bfd/elfxx-riscv.c
+++ b/bfd/elfxx-riscv.c
@@ -599,35 +599,9 @@ static reloc_howto_type howto_table[] =
 	 MINUS_ONE,			/* dst_mask */
 	 false),			/* pcrel_offset */
 
-  /* GNU extension to record C++ vtable hierarchy */
-  HOWTO (R_RISCV_GNU_VTINHERIT,		/* type */
-	 0,				/* rightshift */
-	 8,				/* size */
-	 0,				/* bitsize */
-	 false,				/* pc_relative */
-	 0,				/* bitpos */
-	 complain_overflow_dont,	/* complain_on_overflow */
-	 NULL,				/* special_function */
-	 "R_RISCV_GNU_VTINHERIT",	/* name */
-	 false,				/* partial_inplace */
-	 0,				/* src_mask */
-	 0,				/* dst_mask */
-	 false),			/* pcrel_offset */
-
-  /* GNU extension to record C++ vtable member usage */
-  HOWTO (R_RISCV_GNU_VTENTRY,		/* type */
-	 0,				/* rightshift */
-	 8,				/* size */
-	 0,				/* bitsize */
-	 false,				/* pc_relative */
-	 0,				/* bitpos */
-	 complain_overflow_dont,	/* complain_on_overflow */
-	 _bfd_elf_rel_vtable_reloc_fn,	/* special_function */
-	 "R_RISCV_GNU_VTENTRY",		/* name */
-	 false,				/* partial_inplace */
-	 0,				/* src_mask */
-	 0,				/* dst_mask */
-	 false),			/* pcrel_offset */
+  /* 41 and 42 are reserved.  */
+  EMPTY_HOWTO (0),
+  EMPTY_HOWTO (0),
 
   /* Indicates an alignment statement.  The addend field encodes how many
      bytes of NOPs follow the statement.  The desired alignment is the
diff --git a/include/elf/riscv.h b/include/elf/riscv.h
index d0acf6886d8..9b3ea376ff3 100644
--- a/include/elf/riscv.h
+++ b/include/elf/riscv.h
@@ -71,8 +71,6 @@ START_RELOC_NUMBERS (elf_riscv_reloc_type)
   RELOC_NUMBER (R_RISCV_SUB16, 38)
   RELOC_NUMBER (R_RISCV_SUB32, 39)
   RELOC_NUMBER (R_RISCV_SUB64, 40)
-  RELOC_NUMBER (R_RISCV_GNU_VTINHERIT, 41)
-  RELOC_NUMBER (R_RISCV_GNU_VTENTRY, 42)
   RELOC_NUMBER (R_RISCV_ALIGN, 43)
   RELOC_NUMBER (R_RISCV_RVC_BRANCH, 44)
   RELOC_NUMBER (R_RISCV_RVC_JUMP, 45)
-- 
2.37.1.559.g78731f0fdb-goog

Re: [PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Nelson Chu]

Not sure about the history, but if we never use these two relocations,
then removing them makes sense to me.  Btw, just curious that, will we
use numbers 41 and 42 first when adding new relocations?

On Mon, Aug 8, 2022 at 2:55 PM Fangrui Song <maskray@google.com> wrote:
>
> There is no assembler syntax for the two relocation types and therefore
> they appear to be never used. Remove related ld code.
>
> Link: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/323
> ---
>  bfd/elfnn-riscv.c   | 32 +-------------------------------
>  bfd/elfxx-riscv.c   | 32 +++-----------------------------
>  include/elf/riscv.h |  2 --
>  3 files changed, 4 insertions(+), 62 deletions(-)
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 8f9f0d8a86a..86cb207b573 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -932,16 +932,6 @@ riscv_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
>
>           break;
>
> -       case R_RISCV_GNU_VTINHERIT:
> -         if (!bfd_elf_gc_record_vtinherit (abfd, sec, h, rel->r_offset))
> -           return false;
> -         break;
> -
> -       case R_RISCV_GNU_VTENTRY:
> -         if (!bfd_elf_gc_record_vtentry (abfd, sec, h, rel->r_addend))
> -           return false;
> -         break;
> -
>         default:
>           break;
>         }
> @@ -950,24 +940,6 @@ riscv_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
>    return true;
>  }
>
> -static asection *
> -riscv_elf_gc_mark_hook (asection *sec,
> -                       struct bfd_link_info *info,
> -                       Elf_Internal_Rela *rel,
> -                       struct elf_link_hash_entry *h,
> -                       Elf_Internal_Sym *sym)
> -{
> -  if (h != NULL)
> -    switch (ELFNN_R_TYPE (rel->r_info))
> -      {
> -      case R_RISCV_GNU_VTINHERIT:
> -      case R_RISCV_GNU_VTENTRY:
> -       return NULL;
> -      }
> -
> -  return _bfd_elf_gc_mark_hook (sec, info, rel, h, sym);
> -}
> -
>  /* Adjust a symbol defined by a dynamic object and referenced by a
>     regular object.  The current definition is in some section of the
>     dynamic object, but we're not including those sections.  We have to
> @@ -2027,8 +1999,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
>        char *msg_buf = NULL;
>        bool resolved_to_zero;
>
> -      if (howto == NULL
> -         || r_type == R_RISCV_GNU_VTINHERIT || r_type == R_RISCV_GNU_VTENTRY)
> +      if (howto == NULL)
>         continue;
>
>        /* This is a final link.  */
> @@ -5260,7 +5231,6 @@ riscv_elf_merge_symbol_attribute (struct elf_link_hash_entry *h,
>  #define elf_backend_relocate_section           riscv_elf_relocate_section
>  #define elf_backend_finish_dynamic_symbol      riscv_elf_finish_dynamic_symbol
>  #define elf_backend_finish_dynamic_sections    riscv_elf_finish_dynamic_sections
> -#define elf_backend_gc_mark_hook               riscv_elf_gc_mark_hook
>  #define elf_backend_plt_sym_val                        riscv_elf_plt_sym_val
>  #define elf_backend_grok_prstatus              riscv_elf_grok_prstatus
>  #define elf_backend_grok_psinfo                        riscv_elf_grok_psinfo
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 0b2021f5cc7..c9636bcbdc8 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -599,35 +599,9 @@ static reloc_howto_type howto_table[] =
>          MINUS_ONE,                     /* dst_mask */
>          false),                        /* pcrel_offset */
>
> -  /* GNU extension to record C++ vtable hierarchy */
> -  HOWTO (R_RISCV_GNU_VTINHERIT,                /* type */
> -        0,                             /* rightshift */
> -        8,                             /* size */
> -        0,                             /* bitsize */
> -        false,                         /* pc_relative */
> -        0,                             /* bitpos */
> -        complain_overflow_dont,        /* complain_on_overflow */
> -        NULL,                          /* special_function */
> -        "R_RISCV_GNU_VTINHERIT",       /* name */
> -        false,                         /* partial_inplace */
> -        0,                             /* src_mask */
> -        0,                             /* dst_mask */
> -        false),                        /* pcrel_offset */
> -
> -  /* GNU extension to record C++ vtable member usage */
> -  HOWTO (R_RISCV_GNU_VTENTRY,          /* type */
> -        0,                             /* rightshift */
> -        8,                             /* size */
> -        0,                             /* bitsize */
> -        false,                         /* pc_relative */
> -        0,                             /* bitpos */
> -        complain_overflow_dont,        /* complain_on_overflow */
> -        _bfd_elf_rel_vtable_reloc_fn,  /* special_function */
> -        "R_RISCV_GNU_VTENTRY",         /* name */
> -        false,                         /* partial_inplace */
> -        0,                             /* src_mask */
> -        0,                             /* dst_mask */
> -        false),                        /* pcrel_offset */
> +  /* 41 and 42 are reserved.  */
> +  EMPTY_HOWTO (0),
> +  EMPTY_HOWTO (0),
>
>    /* Indicates an alignment statement.  The addend field encodes how many
>       bytes of NOPs follow the statement.  The desired alignment is the
> diff --git a/include/elf/riscv.h b/include/elf/riscv.h
> index d0acf6886d8..9b3ea376ff3 100644
> --- a/include/elf/riscv.h
> +++ b/include/elf/riscv.h
> @@ -71,8 +71,6 @@ START_RELOC_NUMBERS (elf_riscv_reloc_type)
>    RELOC_NUMBER (R_RISCV_SUB16, 38)
>    RELOC_NUMBER (R_RISCV_SUB32, 39)
>    RELOC_NUMBER (R_RISCV_SUB64, 40)
> -  RELOC_NUMBER (R_RISCV_GNU_VTINHERIT, 41)
> -  RELOC_NUMBER (R_RISCV_GNU_VTENTRY, 42)
>    RELOC_NUMBER (R_RISCV_ALIGN, 43)
>    RELOC_NUMBER (R_RISCV_RVC_BRANCH, 44)
>    RELOC_NUMBER (R_RISCV_RVC_JUMP, 45)
> --
> 2.37.1.559.g78731f0fdb-goog
>

Re: [PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Andreas Schwab]

On Aug 07 2022, Fangrui Song via Binutils wrote:

> There is no assembler syntax for the two relocation types and therefore
> they appear to be never used. Remove related ld code.

FWIW, they are still generically handled by the .vtable_inherit and
.vtable_entry pseudo-ops, but support for -fvtable-gc has been removed
from gcc in 2003.  The RISC-V assembler never picked them up.

$ riscv64-suse-linux-as vtable.s 
vtable.s: Assembler messages:
vtable.s:1: Error: cannot represent BFD_RELOC_VTABLE_ENTRY relocation in object file
vtable.s:2: Error: cannot represent BFD_RELOC_VTABLE_INHERIT relocation in object file

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Kito Cheng]

Hi Andreas:

> FWIW, they are still generically handled by the .vtable_inherit and
> .vtable_entry pseudo-ops, but support for -fvtable-gc has been removed
> from gcc in 2003.  The RISC-V assembler never picked them up.

Thanks for the historical data! RISC-V GNU toolchain is upstreamed
after that time, so sounds like we could remove that safely for
RISC-V.

> $ riscv64-suse-linux-as vtable.s
> vtable.s: Assembler messages:
> vtable.s:1: Error: cannot represent BFD_RELOC_VTABLE_ENTRY relocation in object file
> vtable.s:2: Error: cannot represent BFD_RELOC_VTABLE_INHERIT relocation in object file

Maybe we can improve the error message into something like:
.vtable_inherit / .vtable_entry is unsupported for RISC-V.

Re: [PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Fangrui Song]

On 2022-08-08, Kito Cheng wrote:
>Hi Andreas:
>
>> FWIW, they are still generically handled by the .vtable_inherit and
>> .vtable_entry pseudo-ops, but support for -fvtable-gc has been removed
>> from gcc in 2003.  The RISC-V assembler never picked them up.
>
>Thanks for the historical data! RISC-V GNU toolchain is upstreamed
>after that time, so sounds like we could remove that safely for
>RISC-V.

Second this.

>> $ riscv64-suse-linux-as vtable.s
>> vtable.s: Assembler messages:
>> vtable.s:1: Error: cannot represent BFD_RELOC_VTABLE_ENTRY relocation in object file
>> vtable.s:2: Error: cannot represent BFD_RELOC_VTABLE_INHERIT relocation in object file
>
>Maybe we can improve the error message into something like:
>.vtable_inherit / .vtable_entry is unsupported for RISC-V.

I think the diagnostic is from gas/config/tc-riscv.c:4135 .
Since gcc -fvtable-gc was gone in 2003 and we essentially cannot find
.vtable_entry uses, I think sticking with the existing generic diagnostic isn't bad.

Re: [PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Palmer Dabbelt]

On Mon, 08 Aug 2022 10:27:42 PDT (-0700), binutils@sourceware.org wrote:
> On 2022-08-08, Kito Cheng wrote:
>>Hi Andreas:
>>
>>> FWIW, they are still generically handled by the .vtable_inherit and
>>> .vtable_entry pseudo-ops, but support for -fvtable-gc has been removed
>>> from gcc in 2003.  The RISC-V assembler never picked them up.
>>
>>Thanks for the historical data! RISC-V GNU toolchain is upstreamed
>>after that time, so sounds like we could remove that safely for
>>RISC-V.
>
> Second this.

Given the age I'm assuming LLVM never supported this on RISC-V either, 
so this seems reasonable to me.

IMO we can't re-use the relocation numbers, so maybe we should leave 
some sort of deprecated stub in there just in case?  Not sure that's 
even worth it in this case, though.

>>> $ riscv64-suse-linux-as vtable.s
>>> vtable.s: Assembler messages:
>>> vtable.s:1: Error: cannot represent BFD_RELOC_VTABLE_ENTRY relocation in object file
>>> vtable.s:2: Error: cannot represent BFD_RELOC_VTABLE_INHERIT relocation in object file
>>
>>Maybe we can improve the error message into something like:
>>.vtable_inherit / .vtable_entry is unsupported for RISC-V.
>
> I think the diagnostic is from gas/config/tc-riscv.c:4135 .
> Since gcc -fvtable-gc was gone in 2003 and we essentially cannot find
> .vtable_entry uses, I think sticking with the existing generic diagnostic isn't bad.

Re: [PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Fangrui Song]

On Mon, Aug 8, 2022 at 10:27 AM Fangrui Song <maskray@google.com> wrote:
>
> On 2022-08-08, Kito Cheng wrote:
> >Hi Andreas:
> >
> >> FWIW, they are still generically handled by the .vtable_inherit and
> >> .vtable_entry pseudo-ops, but support for -fvtable-gc has been removed
> >> from gcc in 2003.  The RISC-V assembler never picked them up.
> >
> >Thanks for the historical data! RISC-V GNU toolchain is upstreamed
> >after that time, so sounds like we could remove that safely for
> >RISC-V.
>
> Second this.
>
> >> $ riscv64-suse-linux-as vtable.s
> >> vtable.s: Assembler messages:
> >> vtable.s:1: Error: cannot represent BFD_RELOC_VTABLE_ENTRY relocation in object file
> >> vtable.s:2: Error: cannot represent BFD_RELOC_VTABLE_INHERIT relocation in object file
> >
> >Maybe we can improve the error message into something like:
> >.vtable_inherit / .vtable_entry is unsupported for RISC-V.
>
> I think the diagnostic is from gas/config/tc-riscv.c:4135 .
> Since gcc -fvtable-gc was gone in 2003 and we essentially cannot find
> .vtable_entry uses, I think sticking with the existing generic diagnostic isn't bad.

If this patch has no objection, I will push this.

Re: [PATCH] RISC-V: Remove R_RISCV_GNU_VTINHERIT/R_RISCV_GNU_VTENTRY

[Kito Cheng]

LGTM, although I am not one of RISC-V binutils maintainer :P


On Wed, Aug 10, 2022 at 2:33 PM Fangrui Song <maskray@google.com> wrote:
>
> On Mon, Aug 8, 2022 at 10:27 AM Fangrui Song <maskray@google.com> wrote:
> >
> > On 2022-08-08, Kito Cheng wrote:
> > >Hi Andreas:
> > >
> > >> FWIW, they are still generically handled by the .vtable_inherit and
> > >> .vtable_entry pseudo-ops, but support for -fvtable-gc has been removed
> > >> from gcc in 2003.  The RISC-V assembler never picked them up.
> > >
> > >Thanks for the historical data! RISC-V GNU toolchain is upstreamed
> > >after that time, so sounds like we could remove that safely for
> > >RISC-V.
> >
> > Second this.
> >
> > >> $ riscv64-suse-linux-as vtable.s
> > >> vtable.s: Assembler messages:
> > >> vtable.s:1: Error: cannot represent BFD_RELOC_VTABLE_ENTRY relocation in object file
> > >> vtable.s:2: Error: cannot represent BFD_RELOC_VTABLE_INHERIT relocation in object file
> > >
> > >Maybe we can improve the error message into something like:
> > >.vtable_inherit / .vtable_entry is unsupported for RISC-V.
> >
> > I think the diagnostic is from gas/config/tc-riscv.c:4135 .
> > Since gcc -fvtable-gc was gone in 2003 and we essentially cannot find
> > .vtable_entry uses, I think sticking with the existing generic diagnostic isn't bad.
>
> If this patch has no objection, I will push this.