* [PATCH 0/3] RISC-V: Make ISA parser stricter (with code clarity improvement)
@ 2022-02-23 1:47 Tsukasa OI
2022-02-23 1:47 ` [PATCH 1/3] RISC-V: Remove a loop in the ISA parser Tsukasa OI
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Tsukasa OI @ 2022-02-23 1:47 UTC (permalink / raw)
To: Tsukasa OI; +Cc: binutils
[note: my copyright assignment is already complete on 2022-01-19]
Current GNU Toolchain accepts invalid RISC-V ISA string containing
invalid underscores (double underscores or trailing underscore). This
patchset will reject those.
It's not sure that prohibiting those invalid patterns helps users from
accident but would be better than nothing.
As a part of this patchset, PATCH 1/3 removes unnecessary loop in the
ISA string parser (riscv_parse_subset function). It is a requirement to
reject invalid trailing underscore after a multi-letter extension but it
also improves clarity of the code, regardless of whether invalid
underscores are rejected.
Removal of Unnecessary Loop (PATCH 1/3):
riscv_parse_prefixed_ext function (formerly,
riscv_parse_sv_or_non_std_ext function) used to parse multi-letter
extension with SINGLE type of prefix ('Z', 'S', likewise...). At the
time when this function is created, iterating over prefix type was
absolutely necessary.
However, commit e601909a3287bf541c6a7d82214bb387d2c76d82 ("RISC-V:
Support to parse the multi-letter prefix in the architecture string.")
by Nelson Chu changed the situation. Updated riscv_parse_prefixed_ext
function now parses all prefix types in a single call and no longer
requires a loop (in fact, this function is called only once and "while"
loop in riscv_parse_subset function does not actually "loop").
I added trailing underscore-checking code (in PATCH 2/3) at the end of
riscv_parse_prefixed_ext function. That means, my PATCH 2/3 assumes
that riscv_parse_prefixed_ext is called only once.
PATCH 1/3 ensures that riscv_parse_prefixed_ext is called only once and
that improves the clarity, even without other patches.
Invalid Double Underscores (Rejected in PATCH 2/3):
multi-multi extensions (e.g. Zicsr and Zifencei) must be separated by
AN underscore. Quoting ISA Manual (version 20191213),
27.6 "Additional Standard Extension Names":
> Extensions with the “Z” prefix must be separated from other multi-
> letter extensions by AN underscore, e.g., “RV32IMACZicsr_Zifencei”.
27.7 "Supervisor-level Instruction-Set Extensions":
> Supervisor-level extensions must be separated from other multi-letter
> extensions by AN underscore.
27.10 "Non-Standard Extension Names":
> They must be separated from other multi-letter extensions by AN
> underscore.
Although multiple underscores are not explicitly prohibited to separate
single-multi or single-single extension pairs, it's not natural to
assume that multiple underscores are allowed here as "AN underscore" is
required to avoid “P” extension ambiguity.
27.5 "Underscores":
> Because the “P” extension for Packed SIMD can be confused for the
> decimal point in a version number, it must be preceded by AN
> underscore if it follows a number.
Invalid Trailing Underscore (Rejected in PATCH 2/3):
This is not allowed in any way because all underscores are intended to
be used to separate ISA extensions.
Tests (PATCH 3/3):
New tests contain invalid ISA strings.
Tsukasa OI (3):
RISC-V: Remove a loop in the ISA parser
RISC-V: Stricter underscore handling for ISA
RISC-V: Tests for ISA string handling (underscore)
bfd/elfxx-riscv.c | 36 ++++++++++++++-----
.../riscv/march-fail-underscore-double-01.d | 3 ++
.../riscv/march-fail-underscore-double-02.d | 3 ++
.../riscv/march-fail-underscore-double-03.d | 3 ++
.../riscv/march-fail-underscore-double-04.d | 3 ++
.../riscv/march-fail-underscore-double-05.d | 3 ++
.../riscv/march-fail-underscore-double-06.d | 3 ++
.../riscv/march-fail-underscore-double-07.d | 3 ++
.../riscv/march-fail-underscore-double-08.d | 3 ++
.../gas/riscv/march-fail-underscore-double.l | 2 ++
.../riscv/march-fail-underscore-trailing-01.d | 3 ++
.../riscv/march-fail-underscore-trailing-02.d | 3 ++
.../riscv/march-fail-underscore-trailing-03.d | 3 ++
.../riscv/march-fail-underscore-trailing-04.d | 3 ++
.../riscv/march-fail-underscore-trailing.l | 2 ++
15 files changed, 68 insertions(+), 8 deletions(-)
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-01.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-02.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-03.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-04.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-05.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-06.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-07.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-08.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double.l
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-01.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-02.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-03.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-04.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing.l
base-commit: 3a3e333f65483b864bf2624392f8aa4a88c7a498
--
2.32.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] RISC-V: Remove a loop in the ISA parser
2022-02-23 1:47 [PATCH 0/3] RISC-V: Make ISA parser stricter (with code clarity improvement) Tsukasa OI
@ 2022-02-23 1:47 ` Tsukasa OI
2022-02-25 9:10 ` Nelson Chu
2022-02-23 1:47 ` [PATCH 2/3] RISC-V: Stricter underscore handling for ISA Tsukasa OI
2022-02-23 1:47 ` [PATCH 3/3] RISC-V: Tests for ISA string handling (underscore) Tsukasa OI
2 siblings, 1 reply; 8+ messages in thread
From: Tsukasa OI @ 2022-02-23 1:47 UTC (permalink / raw)
To: Tsukasa OI; +Cc: binutils
Since commit e601909a3287bf541c6a7d82214bb387d2c76d82 ("RISC-V: Support
to parse the multi-letter prefix in the architecture string.") changed
so that all prefixed extensions are parsed in single
riscv_parse_prefixed_ext call, a "while" loop on riscv_parse_subset
is no longer required.
bfd/ChangeLog:
* elfxx-riscv.c (riscv_parse_subset): Remove unnecessary loop.
---
bfd/elfxx-riscv.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
index 27d06d2fa3d..329dcaed304 100644
--- a/bfd/elfxx-riscv.c
+++ b/bfd/elfxx-riscv.c
@@ -2019,14 +2019,11 @@ riscv_parse_subset (riscv_parse_subset_t *rps,
if (p == NULL)
return false;
- /* Parse the different classes of extensions in the specified order. */
- while (*p != '\0')
- {
- p = riscv_parse_prefixed_ext (rps, arch, p);
+ /* Parse prefixed extensions. */
+ p = riscv_parse_prefixed_ext (rps, arch, p);
- if (p == NULL)
- return false;
- }
+ if (p == NULL)
+ return false;
/* Finally add implicit extensions according to the current
extensions. */
--
2.32.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] RISC-V: Stricter underscore handling for ISA
2022-02-23 1:47 [PATCH 0/3] RISC-V: Make ISA parser stricter (with code clarity improvement) Tsukasa OI
2022-02-23 1:47 ` [PATCH 1/3] RISC-V: Remove a loop in the ISA parser Tsukasa OI
@ 2022-02-23 1:47 ` Tsukasa OI
2022-02-25 9:22 ` Nelson Chu
2022-02-23 1:47 ` [PATCH 3/3] RISC-V: Tests for ISA string handling (underscore) Tsukasa OI
2 siblings, 1 reply; 8+ messages in thread
From: Tsukasa OI @ 2022-02-23 1:47 UTC (permalink / raw)
To: Tsukasa OI; +Cc: binutils
Double underscores and trailing underscore in an ISA string are invalid.
bfd/ChangeLog:
* elfxx-riscv.c (riscv_parse_std_ext): Make handling for
underscores in ISA string more strict.
(riscv_parse_prefixed_ext): Likewise.
---
bfd/elfxx-riscv.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
index 329dcaed304..ee4b442ba89 100644
--- a/bfd/elfxx-riscv.c
+++ b/bfd/elfxx-riscv.c
@@ -1650,6 +1650,8 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
const char *arch,
const char *p)
{
+ unsigned char underscores = 0;
+
/* First letter must start with i, e or g. */
if (*p != 'e' && *p != 'i' && *p != 'g')
{
@@ -1669,8 +1671,15 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
if (*p == '_')
{
p++;
+ if (++underscores == 2)
+ {
+ rps->error_handler
+ (_("%s: double underscores"), arch);
+ return NULL;
+ }
continue;
}
+ underscores = 0;
bool implicit = false;
int major = RISCV_UNKNOWN_VERSION;
@@ -1709,7 +1718,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
riscv_parse_add_subset (rps, subset, major, minor, implicit);
}
- return p;
+ return p - underscores;
}
/* Parsing function for prefixed extensions.
@@ -1731,14 +1740,22 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
int minor_version;
const char *last_name;
enum riscv_prefix_ext_class class;
+ unsigned char underscores = 0;
while (*p)
{
if (*p == '_')
{
p++;
+ if (++underscores == 2)
+ {
+ rps->error_handler
+ (_("%s: double underscores"), arch);
+ return NULL;
+ }
continue;
}
+ underscores = 0;
class = riscv_get_prefix_class (p);
if (class == RV_ISA_CLASS_UNKNOWN)
@@ -1843,6 +1860,12 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
}
}
+ if (underscores)
+ {
+ rps->error_handler
+ (_("%s: trailing underscore"), arch);
+ return NULL;
+ }
return p;
}
--
2.32.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] RISC-V: Tests for ISA string handling (underscore)
2022-02-23 1:47 [PATCH 0/3] RISC-V: Make ISA parser stricter (with code clarity improvement) Tsukasa OI
2022-02-23 1:47 ` [PATCH 1/3] RISC-V: Remove a loop in the ISA parser Tsukasa OI
2022-02-23 1:47 ` [PATCH 2/3] RISC-V: Stricter underscore handling for ISA Tsukasa OI
@ 2022-02-23 1:47 ` Tsukasa OI
2 siblings, 0 replies; 8+ messages in thread
From: Tsukasa OI @ 2022-02-23 1:47 UTC (permalink / raw)
To: Tsukasa OI; +Cc: binutils
This commit adds invalid ISA string tests containing double underscores
or trailing underscore.
gas/ChangeLog:
* testsuite/gas/riscv/march-fail-underscore-double-01.d: New
test for invalid ISA string with double underscores.
* testsuite/gas/riscv/march-fail-underscore-double-02.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-double-03.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-double-04.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-double-05.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-double-06.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-double-07.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-double-08.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-double.l: Likewise.
* testsuite/gas/riscv/march-fail-underscore-trailing-01.d: New
test for invalid ISA string with trailing underscore.
* testsuite/gas/riscv/march-fail-underscore-trailing-02.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-trailing-03.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-trailing-04.d: Likewise.
* testsuite/gas/riscv/march-fail-underscore-trailing.l: Likewise.
---
gas/testsuite/gas/riscv/march-fail-underscore-double-01.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double-02.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double-03.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double-04.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double-05.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double-06.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double-07.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double-08.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-double.l | 2 ++
gas/testsuite/gas/riscv/march-fail-underscore-trailing-01.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-trailing-02.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-trailing-03.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-trailing-04.d | 3 +++
gas/testsuite/gas/riscv/march-fail-underscore-trailing.l | 2 ++
14 files changed, 40 insertions(+)
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-01.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-02.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-03.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-04.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-05.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-06.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-07.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double-08.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-double.l
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-01.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-02.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-03.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing-04.d
create mode 100644 gas/testsuite/gas/riscv/march-fail-underscore-trailing.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-01.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-01.d
new file mode 100644
index 00000000000..bc63926d405
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-01.d
@@ -0,0 +1,3 @@
+#as: -march=rv32i__m
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-02.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-02.d
new file mode 100644
index 00000000000..c1fab59ac8f
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-02.d
@@ -0,0 +1,3 @@
+#as: -march=rv32im__zicsr
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-03.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-03.d
new file mode 100644
index 00000000000..903d5667192
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-03.d
@@ -0,0 +1,3 @@
+#as: -march=rv32im2__zicsr
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-04.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-04.d
new file mode 100644
index 00000000000..4e77d313cf3
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-04.d
@@ -0,0 +1,3 @@
+#as: -march=rv32im2p0__zicsr
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-05.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-05.d
new file mode 100644
index 00000000000..b85750e68fa
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-05.d
@@ -0,0 +1,3 @@
+#as: -march=rv32ima_zicsr__zifencei
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-06.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-06.d
new file mode 100644
index 00000000000..be532aa34b8
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-06.d
@@ -0,0 +1,3 @@
+#as: -march=rv32ima_zicsr2__zifencei
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-07.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-07.d
new file mode 100644
index 00000000000..6f7223a7eef
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-07.d
@@ -0,0 +1,3 @@
+#as: -march=rv32ima_zicsr2p0__zifencei
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double-08.d b/gas/testsuite/gas/riscv/march-fail-underscore-double-08.d
new file mode 100644
index 00000000000..75e3f07be52
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double-08.d
@@ -0,0 +1,3 @@
+#as: -march=rv32imazicsr__zifencei
+#source: empty.s
+#error_output: march-fail-underscore-double.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-double.l b/gas/testsuite/gas/riscv/march-fail-underscore-double.l
new file mode 100644
index 00000000000..aa20f34f4a6
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-double.l
@@ -0,0 +1,2 @@
+.*Assembler messages:
+.*Error: .*: double underscores
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-trailing-01.d b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-01.d
new file mode 100644
index 00000000000..d5b5b0a65d9
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-01.d
@@ -0,0 +1,3 @@
+#as: -march=rv32ima_
+#source: empty.s
+#error_output: march-fail-underscore-trailing.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-trailing-02.d b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-02.d
new file mode 100644
index 00000000000..a0bd12b8335
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-02.d
@@ -0,0 +1,3 @@
+#as: -march=rv32ima_zicsr_
+#source: empty.s
+#error_output: march-fail-underscore-trailing.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-trailing-03.d b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-03.d
new file mode 100644
index 00000000000..7c0f99e4b04
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-03.d
@@ -0,0 +1,3 @@
+#as: -march=rv32ima_zicsr2_
+#source: empty.s
+#error_output: march-fail-underscore-trailing.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-trailing-04.d b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-04.d
new file mode 100644
index 00000000000..c5a1242e9dc
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-trailing-04.d
@@ -0,0 +1,3 @@
+#as: -march=rv32ima_zicsr2p0_
+#source: empty.s
+#error_output: march-fail-underscore-trailing.l
diff --git a/gas/testsuite/gas/riscv/march-fail-underscore-trailing.l b/gas/testsuite/gas/riscv/march-fail-underscore-trailing.l
new file mode 100644
index 00000000000..7f9ea0e3c88
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-underscore-trailing.l
@@ -0,0 +1,2 @@
+.*Assembler messages:
+.*Error: .*: trailing underscore
--
2.32.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] RISC-V: Remove a loop in the ISA parser
2022-02-23 1:47 ` [PATCH 1/3] RISC-V: Remove a loop in the ISA parser Tsukasa OI
@ 2022-02-25 9:10 ` Nelson Chu
0 siblings, 0 replies; 8+ messages in thread
From: Nelson Chu @ 2022-02-25 9:10 UTC (permalink / raw)
To: Tsukasa OI; +Cc: Binutils
On Wed, Feb 23, 2022 at 9:48 AM Tsukasa OI via Binutils
<binutils@sourceware.org> wrote:
>
> Since commit e601909a3287bf541c6a7d82214bb387d2c76d82 ("RISC-V: Support
> to parse the multi-letter prefix in the architecture string.") changed
> so that all prefixed extensions are parsed in single
> riscv_parse_prefixed_ext call, a "while" loop on riscv_parse_subset
> is no longer required.
Make sense! LGTM, so committed.
Thanks
Nelson
> bfd/ChangeLog:
>
> * elfxx-riscv.c (riscv_parse_subset): Remove unnecessary loop.
> ---
> bfd/elfxx-riscv.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 27d06d2fa3d..329dcaed304 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -2019,14 +2019,11 @@ riscv_parse_subset (riscv_parse_subset_t *rps,
> if (p == NULL)
> return false;
>
> - /* Parse the different classes of extensions in the specified order. */
> - while (*p != '\0')
> - {
> - p = riscv_parse_prefixed_ext (rps, arch, p);
> + /* Parse prefixed extensions. */
> + p = riscv_parse_prefixed_ext (rps, arch, p);
>
> - if (p == NULL)
> - return false;
> - }
> + if (p == NULL)
> + return false;
>
> /* Finally add implicit extensions according to the current
> extensions. */
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] RISC-V: Stricter underscore handling for ISA
2022-02-23 1:47 ` [PATCH 2/3] RISC-V: Stricter underscore handling for ISA Tsukasa OI
@ 2022-02-25 9:22 ` Nelson Chu
2022-02-25 9:37 ` Kito Cheng
0 siblings, 1 reply; 8+ messages in thread
From: Nelson Chu @ 2022-02-25 9:22 UTC (permalink / raw)
To: Tsukasa OI; +Cc: Binutils
On Wed, Feb 23, 2022 at 9:49 AM Tsukasa OI via Binutils
<binutils@sourceware.org> wrote:
>
> Double underscores and trailing underscore in an ISA string are invalid.
The ISA spec does say that "Extensions with the Z prefix must be
separated from other multi-letter extensions by an underscore". But I
am not sure if we need the stricter underscore checking for now. The
discussion [1] said that we can set the ISA string in any order, so I
think the checks will be more and more user friendly. I know that
these are two different questions, and I don't have a strong opinion.
Maybe see if anyone else has any suggestions.
Thanks
Nelson
[1] https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/14
> bfd/ChangeLog:
>
> * elfxx-riscv.c (riscv_parse_std_ext): Make handling for
> underscores in ISA string more strict.
> (riscv_parse_prefixed_ext): Likewise.
> ---
> bfd/elfxx-riscv.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 329dcaed304..ee4b442ba89 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -1650,6 +1650,8 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> const char *arch,
> const char *p)
> {
> + unsigned char underscores = 0;
> +
> /* First letter must start with i, e or g. */
> if (*p != 'e' && *p != 'i' && *p != 'g')
> {
> @@ -1669,8 +1671,15 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> if (*p == '_')
> {
> p++;
> + if (++underscores == 2)
> + {
> + rps->error_handler
> + (_("%s: double underscores"), arch);
> + return NULL;
> + }
> continue;
> }
> + underscores = 0;
>
> bool implicit = false;
> int major = RISCV_UNKNOWN_VERSION;
> @@ -1709,7 +1718,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> riscv_parse_add_subset (rps, subset, major, minor, implicit);
> }
>
> - return p;
> + return p - underscores;
> }
>
> /* Parsing function for prefixed extensions.
> @@ -1731,14 +1740,22 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> int minor_version;
> const char *last_name;
> enum riscv_prefix_ext_class class;
> + unsigned char underscores = 0;
>
> while (*p)
> {
> if (*p == '_')
> {
> p++;
> + if (++underscores == 2)
> + {
> + rps->error_handler
> + (_("%s: double underscores"), arch);
> + return NULL;
> + }
> continue;
> }
> + underscores = 0;
>
> class = riscv_get_prefix_class (p);
> if (class == RV_ISA_CLASS_UNKNOWN)
> @@ -1843,6 +1860,12 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> }
> }
>
> + if (underscores)
> + {
> + rps->error_handler
> + (_("%s: trailing underscore"), arch);
> + return NULL;
> + }
> return p;
> }
>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] RISC-V: Stricter underscore handling for ISA
2022-02-25 9:22 ` Nelson Chu
@ 2022-02-25 9:37 ` Kito Cheng
2022-02-25 11:26 ` Andrew Waterman
0 siblings, 1 reply; 8+ messages in thread
From: Kito Cheng @ 2022-02-25 9:37 UTC (permalink / raw)
To: Nelson Chu, Andrew Waterman; +Cc: Tsukasa OI, Binutils
Just for reference:
For RISC-V GCC, you can use any number of underscores as you can, so
-march=rv64i___m____a_____c_ is valid input for GCC.
For RISC-V LLVM, you can use only one underscore and have trailing
underscore, e.g. -march=rv64i_mac_
On Fri, Feb 25, 2022 at 5:23 PM Nelson Chu <nelson.chu@sifive.com> wrote:
>
> On Wed, Feb 23, 2022 at 9:49 AM Tsukasa OI via Binutils
> <binutils@sourceware.org> wrote:
> >
> > Double underscores and trailing underscore in an ISA string are invalid.
>
> The ISA spec does say that "Extensions with the Z prefix must be
> separated from other multi-letter extensions by an underscore". But I
> am not sure if we need the stricter underscore checking for now. The
> discussion [1] said that we can set the ISA string in any order, so I
> think the checks will be more and more user friendly. I know that
> these are two different questions, and I don't have a strong opinion.
> Maybe see if anyone else has any suggestions.
>
> Thanks
> Nelson
>
> [1] https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/14
>
> > bfd/ChangeLog:
> >
> > * elfxx-riscv.c (riscv_parse_std_ext): Make handling for
> > underscores in ISA string more strict.
> > (riscv_parse_prefixed_ext): Likewise.
> > ---
> > bfd/elfxx-riscv.c | 25 ++++++++++++++++++++++++-
> > 1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> > index 329dcaed304..ee4b442ba89 100644
> > --- a/bfd/elfxx-riscv.c
> > +++ b/bfd/elfxx-riscv.c
> > @@ -1650,6 +1650,8 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> > const char *arch,
> > const char *p)
> > {
> > + unsigned char underscores = 0;
> > +
> > /* First letter must start with i, e or g. */
> > if (*p != 'e' && *p != 'i' && *p != 'g')
> > {
> > @@ -1669,8 +1671,15 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> > if (*p == '_')
> > {
> > p++;
> > + if (++underscores == 2)
> > + {
> > + rps->error_handler
> > + (_("%s: double underscores"), arch);
> > + return NULL;
> > + }
> > continue;
> > }
> > + underscores = 0;
> >
> > bool implicit = false;
> > int major = RISCV_UNKNOWN_VERSION;
> > @@ -1709,7 +1718,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> > riscv_parse_add_subset (rps, subset, major, minor, implicit);
> > }
> >
> > - return p;
> > + return p - underscores;
> > }
> >
> > /* Parsing function for prefixed extensions.
> > @@ -1731,14 +1740,22 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> > int minor_version;
> > const char *last_name;
> > enum riscv_prefix_ext_class class;
> > + unsigned char underscores = 0;
> >
> > while (*p)
> > {
> > if (*p == '_')
> > {
> > p++;
> > + if (++underscores == 2)
> > + {
> > + rps->error_handler
> > + (_("%s: double underscores"), arch);
> > + return NULL;
> > + }
> > continue;
> > }
> > + underscores = 0;
> >
> > class = riscv_get_prefix_class (p);
> > if (class == RV_ISA_CLASS_UNKNOWN)
> > @@ -1843,6 +1860,12 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> > }
> > }
> >
> > + if (underscores)
> > + {
> > + rps->error_handler
> > + (_("%s: trailing underscore"), arch);
> > + return NULL;
> > + }
> > return p;
> > }
> >
> > --
> > 2.32.0
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] RISC-V: Stricter underscore handling for ISA
2022-02-25 9:37 ` Kito Cheng
@ 2022-02-25 11:26 ` Andrew Waterman
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Waterman @ 2022-02-25 11:26 UTC (permalink / raw)
To: Kito Cheng; +Cc: Nelson Chu, Tsukasa OI, Binutils
Although the spec isn't written as a formal grammar, my interpretation
of the text is that excess underscores are acceptable. The basis for
my claim is that "underscores [...] may be used [...] to improve
readability", implying they can sometimes be inserted where they
aren't required.
Sounds like GCC agrees with my thinking.
I don't have an opinion on the trailing underscore.
On Fri, Feb 25, 2022 at 1:37 AM Kito Cheng <kito.cheng@gmail.com> wrote:
>
> Just for reference:
>
> For RISC-V GCC, you can use any number of underscores as you can, so
> -march=rv64i___m____a_____c_ is valid input for GCC.
> For RISC-V LLVM, you can use only one underscore and have trailing
> underscore, e.g. -march=rv64i_mac_
>
> On Fri, Feb 25, 2022 at 5:23 PM Nelson Chu <nelson.chu@sifive.com> wrote:
> >
> > On Wed, Feb 23, 2022 at 9:49 AM Tsukasa OI via Binutils
> > <binutils@sourceware.org> wrote:
> > >
> > > Double underscores and trailing underscore in an ISA string are invalid.
> >
> > The ISA spec does say that "Extensions with the Z prefix must be
> > separated from other multi-letter extensions by an underscore". But I
> > am not sure if we need the stricter underscore checking for now. The
> > discussion [1] said that we can set the ISA string in any order, so I
> > think the checks will be more and more user friendly. I know that
> > these are two different questions, and I don't have a strong opinion.
> > Maybe see if anyone else has any suggestions.
> >
> > Thanks
> > Nelson
> >
> > [1] https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/14
> >
> > > bfd/ChangeLog:
> > >
> > > * elfxx-riscv.c (riscv_parse_std_ext): Make handling for
> > > underscores in ISA string more strict.
> > > (riscv_parse_prefixed_ext): Likewise.
> > > ---
> > > bfd/elfxx-riscv.c | 25 ++++++++++++++++++++++++-
> > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> > > index 329dcaed304..ee4b442ba89 100644
> > > --- a/bfd/elfxx-riscv.c
> > > +++ b/bfd/elfxx-riscv.c
> > > @@ -1650,6 +1650,8 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> > > const char *arch,
> > > const char *p)
> > > {
> > > + unsigned char underscores = 0;
> > > +
> > > /* First letter must start with i, e or g. */
> > > if (*p != 'e' && *p != 'i' && *p != 'g')
> > > {
> > > @@ -1669,8 +1671,15 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> > > if (*p == '_')
> > > {
> > > p++;
> > > + if (++underscores == 2)
> > > + {
> > > + rps->error_handler
> > > + (_("%s: double underscores"), arch);
> > > + return NULL;
> > > + }
> > > continue;
> > > }
> > > + underscores = 0;
> > >
> > > bool implicit = false;
> > > int major = RISCV_UNKNOWN_VERSION;
> > > @@ -1709,7 +1718,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
> > > riscv_parse_add_subset (rps, subset, major, minor, implicit);
> > > }
> > >
> > > - return p;
> > > + return p - underscores;
> > > }
> > >
> > > /* Parsing function for prefixed extensions.
> > > @@ -1731,14 +1740,22 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> > > int minor_version;
> > > const char *last_name;
> > > enum riscv_prefix_ext_class class;
> > > + unsigned char underscores = 0;
> > >
> > > while (*p)
> > > {
> > > if (*p == '_')
> > > {
> > > p++;
> > > + if (++underscores == 2)
> > > + {
> > > + rps->error_handler
> > > + (_("%s: double underscores"), arch);
> > > + return NULL;
> > > + }
> > > continue;
> > > }
> > > + underscores = 0;
> > >
> > > class = riscv_get_prefix_class (p);
> > > if (class == RV_ISA_CLASS_UNKNOWN)
> > > @@ -1843,6 +1860,12 @@ riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> > > }
> > > }
> > >
> > > + if (underscores)
> > > + {
> > > + rps->error_handler
> > > + (_("%s: trailing underscore"), arch);
> > > + return NULL;
> > > + }
> > > return p;
> > > }
> > >
> > > --
> > > 2.32.0
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-25 11:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-23 1:47 [PATCH 0/3] RISC-V: Make ISA parser stricter (with code clarity improvement) Tsukasa OI
2022-02-23 1:47 ` [PATCH 1/3] RISC-V: Remove a loop in the ISA parser Tsukasa OI
2022-02-25 9:10 ` Nelson Chu
2022-02-23 1:47 ` [PATCH 2/3] RISC-V: Stricter underscore handling for ISA Tsukasa OI
2022-02-25 9:22 ` Nelson Chu
2022-02-25 9:37 ` Kito Cheng
2022-02-25 11:26 ` Andrew Waterman
2022-02-23 1:47 ` [PATCH 3/3] RISC-V: Tests for ISA string handling (underscore) Tsukasa OI
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 read-only IMAP folder(s) and NNTP newsgroup(s).