diff mbox series

RISC-V: Add string length check for operands in AS

Message ID 20221213043428.33155-1-xuli1@eswincomputing.com
State Accepted
Headers
Series RISC-V: Add string length check for operands in AS |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Li Xu Dec. 13, 2022, 4:34 a.m. UTC 
  The current AS accepts invaild operands due to miss of operands length check.
For example, "e6" is an invalid operand in (vsetvli a0, a1, e6, mf8, tu, ma),
but it's still accepted by assembler. In detail, the condition check "strncmp
(array[i], *s, len) == 0" in arg_lookup function passes with "strncmp ("e64",
"e6", 2)" in the case above. So the generated encoding is same as that of
(vsetvli a0, a1, e64, mf8, tu, ma).
This patch fixes issue above by prompting an error in such case and also adds
a new testcase.

gas/ChangeLog:

        * config/tc-riscv.c (arg_lookup): Add string length check for operands.
        * testsuite/gas/riscv/vector-insns-fail-vsew.d: New testcase for an illegal vsew.
        * testsuite/gas/riscv/vector-insns-fail-vsew.l: Likewise.
        * testsuite/gas/riscv/vector-insns-fail-vsew.s: Likewise.
---
 gas/config/tc-riscv.c                            | 2 +-
 gas/testsuite/gas/riscv/vector-insns-fail-vsew.d | 3 +++
 gas/testsuite/gas/riscv/vector-insns-fail-vsew.l | 3 +++
 gas/testsuite/gas/riscv/vector-insns-fail-vsew.s | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)
 create mode 100644 gas/testsuite/gas/riscv/vector-insns-fail-vsew.d
 create mode 100644 gas/testsuite/gas/riscv/vector-insns-fail-vsew.l
 create mode 100644 gas/testsuite/gas/riscv/vector-insns-fail-vsew.s

Comments

Jan Beulich Dec. 13, 2022, 7:47 a.m. UTC | #1 
On 13.12.2022 05:34, Li Xu wrote:
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1206,7 +1206,7 @@ arg_lookup (char **s, const char *const *array, size_t size, unsigned *regnop)
>      return false;
>  
>    for (i = 0; i < size; i++)
> -    if (array[i] != NULL && strncmp (array[i], *s, len) == 0)
> +    if (array[i] != NULL && (strlen(array[i]) == len) && strncmp (array[i], *s, len) == 0)

A couple of remarks: First of all this looks like another case where
startswith() might better be used. And then style aspects: There's
a blank missing after "strlen", and you will want to be consistent
with the use of parentheses: Both pre-existing comparisons aren't
wrapped in any, so any new one shouldn't be either (or, less desirably
imo, all three should be). Plus finally the resulting line is overly
long.

Jan
Andreas Schwab Dec. 13, 2022, 9:07 a.m. UTC | #2 
On Dez 13 2022, Li Xu wrote:

> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 0682eb35524..b0989e4b124 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1206,7 +1206,7 @@ arg_lookup (char **s, const char *const *array, size_t size, unsigned *regnop)
>      return false;
>  
>    for (i = 0; i < size; i++)
> -    if (array[i] != NULL && strncmp (array[i], *s, len) == 0)
> +    if (array[i] != NULL && (strlen(array[i]) == len) && strncmp (array[i], *s, len) == 0)

There is no need to read through array[i] twice.

    if (array[i] != NULL && strncmp (array[i], *s, len) == 0
        && array[i][len] == '\0')
Li Xu Dec. 14, 2022, 8:06 a.m. UTC | #3 
2022-12-13 17:07 Andreas Schwab <schwab@suse.de> wrote:



>



>On Dez 13 2022, Li Xu wrote:



>



>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c



>> index 0682eb35524..b0989e4b124 100644



>> --- a/gas/config/tc-riscv.c



>> +++ b/gas/config/tc-riscv.c



>> @@ -1206,7 +1206,7 @@ arg_lookup (char **s, const char *const *array, size_t size, unsigned *regnop)



>>      return false;



>>  



>>    for (i = 0; i < size; i++)



>> -    if (array[i] != NULL && strncmp (array[i], *s, len) == 0)



>> +    if (array[i] != NULL && (strlen(array[i]) == len) && strncmp (array[i], *s, len) == 0)



>



>There is no need to read through array[i] twice.



>



>    if (array[i] != NULL && strncmp (array[i], *s, len) == 0



>        && array[i][len] == '\0')



>



>-- 



>Andreas Schwab, SUSE Labs, schwab@suse.de



>GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7



>"And now for something completely different."


Thanks to Jan and Andreas review comments.
I tried to use startswith(), but the first  argument is not easy to pass.  So i choose to
follow Andreas's advice.
The new patch is https://sourceware.org/pipermail/binutils/2022-December/125094.html, thanks.
diff mbox series

Patch

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 0682eb35524..b0989e4b124 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1206,7 +1206,7 @@  arg_lookup (char **s, const char *const *array, size_t size, unsigned *regnop)
     return false;
 
   for (i = 0; i < size; i++)
-    if (array[i] != NULL && strncmp (array[i], *s, len) == 0)
+    if (array[i] != NULL && (strlen(array[i]) == len) && strncmp (array[i], *s, len) == 0)
       {
 	*regnop = i;
 	*s += len;
diff --git a/gas/testsuite/gas/riscv/vector-insns-fail-vsew.d b/gas/testsuite/gas/riscv/vector-insns-fail-vsew.d
new file mode 100644
index 00000000000..c0c81579741
--- /dev/null
+++ b/gas/testsuite/gas/riscv/vector-insns-fail-vsew.d
@@ -0,0 +1,3 @@ 
+#as: -march=rv32ifv
+#source: vector-insns-fail-vsew.s
+#error_output: vector-insns-fail-vsew.l
diff --git a/gas/testsuite/gas/riscv/vector-insns-fail-vsew.l b/gas/testsuite/gas/riscv/vector-insns-fail-vsew.l
new file mode 100644
index 00000000000..87a2c22a805
--- /dev/null
+++ b/gas/testsuite/gas/riscv/vector-insns-fail-vsew.l
@@ -0,0 +1,3 @@ 
+.*: Assembler messages:
+.*: Error: instruction vsetvli requires absolute expression
+.*: Error: illegal operands `vsetvli a0,a1,e6,mf8,tu,ma'
diff --git a/gas/testsuite/gas/riscv/vector-insns-fail-vsew.s b/gas/testsuite/gas/riscv/vector-insns-fail-vsew.s
new file mode 100644
index 00000000000..b8f3242406f
--- /dev/null
+++ b/gas/testsuite/gas/riscv/vector-insns-fail-vsew.s
@@ -0,0 +1 @@ 
+	vsetvli  a0, a1, e6, mf8, tu, ma		# unrecognized vsew