| Message ID | 20220402050041.21302-2-palmer@rivosinc.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | RISC-V -march handling improvements | expand |
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
diff --git a/lib.h b/lib.h index b96e3192..2c0d7116 100644 --- a/lib.h +++ b/lib.h @@ -125,6 +125,11 @@ enum phase { #define PASS_OPTIM (1UL << PASS__OPTIM) #define PASS_FINAL (1UL << PASS__FINAL) +enum flag_type { + FLAG_OFF, + FLAG_ON, + FLAG_FORCE_OFF, +}; extern void add_pre_buffer(const char *fmt, ...) FORMAT_ATTR(1); extern void predefine(const char *name, int weak, const char *fmt, ...) FORMAT_ATTR(3); diff --git a/options.c b/options.c index 6704fc8d..41a98240 100644 --- a/options.c +++ b/options.c @@ -23,12 +23,6 @@ # define __GNUC_PATCHLEVEL__ 0 #endif -enum flag_type { - FLAG_OFF, - FLAG_ON, - FLAG_FORCE_OFF -}; - int die_if_error = 0; int do_output = 1; int gcc_major = __GNUC__; diff --git a/target-riscv.c b/target-riscv.c index 6d9113c1..f5cc6cc3 100644 --- a/target-riscv.c +++ b/target-riscv.c @@ -3,6 +3,7 @@ #include "target.h" #include "machine.h" #include <string.h> +#include <stdio.h> #define RISCV_32BIT (1 << 0) #define RISCV_64BIT (1 << 1) @@ -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; ext: for (i = 0; i < ARRAY_SIZE(extensions); i++) { @@ -73,7 +85,7 @@ ext: } } if (arg[0]) - die("invalid argument to '-march': '%s'\n", arg); + goto unknown; } static void init_riscv(const struct target *self)
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. Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> --- lib.h | 5 +++++ options.c | 6 ------ target-riscv.c | 16 ++++++++++++++-- 3 files changed, 19 insertions(+), 8 deletions(-)