diff mbox series

[1/1] RISC-V: Correct as --dump-config output for --target=rv64*

Message ID 20250106011532.38424-2-zengxiao@eswincomputing.com
State New
Headers
Series RISC-V: Correct as --dump-config output for --target=rv64* |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Xiao Zeng Jan. 6, 2025, 1:15 a.m. UTC 
  gas/ChangeLog:

	* config/tc-riscv.c (riscv_target_format): Correct as
	--dump-config output for --target=rv64*.

Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com>
---
 gas/config/tc-riscv.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Nelson Chu Jan. 6, 2025, 3:01 a.m. UTC | #1 
Hi Xiao,

Thanks, could we have a testcase for this change :-)

Nelson

On Mon, Jan 6, 2025 at 9:15 AM Xiao Zeng <zengxiao@eswincomputing.com>
wrote:

> gas/ChangeLog:
>
>         * config/tc-riscv.c (riscv_target_format): Correct as
>         --dump-config output for --target=rv64*.
>
> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com>
> ---
>  gas/config/tc-riscv.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index a915c8b4995..35d19ac4e5d 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -734,6 +734,18 @@ riscv_check_mapping_symbols (bfd *abfd
> ATTRIBUTE_UNUSED,
>  const char *
>  riscv_target_format (void)
>  {
> +  /* When as --dump-config is used, xlen is not initialized, resulting in
> +     bfd-target being set to elf32-*riscv even with --target=rv64*.  */
> +  if (xlen == 0)
> +    {
> +      if (strcmp (default_arch, "riscv32") == 0)
> +       xlen = 32;
> +      else if (strcmp (default_arch, "riscv64") == 0)
> +       xlen = 64;
> +      else
> +       as_bad ("unknown default architecture `%s'", default_arch);
> +    }
> +
>    if (target_big_endian)
>      return xlen == 64 ? "elf64-bigriscv" : "elf32-bigriscv";
>    else
> --
> 2.17.1
>
>
Xiao Zeng Jan. 6, 2025, 8:29 a.m. UTC | #2 
2025-01-06 11:01  Nelson Chu <nelson@rivosinc.com> wrote:
>
>Hi Xiao,
>
>Thanks, could we have a testcase for this change :-)
This should have been my responsibility.

>
>Nelson
>
>On Mon, Jan 6, 2025 at 9:15 AM Xiao Zeng <zengxiao@eswincomputing.com>
>wrote:
>
>> gas/ChangeLog:
>>
>>         * config/tc-riscv.c (riscv_target_format): Correct as
>>         --dump-config output for --target=rv64*.
>>
>> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com>
>> ---
>>  gas/config/tc-riscv.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index a915c8b4995..35d19ac4e5d 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -734,6 +734,18 @@ riscv_check_mapping_symbols (bfd *abfd
>> ATTRIBUTE_UNUSED,
>>  const char *
>>  riscv_target_format (void)
>>  {
>> +  /* When as --dump-config is used, xlen is not initialized, resulting in
>> +     bfd-target being set to elf32-*riscv even with --target=rv64*.  */
>> +  if (xlen == 0)
>> +    {
>> +      if (strcmp (default_arch, "riscv32") == 0)
>> +       xlen = 32;
>> +      else if (strcmp (default_arch, "riscv64") == 0)
>> +       xlen = 64;
>> +      else
>> +       as_bad ("unknown default architecture `%s'", default_arch);
>> +    }
>> +
>>    if (target_big_endian)
>>      return xlen == 64 ? "elf64-bigriscv" : "elf32-bigriscv";
>>    else
>> --
>> 2.17.1
>>
>> 
Hi Nelson,  

I've roughly gone through the test cases in gas/testsuite/gas/riscv/*
and found that they cannot achieve the desired testing.
I have outlined the expected test case in: <https://sourceware.org/pipermail/binutils/2025-January/138366.html>.  

Could you please give me some guidance? Thank you!  

Thanks
Xiao Zeng
Jan Beulich Jan. 6, 2025, 9:29 a.m. UTC | #3 
On 06.01.2025 02:15, Xiao Zeng wrote:
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -734,6 +734,18 @@ riscv_check_mapping_symbols (bfd *abfd ATTRIBUTE_UNUSED,
>  const char *
>  riscv_target_format (void)
>  {
> +  /* When as --dump-config is used, xlen is not initialized, resulting in
> +     bfd-target being set to elf32-*riscv even with --target=rv64*.  */
> +  if (xlen == 0)
> +    {
> +      if (strcmp (default_arch, "riscv32") == 0)
> +	xlen = 32;
> +      else if (strcmp (default_arch, "riscv64") == 0)
> +	xlen = 64;
> +      else
> +	as_bad ("unknown default architecture `%s'", default_arch);

My take is that as_bad() is inappropriate here. The function is used to
signal user mistakes. Whereas here we'd really deal with a bug in the
implementation. I expect you want to use e.g. as_abort() instead.

Jan
Xiao Zeng Jan. 7, 2025, 1:09 a.m. UTC | #4 
2025-01-06 17:29  Jan Beulich <jbeulich@suse.com> wrote:
>
>On 06.01.2025 02:15, Xiao Zeng wrote:
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -734,6 +734,18 @@ riscv_check_mapping_symbols (bfd *abfd ATTRIBUTE_UNUSED,
>>  const char *
>>  riscv_target_format (void)
>>  {
>> +  /* When as --dump-config is used, xlen is not initialized, resulting in
>> +     bfd-target being set to elf32-*riscv even with --target=rv64*.  */
>> +  if (xlen == 0)
>> +    {
>> +      if (strcmp (default_arch, "riscv32") == 0)
>> +	xlen = 32;
>> +      else if (strcmp (default_arch, "riscv64") == 0)
>> +	xlen = 64;
>> +      else
>> +	as_bad ("unknown default architecture `%s'", default_arch);
>
>My take is that as_bad() is inappropriate here. The function is used to
>signal user mistakes. Whereas here we'd really deal with a bug in the
>implementation. I expect you want to use e.g. as_abort() instead. 
Yes, I will fix this issue in v2, thank you~

>
>Jan
Thanks
Xiao Zeng
diff mbox series

Patch

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index a915c8b4995..35d19ac4e5d 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -734,6 +734,18 @@  riscv_check_mapping_symbols (bfd *abfd ATTRIBUTE_UNUSED,
 const char *
 riscv_target_format (void)
 {
+  /* When as --dump-config is used, xlen is not initialized, resulting in
+     bfd-target being set to elf32-*riscv even with --target=rv64*.  */
+  if (xlen == 0)
+    {
+      if (strcmp (default_arch, "riscv32") == 0)
+	xlen = 32;
+      else if (strcmp (default_arch, "riscv64") == 0)
+	xlen = 64;
+      else
+	as_bad ("unknown default architecture `%s'", default_arch);
+    }
+
   if (target_big_endian)
     return xlen == 64 ? "elf64-bigriscv" : "elf32-bigriscv";
   else