From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
To: Palmer Dabbelt <palmer@rivosinc.com>
Cc: linux-sparse@vger.kernel.org, Kito Cheng <kito.cheng@gmail.com>,
linux-riscv@lists.infradead.org, mkl@pengutronix.de,
aurelien@aurel32.net,
Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH v1 1/6] RISC-V: Respect -Wsparse-error for -march errors
Date: Sat, 21 May 2022 23:46:18 +0200 [thread overview]
Message-ID: <20220521214618.2i5xokersg3hhzfn@mail> (raw)
In-Reply-To: <20220402050041.21302-2-palmer@rivosinc.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
next prev parent reply other threads:[~2022-05-21 21:46 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-02 5:00 [PATCH v1 0/6] RISC-V -march handling improvements Palmer Dabbelt
2022-04-02 5:00 ` [PATCH v1 1/6] RISC-V: Respect -Wsparse-error for -march errors Palmer Dabbelt
2022-05-21 21:46 ` Luc Van Oostenryck [this message]
2022-04-02 5:00 ` [PATCH v1 2/6] RISC-V: Match GCC's semantics for multiple -march instances Palmer Dabbelt
2022-05-21 21:52 ` Luc Van Oostenryck
2022-04-02 5:00 ` [PATCH v1 3/6] RISC-V: Remove the unimplemented ISA extensions Palmer Dabbelt
2022-05-21 22:05 ` Luc Van Oostenryck
2022-04-02 5:00 ` [PATCH v1 4/6] RISC-V: Remove "g" from the extension list Palmer Dabbelt
2022-04-02 5:00 ` [PATCH v1 5/6] RISC-V: Add the Zicsr extension Palmer Dabbelt
2022-04-02 5:00 ` [PATCH v1 6/6] RISC-V: Add the Zifencei extension Palmer Dabbelt
2022-04-04 17:52 ` [PATCH v1 0/6] RISC-V -march handling improvements Marc Kleine-Budde
2022-04-04 23:15 ` Palmer Dabbelt
2022-05-21 21:23 ` Luc Van Oostenryck
2022-07-05 23:09 ` Palmer Dabbelt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220521214618.2i5xokersg3hhzfn@mail \
--to=luc.vanoostenryck@gmail.com \
--cc=aurelien@aurel32.net \
--cc=kito.cheng@gmail.com \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-sparse@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=palmer@rivosinc.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).