From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2d.google.com (mail-oa1-x2d.google.com [IPv6:2001:4860:4864:20::2d]) by sourceware.org (Postfix) with ESMTPS id 81BB63858D20 for ; Fri, 28 Oct 2022 19:17:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 81BB63858D20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com Received: by mail-oa1-x2d.google.com with SMTP id 586e51a60fabf-13c2cfd1126so7329759fac.10 for ; Fri, 28 Oct 2022 12:17:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=EABeJE86wM/M12jOAVmRiC0uhPkbln2O+gH/UaAI7sw=; b=o1+AwzbAMqcictsDAuTun6RIzquvusrt3aXSP46R1P+jUupwxJe7doA4y/AqL9X72M 5iwaLRriP5nKv2z0Kz4YCDfhjuGeuoc/whxhdyTVrNkpgOgmLY8YrDWnlDc1KVehqzcT j6H+HzVyvxZkCauhQCgXuAwxoRPCsfQFfXCVdCszWyOHLp8FLgrgIfT245gGgp1w6TGx BA3ScUUBOAOiVwpoMeTWs8XLX+Bwk4tRVfdmQsBL3lIt7GL8vS84tuOZFR91a4b+nUKc +iCvz6s4VuI1yyqX0R987Ul7HUACZK6i8OCMTFfZZD1jXUi/JZKXSM74YNPypm0WNgxf 2w1Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=EABeJE86wM/M12jOAVmRiC0uhPkbln2O+gH/UaAI7sw=; b=LvwSN1rX3GJYFWNmAwjGHx1WGYzjqgEx3zGW4kav4td3E09B8mfmNZ6lJSi/DTRhkg 81mmtvM//t3AewsFlQwqhiWOa8AmY504sWhNNt2iZbgBk3eSCtlm32cqN2RYU1C/Cq0J M0fADDSHtJqo5T02tHrouMmtxU/iOcHmQV4vFIOPPSJzuyBIP30INCnmDrF89XSIUzFA Ms6ZDJqxVGMgdKgqc+Ju8zjtMwkQ8syNYzdtNvfw0pAO0JXjoOusz3M+vl25yZeSWt62 1k5k3HWT2I192M/YVLYLP53JeuHSNFSMpkiLsUPQWD/cGNolgrKYG1IGM9xMGNv8cdwR RT6w== X-Gm-Message-State: ACrzQf1BUzIY0n3V6fuKTC4a+Vvp4rWzEMK+yDVIs7Ob1qFX3MfmIFEq LYocjHq6Z618txAH0+JJPPTuHXy1H/nq9FdqjeXo8eTNfYkVpQ== X-Google-Smtp-Source: AMsMyM6B5rc47rJijTTEuMSEBzyhq7iPZwO2X2gF/SiG3TdigBvT2RtxkeQfZa+CKzYXdseF+nWfLxecy3Cj84zlnLw= X-Received: by 2002:a05:6870:507:b0:130:ae8d:db0e with SMTP id j7-20020a056870050700b00130ae8ddb0emr10379385oao.82.1666984677907; Fri, 28 Oct 2022 12:17:57 -0700 (PDT) MIME-Version: 1.0 References: <17f57574936af82be381a1451eac56b3709b60bb.1666968673.git.research_trasio@irq.a4lg.com> In-Reply-To: From: Nelson Chu Date: Sat, 29 Oct 2022 03:17:47 +0800 Message-ID: Subject: Re: [PATCH] RISC-V: Emit mapping symbol with ISA string if non-default arch is used To: Tsukasa OI Cc: Kito Cheng , Palmer Dabbelt , binutils@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 wrote: > > On Fri, Oct 28, 2022 at 10:52 PM Tsukasa OI > 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: 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 : > > +[ ]+[0-9a-f]+:[ ]+00b57553[ ]+fadd\.s[ ]+fa0,fa0,fa1 > > +[ ]+[0-9a-f]+:[ ]+00008067[ ]+ret > > + > > +Disassembly of section .text.1.2: > > + > > +0+000 : > > +[ ]+[0-9a-f]+:[ ]+02b57553[ ]+fadd\.d[ ]+fa0,fa0,fa1 > > +[ ]+[0-9a-f]+:[ ]+00008067[ ]+ret > > + > > +Disassembly of section .text.1.3: > > + > > +0+000 : > > +[ ]+[0-9a-f]+:[ ]+06b57553[ ]+fadd\.q[ ]+fa0,fa0,fa1 > > +[ ]+[0-9a-f]+:[ ]+00008067[ ]+ret > > + > > +Disassembly of section .text.2: > > + > > +0+000 : > > +[ ]+[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 > >