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

[Nelson Chu <nelson@rivosinc.com>]

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
>