[PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used

[PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used

[Tsukasa OI]

GAS incorrectly emits instruction mapping symbols without ISA string if
no segments use two or more architectures simultaneously.

If no segments use two or more architectures (using .option directives),
need_arch_map_symbol is not set to true, making riscv_check_mapping_symbols
function to replace segment's mapping symbols ($x+arch) with $x.

This easily occurs when a segment (section) is surrounded by .option
push/pop.  In this example, even when a non-default architecture is used by
the program, $x+arch mapping symbol emission is suppressed unless the
architecture is changed **during** a section so that two instructions in one
section use the different architecture.

This commit renames need_arch_map_symbol to arch_changed_in_seg to reflect
the actual role and suppresses $x+arch mapping symbol emission only if this
variable is false (as before) **and** the section's architecture is the
same as the default one (to be written as an ELF attribute).

gas/ChangeLog:

	* config/tc-riscv.c
	(arch_changed_in_seg): Rename from need_arch_map_symbol.
	(riscv_mapping_state): Reflect variable name change.
	(riscv_check_mapping_symbols): Suppress emission of $x+arch only
	if the default architecture is used in that segment.
	* testsuite/gas/riscv/mapping-onearch-in-seg.s: New test.
	* testsuite/gas/riscv/mapping-onearch-in-seg-dis.d: Likewise.
	* testsuite/gas/riscv/mapping-onearch-in-seg-attr.d: Likewise.
	* testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d: Likewise.
---
 gas/config/tc-riscv.c                         | 26 ++++++-----
 .../gas/riscv/mapping-onearch-in-seg-attr.d   |  6 +++
 .../gas/riscv/mapping-onearch-in-seg-dis.d    | 30 +++++++++++++
 .../riscv/mapping-onearch-in-seg-symbols.d    | 23 ++++++++++
 .../gas/riscv/mapping-onearch-in-seg.s        | 44 +++++++++++++++++++
 5 files changed, 119 insertions(+), 10 deletions(-)
 create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
 create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
 create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
 create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg.s

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 01bfc01b0fc..6d6aa631106 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -470,10 +470,12 @@ static char *expr_end;
 #define OPCODE_MATCHES(OPCODE, OP) \
   (((OPCODE) & MASK_##OP) == MATCH_##OP)
 
-/* Indicate if .option directives do affect instructions.  Set to true means
-   we need to add $x+arch at somewhere; Otherwise just add $x for instructions
-   should be enough.  */
-static bool need_arch_map_symbol = false;
+/* Indicate if .option directives changed the architecture so that
+   two or more architectures are simultaneously used in one segment.
+   Set to true means we we need to add $x+arch at somewhere; Otherwise check
+   final architecture (to be written as an ELF attribute) and decide whether
+   we need to replace $x+arch with $x.  */
+static bool arch_changed_in_seg = false;
 
 /* Create a new mapping symbol for the transition to STATE.  */
 
@@ -579,7 +581,7 @@ riscv_mapping_state (enum riscv_seg_mstate to_state,
 		      S_GET_NAME (seg_arch_symbol) + 2) != 0)
     {
       reset_seg_arch_str = true;
-      need_arch_map_symbol = true;
+      arch_changed_in_seg = true;
     }
   else if (from_state == to_state)
     return;
@@ -634,11 +636,15 @@ riscv_check_mapping_symbols (bfd *abfd ATTRIBUTE_UNUSED,
   if (seginfo == NULL || seginfo->frchainP == NULL)
     return;
 
-  /* If we don't set any .option arch directive, then the arch_map_symbol
-     in each segment must be the first instruction, and we don't need to
-     add $x+arch for them.  */
-  if (!need_arch_map_symbol
-      && seginfo->tc_segment_info_data.arch_map_symbol != 0)
+  /* If up to one architecture is used per segment, arch_map_symbol
+     (if exists) represents the architecture used in the segment.
+     Replace to $x if the architecture is the same as the final architecture
+     (to be written as an ELF attribute).  */
+  if (!arch_changed_in_seg
+      && seginfo->tc_segment_info_data.arch_map_symbol != 0
+      && strcmp (riscv_rps_as.subset_list->arch_str,
+		 S_GET_NAME (seginfo->tc_segment_info_data.arch_map_symbol) + 2)
+	 == 0)
     S_SET_NAME (seginfo->tc_segment_info_data.arch_map_symbol, "$x");
 
   for (fragp = seginfo->frchainP->frch_root;
diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
new file mode 100644
index 00000000000..7bcf54766a3
--- /dev/null
+++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
@@ -0,0 +1,6 @@
+#as: -misa-spec=20191213 -march=rv32i2p1
+#source: mapping-onearch-in-seg.s
+#readelf: -A
+Attribute Section: riscv
+File Attributes
+  Tag_RISCV_arch: "rv32i2p1_zicsr2p0"
diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
new file mode 100644
index 00000000000..c086b92535c
--- /dev/null
+++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
@@ -0,0 +1,30 @@
+#as: -misa-spec=20191213 -march=rv32i2p1
+#source: mapping-onearch-in-seg.s
+#objdump: -d
+
+.*:[ 	]+file format .*
+
+
+Disassembly of section .text.1.1:
+
+0+000 <fadd_1>:
+[ 	]+[0-9a-f]+:[ 	]+00b57553[ 	]+fadd\.s[ 	]+fa0,fa0,fa1
+[ 	]+[0-9a-f]+:[ 	]+00008067[ 	]+ret
+
+Disassembly of section .text.1.2:
+
+0+000 <fadd_2>:
+[ 	]+[0-9a-f]+:[ 	]+02b57553[ 	]+fadd\.d[ 	]+fa0,fa0,fa1
+[ 	]+[0-9a-f]+:[ 	]+00008067[ 	]+ret
+
+Disassembly of section .text.1.3:
+
+0+000 <fadd_3>:
+[ 	]+[0-9a-f]+:[ 	]+06b57553[ 	]+fadd\.q[ 	]+fa0,fa0,fa1
+[ 	]+[0-9a-f]+:[ 	]+00008067[ 	]+ret
+
+Disassembly of section .text.2:
+
+0+000 <add_1>:
+[ 	]+[0-9a-f]+:[ 	]+00b50533[ 	]+add[ 	]+a0,a0,a1
+[ 	]+[0-9a-f]+:[ 	]+00008067[ 	]+ret
diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
new file mode 100644
index 00000000000..3a476a373b7
--- /dev/null
+++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
@@ -0,0 +1,23 @@
+#as: -misa-spec=20191213 -march=rv32i2p1
+#source: mapping-onearch-in-seg.s
+#objdump: --syms --special-syms
+
+.*file format.*riscv.*
+
+SYMBOL TABLE:
+0+00 l    d  .text	0+00 .text
+0+00 l    d  .data	0+00 .data
+0+00 l    d  .bss	0+00 .bss
+0+00 l    d  .text.1.1	0+00 .text.1.1
+0+00 l       .text.1.1	0+00 fadd_1
+0+00 l       .text.1.1	0+00 \$xrv32i2p1_f2p2_zicsr2p0
+0+00 l    d  .text.1.2	0+00 .text.1.2
+0+00 l       .text.1.2	0+00 fadd_2
+0+00 l       .text.1.2	0+00 \$xrv32i2p1_f2p2_d2p2_zicsr2p0
+0+00 l    d  .text.1.3	0+00 .text.1.3
+0+00 l       .text.1.3	0+00 fadd_3
+0+00 l       .text.1.3	0+00 \$xrv32i2p1_f2p2_d2p2_q2p2_zicsr2p0
+0+00 l    d  .text.2	0+00 .text.2
+0+00 l       .text.2	0+00 add_1
+0+00 l       .text.2	0+00 \$x
+0+00 l    d  .riscv.attributes	0+00 .riscv.attributes
diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s b/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
new file mode 100644
index 00000000000..59b3f5dea98
--- /dev/null
+++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
@@ -0,0 +1,44 @@
+# In following examples, we use the same architecture per section
+# (do not change the architecture *in* the section).
+
+
+	.section .text.1.1, "ax", %progbits
+fadd_1:
+	.option	push
+	.option	arch, rv32i2p1_f2p2_zicsr2p0
+	fadd.s	fa0, fa0, fa1			# rv32if_zicsr
+	ret					# rv32if_zicsr
+	.option pop
+	# rv32i
+
+
+	.section .text.1.2, "ax", %progbits
+fadd_2:
+	.option	arch, rv32i2p1_f2p2_d2p2_zicsr2p0
+	fadd.d	fa0, fa0, fa1			# rv32ifd_zicsr
+	.option	arch, rv32i2p1_f2p2_d2p2_zicsr2p0
+	ret					# rv32ifd_zicsr
+	# rv32ifd_zicsr
+
+
+	.section .text.1.3, "ax", %progbits
+fadd_3:
+	.option	push
+	.option	arch, +q
+	fadd.q	fa0, fa0, fa1			# rv32ifdq_zicsr
+	.option	pop
+	.option	push
+	.option	arch, rv32i2p1_f2p2_d2p2_q2p2_zicsr2p0
+	ret					# rv32ifdq_zicsr
+	.option	pop
+	# rv32ifd_zicsr (written in .text.1.2)
+
+
+	.option arch, rv32i2p1_zicsr2p0
+	.section .text.2, "ax", %progbits
+add_1:
+	add a0, a0, a1				# rv32i_zicsr
+	ret					# rv32i_zicsr
+
+	# Final arch (for ELF attribute):
+	# rv32i_zicsr (set just before .section .text.2)

base-commit: 3190ebcbbf846617c0d5026995c26917f609a0f4
-- 
2.37.2

Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used

[Andreas Schwab]

On Okt 28 2022, Tsukasa OI via Binutils wrote:

> +   Set to true means we we need to add $x+arch at somewhere; Otherwise check

s/we we/we/

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used

[Tsukasa OI]

On 2022/10/29 0:02, Andreas Schwab wrote:
> On Okt 28 2022, Tsukasa OI via Binutils wrote:
> 
>> +   Set to true means we we need to add $x+arch at somewhere; Otherwise check
> 
> s/we we/we/
> 

Thanks.  It seems there are several wording issues in it (possibly
because it's a rushed job) and will change some sentences (including this).

Thanks,
Tsukasa

Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used

[Nelson Chu]

On Fri, Oct 28, 2022 at 10:52 PM Tsukasa OI
<research_trasio@irq.a4lg.com> wrote:
>
> GAS incorrectly emits instruction mapping symbols without ISA string if
> no segments use two or more architectures simultaneously.

First of all, thanks for pointing this out so quickly :)

> If no segments use two or more architectures (using .option directives),
> need_arch_map_symbol is not set to true, making riscv_check_mapping_symbols
> function to replace segment's mapping symbols ($x+arch) with $x.
>
> This easily occurs when a segment (section) is surrounded by .option
> push/pop.  In this example, even when a non-default architecture is used by
> the program, $x+arch mapping symbol emission is suppressed unless the
> architecture is changed **during** a section so that two instructions in one
> section use the different architecture.
>
> This commit renames need_arch_map_symbol to arch_changed_in_seg to reflect
> the actual role and suppresses $x+arch mapping symbol emission only if this
> variable is false (as before) **and** the section's architecture is the
> same as the default one (to be written as an ELF attribute).
>
> gas/ChangeLog:
>
>         * config/tc-riscv.c
>         (arch_changed_in_seg): Rename from need_arch_map_symbol.
>         (riscv_mapping_state): Reflect variable name change.
>         (riscv_check_mapping_symbols): Suppress emission of $x+arch only
>         if the default architecture is used in that segment.
>         * testsuite/gas/riscv/mapping-onearch-in-seg.s: New test.
>         * testsuite/gas/riscv/mapping-onearch-in-seg-dis.d: Likewise.
>         * testsuite/gas/riscv/mapping-onearch-in-seg-attr.d: Likewise.
>         * testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d: Likewise.
> ---
>  gas/config/tc-riscv.c                         | 26 ++++++-----
>  .../gas/riscv/mapping-onearch-in-seg-attr.d   |  6 +++
>  .../gas/riscv/mapping-onearch-in-seg-dis.d    | 30 +++++++++++++
>  .../riscv/mapping-onearch-in-seg-symbols.d    | 23 ++++++++++
>  .../gas/riscv/mapping-onearch-in-seg.s        | 44 +++++++++++++++++++
>  5 files changed, 119 insertions(+), 10 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
>  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
>  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
>  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 01bfc01b0fc..6d6aa631106 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -470,10 +470,12 @@ static char *expr_end;
>  #define OPCODE_MATCHES(OPCODE, OP) \
>    (((OPCODE) & MASK_##OP) == MATCH_##OP)
>
> -/* Indicate if .option directives do affect instructions.  Set to true means
> -   we need to add $x+arch at somewhere; Otherwise just add $x for instructions
> -   should be enough.  */
> -static bool need_arch_map_symbol = false;
> +/* Indicate if .option directives changed the architecture so that
> +   two or more architectures are simultaneously used in one segment.
> +   Set to true means we we need to add $x+arch at somewhere; Otherwise check
> +   final architecture (to be written as an ELF attribute) and decide whether
> +   we need to replace $x+arch with $x.  */
> +static bool arch_changed_in_seg = false;
>
>  /* Create a new mapping symbol for the transition to STATE.  */
>
> @@ -579,7 +581,7 @@ riscv_mapping_state (enum riscv_seg_mstate to_state,
>                       S_GET_NAME (seg_arch_symbol) + 2) != 0)
>      {
>        reset_seg_arch_str = true;
> -      need_arch_map_symbol = true;
> +      arch_changed_in_seg = true;

Add a new int flag in struct riscv_segment_info_type to replace the
need_arch_map_symbol.

struct riscv_segment_info_type
{
  enum riscv_seg_mstate map_state;
  /* The current mapping symbol with architecture string.  */
  symbolS *arch_map_symbol;
+ int arch_changed : 0;
};

and then set the flag to 1 here,
seg_info (now_seg)->tc_segment_info_data.arch_changed = 1;

>      }
>    else if (from_state == to_state)
>      return;
> @@ -634,11 +636,15 @@ riscv_check_mapping_symbols (bfd *abfd ATTRIBUTE_UNUSED,
>    if (seginfo == NULL || seginfo->frchainP == NULL)
>      return;
>
> -  /* If we don't set any .option arch directive, then the arch_map_symbol
> -     in each segment must be the first instruction, and we don't need to
> -     add $x+arch for them.  */

I prefer the comment here as,

/* If the section only has a mapping symbol at the first instruction,
and it is the same as
   the elf architecture attribute, then no need to add $x+arch for it.

   Please see gas/testsuite/gas/riscv/mapping.s: <your testcase
section name>.  */

> -  if (!need_arch_map_symbol
> -      && seginfo->tc_segment_info_data.arch_map_symbol != 0)
> +  if (!arch_changed_in_seg

if (!seg_info (now_seg)->tc_segment_info_data.arch_changed

> +      && seginfo->tc_segment_info_data.arch_map_symbol != 0
> +      && strcmp (riscv_rps_as.subset_list->arch_str,

There is a TODO that I had discussed with Kito a long time ago - the
elf arch attribute should be affected by the .option arch directives,
though the current implementation doesn't do that.  That means, the
mapping symbol here isn't necessarily the same as the elf arch
attribute.  So in this place, we should compare the saved elf arch
attribute, rather than the arch mapping symbol in each segment.
However, according to the current implementation, the final elf
attribute is the same as riscv_rps_as.subset_list->arch_str, so the
change is reasonable for now.

> +                S_GET_NAME (seginfo->tc_segment_info_data.arch_map_symbol) + 2)
> +        == 0)
>      S_SET_NAME (seginfo->tc_segment_info_data.arch_map_symbol, "$x");
>
>    for (fragp = seginfo->frchainP->frch_root;
> diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
> new file mode 100644
> index 00000000000..7bcf54766a3
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
> @@ -0,0 +1,6 @@
> +#as: -misa-spec=20191213 -march=rv32i2p1
> +#source: mapping-onearch-in-seg.s
> +#readelf: -A
> +Attribute Section: riscv
> +File Attributes
> +  Tag_RISCV_arch: "rv32i2p1_zicsr2p0"
> diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d

Don't need this test case.

> new file mode 100644
> index 00000000000..c086b92535c
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
> @@ -0,0 +1,30 @@
> +#as: -misa-spec=20191213 -march=rv32i2p1
> +#source: mapping-onearch-in-seg.s
> +#objdump: -d
> +
> +.*:[   ]+file format .*
> +
> +
> +Disassembly of section .text.1.1:
> +
> +0+000 <fadd_1>:
> +[      ]+[0-9a-f]+:[   ]+00b57553[     ]+fadd\.s[      ]+fa0,fa0,fa1
> +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> +
> +Disassembly of section .text.1.2:
> +
> +0+000 <fadd_2>:
> +[      ]+[0-9a-f]+:[   ]+02b57553[     ]+fadd\.d[      ]+fa0,fa0,fa1
> +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> +
> +Disassembly of section .text.1.3:
> +
> +0+000 <fadd_3>:
> +[      ]+[0-9a-f]+:[   ]+06b57553[     ]+fadd\.q[      ]+fa0,fa0,fa1
> +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> +
> +Disassembly of section .text.2:
> +
> +0+000 <add_1>:
> +[      ]+[0-9a-f]+:[   ]+00b50533[     ]+add[  ]+a0,a0,a1
> +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
> new file mode 100644
> index 00000000000..3a476a373b7
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
> @@ -0,0 +1,23 @@
> +#as: -misa-spec=20191213 -march=rv32i2p1
> +#source: mapping-onearch-in-seg.s
> +#objdump: --syms --special-syms
> +
> +.*file format.*riscv.*
> +
> +SYMBOL TABLE:
> +0+00 l    d  .text     0+00 .text
> +0+00 l    d  .data     0+00 .data
> +0+00 l    d  .bss      0+00 .bss
> +0+00 l    d  .text.1.1 0+00 .text.1.1
> +0+00 l       .text.1.1 0+00 fadd_1
> +0+00 l       .text.1.1 0+00 \$xrv32i2p1_f2p2_zicsr2p0
> +0+00 l    d  .text.1.2 0+00 .text.1.2
> +0+00 l       .text.1.2 0+00 fadd_2
> +0+00 l       .text.1.2 0+00 \$xrv32i2p1_f2p2_d2p2_zicsr2p0
> +0+00 l    d  .text.1.3 0+00 .text.1.3
> +0+00 l       .text.1.3 0+00 fadd_3
> +0+00 l       .text.1.3 0+00 \$xrv32i2p1_f2p2_d2p2_q2p2_zicsr2p0
> +0+00 l    d  .text.2   0+00 .text.2
> +0+00 l       .text.2   0+00 add_1
> +0+00 l       .text.2   0+00 \$x
> +0+00 l    d  .riscv.attributes 0+00 .riscv.attributes
> diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s b/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
> new file mode 100644
> index 00000000000..59b3f5dea98
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
> @@ -0,0 +1,44 @@
> +# In following examples, we use the same architecture per section
> +# (do not change the architecture *in* the section).
> +
> +
> +       .section .text.1.1, "ax", %progbits
> +fadd_1:
> +       .option push
> +       .option arch, rv32i2p1_f2p2_zicsr2p0
> +       fadd.s  fa0, fa0, fa1                   # rv32if_zicsr
> +       ret                                     # rv32if_zicsr
> +       .option pop
> +       # rv32i
> +
> +
> +       .section .text.1.2, "ax", %progbits
> +fadd_2:
> +       .option arch, rv32i2p1_f2p2_d2p2_zicsr2p0
> +       fadd.d  fa0, fa0, fa1                   # rv32ifd_zicsr
> +       .option arch, rv32i2p1_f2p2_d2p2_zicsr2p0
> +       ret                                     # rv32ifd_zicsr
> +       # rv32ifd_zicsr
> +
> +
> +       .section .text.1.3, "ax", %progbits
> +fadd_3:
> +       .option push
> +       .option arch, +q
> +       fadd.q  fa0, fa0, fa1                   # rv32ifdq_zicsr
> +       .option pop
> +       .option push
> +       .option arch, rv32i2p1_f2p2_d2p2_q2p2_zicsr2p0
> +       ret                                     # rv32ifdq_zicsr
> +       .option pop
> +       # rv32ifd_zicsr (written in .text.1.2)

For me, fadd_3 is redundant, fadd_2 and fadd_1 should be the same
thing, so just keep one of them.  Generally, the more test cases the
better, but short and precise test cases are what we really need.  I
appreciate you always spending lots of time to make risc-v better, but
sometimes you are adding lots of test cases, which might all just be
testing the similar thing.

> +
> +       .option arch, rv32i2p1_zicsr2p0
> +       .section .text.2, "ax", %progbits
> +add_1:
> +       add a0, a0, a1                          # rv32i_zicsr
> +       ret                                     # rv32i_zicsr
> +
> +       # Final arch (for ELF attribute):
> +       # rv32i_zicsr (set just before .section .text.2)

Please try to add the test cases into the mapping-symbol.s.

Thanks
Nelson

> base-commit: 3190ebcbbf846617c0d5026995c26917f609a0f4
> --
> 2.37.2
>

Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used

[Nelson Chu]

Forgot to say, if you want to suppress the mapping symbol to $x for
the section when it has the same arch with elf attribute, then you may
also need to update the riscv_get_map_state in the
opcodes/riscv-dis.c, to see if the $x is the first mapping symbol of
the section.  Since the first mapping symbol of the section is $x
means it is the same as elf arch attribute; Otherwise, $x means the
same as the previous arch mapping symbol.

Nelson

On Sat, Oct 29, 2022 at 3:10 AM Nelson Chu <nelson@rivosinc.com> wrote:
>
> On Fri, Oct 28, 2022 at 10:52 PM Tsukasa OI
> <research_trasio@irq.a4lg.com> wrote:
> >
> > GAS incorrectly emits instruction mapping symbols without ISA string if
> > no segments use two or more architectures simultaneously.
>
> First of all, thanks for pointing this out so quickly :)
>
> > If no segments use two or more architectures (using .option directives),
> > need_arch_map_symbol is not set to true, making riscv_check_mapping_symbols
> > function to replace segment's mapping symbols ($x+arch) with $x.
> >
> > This easily occurs when a segment (section) is surrounded by .option
> > push/pop.  In this example, even when a non-default architecture is used by
> > the program, $x+arch mapping symbol emission is suppressed unless the
> > architecture is changed **during** a section so that two instructions in one
> > section use the different architecture.
> >
> > This commit renames need_arch_map_symbol to arch_changed_in_seg to reflect
> > the actual role and suppresses $x+arch mapping symbol emission only if this
> > variable is false (as before) **and** the section's architecture is the
> > same as the default one (to be written as an ELF attribute).
> >
> > gas/ChangeLog:
> >
> >         * config/tc-riscv.c
> >         (arch_changed_in_seg): Rename from need_arch_map_symbol.
> >         (riscv_mapping_state): Reflect variable name change.
> >         (riscv_check_mapping_symbols): Suppress emission of $x+arch only
> >         if the default architecture is used in that segment.
> >         * testsuite/gas/riscv/mapping-onearch-in-seg.s: New test.
> >         * testsuite/gas/riscv/mapping-onearch-in-seg-dis.d: Likewise.
> >         * testsuite/gas/riscv/mapping-onearch-in-seg-attr.d: Likewise.
> >         * testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d: Likewise.
> > ---
> >  gas/config/tc-riscv.c                         | 26 ++++++-----
> >  .../gas/riscv/mapping-onearch-in-seg-attr.d   |  6 +++
> >  .../gas/riscv/mapping-onearch-in-seg-dis.d    | 30 +++++++++++++
> >  .../riscv/mapping-onearch-in-seg-symbols.d    | 23 ++++++++++
> >  .../gas/riscv/mapping-onearch-in-seg.s        | 44 +++++++++++++++++++
> >  5 files changed, 119 insertions(+), 10 deletions(-)
> >  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
> >  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
> >  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
> >  create mode 100644 gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
> >
> > diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> > index 01bfc01b0fc..6d6aa631106 100644
> > --- a/gas/config/tc-riscv.c
> > +++ b/gas/config/tc-riscv.c
> > @@ -470,10 +470,12 @@ static char *expr_end;
> >  #define OPCODE_MATCHES(OPCODE, OP) \
> >    (((OPCODE) & MASK_##OP) == MATCH_##OP)
> >
> > -/* Indicate if .option directives do affect instructions.  Set to true means
> > -   we need to add $x+arch at somewhere; Otherwise just add $x for instructions
> > -   should be enough.  */
> > -static bool need_arch_map_symbol = false;
> > +/* Indicate if .option directives changed the architecture so that
> > +   two or more architectures are simultaneously used in one segment.
> > +   Set to true means we we need to add $x+arch at somewhere; Otherwise check
> > +   final architecture (to be written as an ELF attribute) and decide whether
> > +   we need to replace $x+arch with $x.  */
> > +static bool arch_changed_in_seg = false;
> >
> >  /* Create a new mapping symbol for the transition to STATE.  */
> >
> > @@ -579,7 +581,7 @@ riscv_mapping_state (enum riscv_seg_mstate to_state,
> >                       S_GET_NAME (seg_arch_symbol) + 2) != 0)
> >      {
> >        reset_seg_arch_str = true;
> > -      need_arch_map_symbol = true;
> > +      arch_changed_in_seg = true;
>
> Add a new int flag in struct riscv_segment_info_type to replace the
> need_arch_map_symbol.
>
> struct riscv_segment_info_type
> {
>   enum riscv_seg_mstate map_state;
>   /* The current mapping symbol with architecture string.  */
>   symbolS *arch_map_symbol;
> + int arch_changed : 0;
> };
>
> and then set the flag to 1 here,
> seg_info (now_seg)->tc_segment_info_data.arch_changed = 1;
>
> >      }
> >    else if (from_state == to_state)
> >      return;
> > @@ -634,11 +636,15 @@ riscv_check_mapping_symbols (bfd *abfd ATTRIBUTE_UNUSED,
> >    if (seginfo == NULL || seginfo->frchainP == NULL)
> >      return;
> >
> > -  /* If we don't set any .option arch directive, then the arch_map_symbol
> > -     in each segment must be the first instruction, and we don't need to
> > -     add $x+arch for them.  */
>
> I prefer the comment here as,
>
> /* If the section only has a mapping symbol at the first instruction,
> and it is the same as
>    the elf architecture attribute, then no need to add $x+arch for it.
>
>    Please see gas/testsuite/gas/riscv/mapping.s: <your testcase
> section name>.  */
>
> > -  if (!need_arch_map_symbol
> > -      && seginfo->tc_segment_info_data.arch_map_symbol != 0)
> > +  if (!arch_changed_in_seg
>
> if (!seg_info (now_seg)->tc_segment_info_data.arch_changed
>
> > +      && seginfo->tc_segment_info_data.arch_map_symbol != 0
> > +      && strcmp (riscv_rps_as.subset_list->arch_str,
>
> There is a TODO that I had discussed with Kito a long time ago - the
> elf arch attribute should be affected by the .option arch directives,
> though the current implementation doesn't do that.  That means, the
> mapping symbol here isn't necessarily the same as the elf arch
> attribute.  So in this place, we should compare the saved elf arch
> attribute, rather than the arch mapping symbol in each segment.
> However, according to the current implementation, the final elf
> attribute is the same as riscv_rps_as.subset_list->arch_str, so the
> change is reasonable for now.
>
> > +                S_GET_NAME (seginfo->tc_segment_info_data.arch_map_symbol) + 2)
> > +        == 0)
> >      S_SET_NAME (seginfo->tc_segment_info_data.arch_map_symbol, "$x");
> >
> >    for (fragp = seginfo->frchainP->frch_root;
> > diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
> > new file mode 100644
> > index 00000000000..7bcf54766a3
> > --- /dev/null
> > +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-attr.d
> > @@ -0,0 +1,6 @@
> > +#as: -misa-spec=20191213 -march=rv32i2p1
> > +#source: mapping-onearch-in-seg.s
> > +#readelf: -A
> > +Attribute Section: riscv
> > +File Attributes
> > +  Tag_RISCV_arch: "rv32i2p1_zicsr2p0"
> > diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
>
> Don't need this test case.
>
> > new file mode 100644
> > index 00000000000..c086b92535c
> > --- /dev/null
> > +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-dis.d
> > @@ -0,0 +1,30 @@
> > +#as: -misa-spec=20191213 -march=rv32i2p1
> > +#source: mapping-onearch-in-seg.s
> > +#objdump: -d
> > +
> > +.*:[   ]+file format .*
> > +
> > +
> > +Disassembly of section .text.1.1:
> > +
> > +0+000 <fadd_1>:
> > +[      ]+[0-9a-f]+:[   ]+00b57553[     ]+fadd\.s[      ]+fa0,fa0,fa1
> > +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> > +
> > +Disassembly of section .text.1.2:
> > +
> > +0+000 <fadd_2>:
> > +[      ]+[0-9a-f]+:[   ]+02b57553[     ]+fadd\.d[      ]+fa0,fa0,fa1
> > +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> > +
> > +Disassembly of section .text.1.3:
> > +
> > +0+000 <fadd_3>:
> > +[      ]+[0-9a-f]+:[   ]+06b57553[     ]+fadd\.q[      ]+fa0,fa0,fa1
> > +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> > +
> > +Disassembly of section .text.2:
> > +
> > +0+000 <add_1>:
> > +[      ]+[0-9a-f]+:[   ]+00b50533[     ]+add[  ]+a0,a0,a1
> > +[      ]+[0-9a-f]+:[   ]+00008067[     ]+ret
> > diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
> > new file mode 100644
> > index 00000000000..3a476a373b7
> > --- /dev/null
> > +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg-symbols.d
> > @@ -0,0 +1,23 @@
> > +#as: -misa-spec=20191213 -march=rv32i2p1
> > +#source: mapping-onearch-in-seg.s
> > +#objdump: --syms --special-syms
> > +
> > +.*file format.*riscv.*
> > +
> > +SYMBOL TABLE:
> > +0+00 l    d  .text     0+00 .text
> > +0+00 l    d  .data     0+00 .data
> > +0+00 l    d  .bss      0+00 .bss
> > +0+00 l    d  .text.1.1 0+00 .text.1.1
> > +0+00 l       .text.1.1 0+00 fadd_1
> > +0+00 l       .text.1.1 0+00 \$xrv32i2p1_f2p2_zicsr2p0
> > +0+00 l    d  .text.1.2 0+00 .text.1.2
> > +0+00 l       .text.1.2 0+00 fadd_2
> > +0+00 l       .text.1.2 0+00 \$xrv32i2p1_f2p2_d2p2_zicsr2p0
> > +0+00 l    d  .text.1.3 0+00 .text.1.3
> > +0+00 l       .text.1.3 0+00 fadd_3
> > +0+00 l       .text.1.3 0+00 \$xrv32i2p1_f2p2_d2p2_q2p2_zicsr2p0
> > +0+00 l    d  .text.2   0+00 .text.2
> > +0+00 l       .text.2   0+00 add_1
> > +0+00 l       .text.2   0+00 \$x
> > +0+00 l    d  .riscv.attributes 0+00 .riscv.attributes
> > diff --git a/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s b/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
> > new file mode 100644
> > index 00000000000..59b3f5dea98
> > --- /dev/null
> > +++ b/gas/testsuite/gas/riscv/mapping-onearch-in-seg.s
> > @@ -0,0 +1,44 @@
> > +# In following examples, we use the same architecture per section
> > +# (do not change the architecture *in* the section).
> > +
> > +
> > +       .section .text.1.1, "ax", %progbits
> > +fadd_1:
> > +       .option push
> > +       .option arch, rv32i2p1_f2p2_zicsr2p0
> > +       fadd.s  fa0, fa0, fa1                   # rv32if_zicsr
> > +       ret                                     # rv32if_zicsr
> > +       .option pop
> > +       # rv32i
> > +
> > +
> > +       .section .text.1.2, "ax", %progbits
> > +fadd_2:
> > +       .option arch, rv32i2p1_f2p2_d2p2_zicsr2p0
> > +       fadd.d  fa0, fa0, fa1                   # rv32ifd_zicsr
> > +       .option arch, rv32i2p1_f2p2_d2p2_zicsr2p0
> > +       ret                                     # rv32ifd_zicsr
> > +       # rv32ifd_zicsr
> > +
> > +
> > +       .section .text.1.3, "ax", %progbits
> > +fadd_3:
> > +       .option push
> > +       .option arch, +q
> > +       fadd.q  fa0, fa0, fa1                   # rv32ifdq_zicsr
> > +       .option pop
> > +       .option push
> > +       .option arch, rv32i2p1_f2p2_d2p2_q2p2_zicsr2p0
> > +       ret                                     # rv32ifdq_zicsr
> > +       .option pop
> > +       # rv32ifd_zicsr (written in .text.1.2)
>
> For me, fadd_3 is redundant, fadd_2 and fadd_1 should be the same
> thing, so just keep one of them.  Generally, the more test cases the
> better, but short and precise test cases are what we really need.  I
> appreciate you always spending lots of time to make risc-v better, but
> sometimes you are adding lots of test cases, which might all just be
> testing the similar thing.
>
> > +
> > +       .option arch, rv32i2p1_zicsr2p0
> > +       .section .text.2, "ax", %progbits
> > +add_1:
> > +       add a0, a0, a1                          # rv32i_zicsr
> > +       ret                                     # rv32i_zicsr
> > +
> > +       # Final arch (for ELF attribute):
> > +       # rv32i_zicsr (set just before .section .text.2)
>
> Please try to add the test cases into the mapping-symbol.s.
>
> Thanks
> Nelson
>
> > base-commit: 3190ebcbbf846617c0d5026995c26917f609a0f4
> > --
> > 2.37.2
> >