diff mbox series

[v2,2/2] RISC-V: Improve "bits undefined" diagnostics

Message ID c6e55781245dd3e8e9b8debd6130fc5449dfbd55.1665031170.git.research_trasio@irq.a4lg.com
State Accepted, archived
Headers
Series RISC-V: Improve "bits undefined" diagnostics |

Checks

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

Commit Message

Tsukasa OI Oct. 6, 2022, 4:40 a.m. UTC 
  This commit improves internal error message
"internal: bad RISC-V opcode (bits 0x%lx undefined): %s %s"
to display actual unused bits (excluding non-instruction bits).

gas/ChangeLog:

	* config/tc-riscv.c (validate_riscv_insn): Exclude non-
	instruction bits from displaying internal diagnostics.
	Change error message slightly.
---
 gas/config/tc-riscv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich Oct. 6, 2022, 8:26 a.m. UTC | #1 
On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1312,8 +1312,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>    if (used_bits != required_bits)
>      {
>        as_bad (_("internal: bad RISC-V opcode "
> -		"(bits 0x%lx undefined): %s %s"),
> -	      ~(unsigned long)(used_bits & required_bits),
> +		"(bits 0x%llx undefined or invalid): %s %s"),
> +	      (unsigned long long)(used_bits ^ required_bits),

May I encourage the use of the # format modifier in cases like this
one (i.e. %#llx here), for producing a one character shorter string
literal? Iirc a respective adjustment was done pretty recently to
some other parts of binutils.

Jan
Tsukasa OI Oct. 6, 2022, 8:34 a.m. UTC | #2 
On 2022/10/06 17:26, Jan Beulich wrote:
> On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -1312,8 +1312,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>>    if (used_bits != required_bits)
>>      {
>>        as_bad (_("internal: bad RISC-V opcode "
>> -		"(bits 0x%lx undefined): %s %s"),
>> -	      ~(unsigned long)(used_bits & required_bits),
>> +		"(bits 0x%llx undefined or invalid): %s %s"),
>> +	      (unsigned long long)(used_bits ^ required_bits),
> 
> May I encourage the use of the # format modifier in cases like this
> one (i.e. %#llx here), for producing a one character shorter string
> literal? Iirc a respective adjustment was done pretty recently to
> some other parts of binutils.

I would disagree if it was a part of the core disassembling portion
but... seems okay here (as exact formatting is not important).  It would
have changed the behavior if (used_bits ^ required_bits) is not zero
(e.g. with "%#x": "0" (0), "0x1" (1)...) but here, (used_bits ^
required_bits) cannot be zero.  So, the behavior won't change either.

Thanks,
Tsukasa

> 
> Jan
>
Jan Beulich Oct. 6, 2022, 8:43 a.m. UTC | #3 
On 06.10.2022 10:34, Tsukasa OI wrote:
> On 2022/10/06 17:26, Jan Beulich wrote:
>> On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
>>> --- a/gas/config/tc-riscv.c
>>> +++ b/gas/config/tc-riscv.c
>>> @@ -1312,8 +1312,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>>>    if (used_bits != required_bits)
>>>      {
>>>        as_bad (_("internal: bad RISC-V opcode "
>>> -		"(bits 0x%lx undefined): %s %s"),
>>> -	      ~(unsigned long)(used_bits & required_bits),
>>> +		"(bits 0x%llx undefined or invalid): %s %s"),
>>> +	      (unsigned long long)(used_bits ^ required_bits),
>>
>> May I encourage the use of the # format modifier in cases like this
>> one (i.e. %#llx here), for producing a one character shorter string
>> literal? Iirc a respective adjustment was done pretty recently to
>> some other parts of binutils.
> 
> I would disagree if it was a part of the core disassembling portion
> but...

Sure - typically in disassembly you want to output leading zeros, and
in that case using # isn't desirable. (I've observed RISC-V disassembly
to omit leading zeros in certain cases though, which personally I find
confusing.)

Jan

> seems okay here (as exact formatting is not important).  It would
> have changed the behavior if (used_bits ^ required_bits) is not zero
> (e.g. with "%#x": "0" (0), "0x1" (1)...) but here, (used_bits ^
> required_bits) cannot be zero.  So, the behavior won't change either.
> 
> Thanks,
> Tsukasa
diff mbox series

Patch

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 2e41cec5c9f..34973d7803c 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1312,8 +1312,8 @@  validate_riscv_insn (const struct riscv_opcode *opc, int length)
   if (used_bits != required_bits)
     {
       as_bad (_("internal: bad RISC-V opcode "
-		"(bits 0x%lx undefined): %s %s"),
-	      ~(unsigned long)(used_bits & required_bits),
+		"(bits 0x%llx undefined or invalid): %s %s"),
+	      (unsigned long long)(used_bits ^ required_bits),
 	      opc->name, opc->args);
       return false;
     }