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

[Nelson Chu <nelson@rivosinc.com>]

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
> >