* [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used @ 2022-10-28 14:52 Tsukasa OI 2022-10-28 15:02 ` Andreas Schwab 2022-10-28 19:10 ` Nelson Chu 0 siblings, 2 replies; 5+ messages in thread From: Tsukasa OI @ 2022-10-28 14:52 UTC (permalink / raw) To: Tsukasa OI, Nelson Chu, Kito Cheng, Palmer Dabbelt; +Cc: binutils 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used 2022-10-28 14:52 [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used Tsukasa OI @ 2022-10-28 15:02 ` Andreas Schwab 2022-10-28 15:05 ` Tsukasa OI 2022-10-28 19:10 ` Nelson Chu 1 sibling, 1 reply; 5+ messages in thread From: Andreas Schwab @ 2022-10-28 15:02 UTC (permalink / raw) To: Tsukasa OI via Binutils Cc: Tsukasa OI, Nelson Chu, Kito Cheng, Palmer Dabbelt 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." ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used 2022-10-28 15:02 ` Andreas Schwab @ 2022-10-28 15:05 ` Tsukasa OI 0 siblings, 0 replies; 5+ messages in thread From: Tsukasa OI @ 2022-10-28 15:05 UTC (permalink / raw) To: Andreas Schwab, Binutils 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used 2022-10-28 14:52 [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used Tsukasa OI 2022-10-28 15:02 ` Andreas Schwab @ 2022-10-28 19:10 ` Nelson Chu 2022-10-28 19:17 ` Nelson Chu 1 sibling, 1 reply; 5+ messages in thread From: Nelson Chu @ 2022-10-28 19:10 UTC (permalink / raw) To: Tsukasa OI; +Cc: Kito Cheng, Palmer Dabbelt, binutils 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used 2022-10-28 19:10 ` Nelson Chu @ 2022-10-28 19:17 ` Nelson Chu 0 siblings, 0 replies; 5+ messages in thread From: Nelson Chu @ 2022-10-28 19:17 UTC (permalink / raw) To: Tsukasa OI; +Cc: Kito Cheng, Palmer Dabbelt, binutils 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-28 19:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-28 14:52 [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used Tsukasa OI 2022-10-28 15:02 ` Andreas Schwab 2022-10-28 15:05 ` Tsukasa OI 2022-10-28 19:10 ` Nelson Chu 2022-10-28 19:17 ` Nelson Chu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).