On Fri, Apr 01, 2022 at 10:00:36PM -0700, Palmer Dabbelt wrote:
> Parsing RISC-V ISA strings is extremely complicated: there are many
> extensions, versions of extensions, versions of the ISA string rules,
> and a bunch of unwritten rules to deal with all the bugs that fell out
> of that complexity.
>
> Rather than forcing users to see an error when the ISA string parsing
> fails, just stop parsing where we get lost. Changes tend to end up at
> the end of the ISA string, so that's probably going to work (and if
> it doesn't there's a warning to true and clue folks in).
>
> This does have the oddity in that "-Wsparse-error -march=..." behaves
> differently than "-march... -Wsparse-error", but that's already the case
> for "--arch=... -march=..." and "-march=... --arch=...". Both
> "-Wsparse-error" and "--arch" are sparse-specific arguments, so they're
> probably both going to be in the same place.
>
> diff --git a/target-riscv.c b/target-riscv.c
> index 6d9113c1..f5cc6cc3 100644
> --- a/target-riscv.c
> +++ b/target-riscv.c
> @@ -60,7 +61,18 @@ static void parse_march_riscv(const char *arg)
> goto ext;
> }
> }
> - die("invalid argument to '-march': '%s'\n", arg);
> +
> +unknown:
> + /*
> + * This behaves like do_warn() / do_error(), but we don't have a
> + * position so it's just inline here.
> + */
> + fflush(stdout);
> + fprintf(stderr, "%s: invalid argument to '-march': '%s'\n",
> + Wsparse_error == FLAG_ON ? "error" : "warning", arg);
> + if (Wsparse_error == FLAG_ON)
> + has_error |= ERROR_CURR_PHASE;
> + return;
I don't like this because:
1) it's way too much intimate with the way options are parsed
(enum flag_type should stay local to options.c).
2) -Wsparse-error is a kind of hack to ignore -Werror but keep
a way to invoke its semantic (but I don' think anyone is using it).
3) I don't think -Wsparse-error (or GCC's -Werror) should be concerned
with the parsing of options.
I think it would be fine, for now, to always simply report a warning,
like Linus' patch (but I would prefer to just handle the correct parsing).
If reporting an error is important, then I would be happy to jut move
this code into an helper defined in "options.c".
Best regards,
-- Luc
