Re: [PATCH v1 1/6] RISC-V: Respect -Wsparse-error for -march errors

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv