Re: [PATCH v2] RISC-V: Add --march=help

[Tsukasa OI <research_trasio@irq.a4lg.com>]

Hi Hau,

I'm not yet ready to come back to Binutils development (because I
haven't finished writing my research paper) but I'd like to comment.

While your idea seems great at first glance, I would never like to see
"help" handling in the riscv_parse_subset function because many other
functions call it (some calls this function with an argument retrieved
from an assembly / object file and your design seems vulnerable from
adversarial files e.g. when disassembling files with unknown origin).

Besides that, I think that's not a bad idea (quite rare to see in the
GNU toolchain, though).

IMHO, the right place to put "help" handling (at least, its entry point)
is the md_parse_option function in gas/config/tc-riscv.c.  Exiting
successfully from here is rare but at least cris and kvx does so for
help-like options.

Thanks,
Tsukasa

On 2024/01/30 15:36, Hau Hsu wrote:
> Use --march=help to print all supported extensions and versions.
> 
> This patch assumes that the supported extensions with the same versions
> are listed together.
> 
> For example
> static struct riscv_supported_ext riscv_supported_std_ext[] =
> {
>   ...
>   {"i",         ISA_SPEC_CLASS_20191213,        2, 1, 0 },
>   {"i",         ISA_SPEC_CLASS_20190608,        2, 1, 0 },
>   {"i",         ISA_SPEC_CLASS_2P2,             2, 0, 0 },
>   ...
> };
> 
> For "i" extension, 2.1.0 with different spec class are listed together.
> This patch records the previous printed extension and version.  If the
> current extension and version are the same as the previous one, skip
> printing.
> 
> Here is part of the output of `as -march=help`:
> 
> All available -march extensions for RISC-V:
>         e                                       1.9
>         i                                       2.1, 2.0
>         m                                       2.0
>         a                                       2.1, 2.0
>         f                                       2.2, 2.0
>         d                                       2.2, 2.0
>         q                                       2.2, 2.0
>         c                                       2.0
>         v                                       1.0
>         h                                       1.0
>         zicbom                                  1.0
>         zicbop                                  1.0
> 
> Signed-off-by: Hau Hsu <hau.hsu@sifive.com>
> ---
>  bfd/ChangeLog     |  9 ++++++
>  bfd/elfxx-riscv.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/bfd/ChangeLog b/bfd/ChangeLog
> index 97d0c585a56..0807a2e0bcb 100644
> --- a/bfd/ChangeLog
> +++ b/bfd/ChangeLog
> @@ -1,3 +1,12 @@
> +2024-01-17 Hau Hsu  <hau.hsu@sifive.com>
> +
> +	* elfxx-riscv.c (riscv_parse_subset): Parse 'help' keyword to print
> +	  available extension and versions.
> +	(riscv_print_extensions): New function.
> +	(riscv_same_extension_version): New function.
> +	(riscv_same_extension_diff_version): New function.
> +	(riscv_valid_ext): New function.
> +
>  2024-01-15  Nick Clifton  <nickc@redhat.com>
>  
>  	* 2.42 branch point.
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 9a121b47121..bb394a589b1 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -2051,6 +2051,71 @@ riscv_set_default_arch (riscv_parse_subset_t *rps)
>      }
>  }
>  
> +static
> +bool riscv_same_extension_version(
> +  const struct riscv_supported_ext* ext1,
> +  const struct riscv_supported_ext* ext2)
> +{
> +  return (strcmp(ext1->name, ext2->name) == 0
> +          && ext1->major_version == ext2->major_version
> +          && ext1->minor_version == ext2->minor_version);
> +}
> +
> +static
> +bool riscv_same_extension_diff_version(
> +  const struct riscv_supported_ext* ext1,
> +  const struct riscv_supported_ext* ext2)
> +{
> +  return (strcmp(ext1->name, ext2->name) == 0
> +          && !(ext1->major_version == ext2->major_version
> +               && ext1->minor_version == ext2->minor_version));
> +}
> +
> +static
> +bool riscv_valid_ext(const struct riscv_supported_ext *ext)
> +{
> +  return (ext->isa_spec_class != ISA_SPEC_CLASS_NONE
> +          && ext->major_version != RISCV_UNKNOWN_VERSION
> +          && ext->minor_version != RISCV_UNKNOWN_VERSION);
> +}
> +
> +static
> +void riscv_print_extensions(void)
> +{
> +  /* Record the previous pritned extension.
> +     Print the current one if they are not the same.  */
> +  const struct riscv_supported_ext *cur = NULL, *prev = NULL;
> +
> +  int i, j;
> +  printf ("All available -march extensions for RISC-V:");
> +  for (i = 0; riscv_all_supported_ext[i] != NULL; i++)
> +    {
> +      const struct riscv_supported_ext *exts = riscv_all_supported_ext[i];
> +      prev = NULL;
> +      for (j = 0; exts[j].name != NULL; j++)
> +        {
> +          cur = &exts[j];
> +          if (!riscv_valid_ext (cur))
> +            continue;
> +
> +          if (prev && riscv_same_extension_version (prev, cur))
> +            continue;
> +
> +          if (!prev || !riscv_same_extension_diff_version (prev, cur))
> +            {
> +              printf("\n\t%-40s%d.%d", cur->name, cur->major_version, cur->minor_version);
> +              prev = &exts[j];
> +            }
> +          else
> +            {
> +              printf(", %d.%d", cur->major_version, cur->minor_version);
> +              prev = &exts[j];
> +            }
> +        }
> +    }
> +  printf ("\n");
> +}
> +
>  /* Function for parsing ISA string.
>  
>     Return Value:
> @@ -2089,6 +2154,13 @@ riscv_parse_subset (riscv_parse_subset_t *rps,
>      }
>  
>    p = arch;
> +  /* List all avaiable archs. */
> +  if (strcmp (p, "help") == 0)
> +    {
> +      riscv_print_extensions();
> +      exit (EXIT_SUCCESS);
> +    }
> +
>    if (startswith (p, "rv32"))
>      {
>        *rps->xlen = 32;