[PATCH] PR32705 opcodes: fix RISC-V $x mapping symbol
Nelson Chu
nelson@rivosinc.com
Mon Feb 17 03:34:14 GMT 2025
I think we should fix the behavior of assembler too in the same patch, so
that is a complete GNU feature, and don't need the binary testcase.
There is a compatibility issue here since all objects, which were built for
three years, will need to be rebuilt when the behavior changes, which will
cause troubles. The implementation was committed first, but the spec
merged the $x rule to be the same as elf attr later after the
implementation, and it seems no one figured out the rule conflicts with the
existing behavior at that time... I think maybe It's too late to change
the psabi, so the whole GNU behavior, which means assembler and
dis-assembler, should be fixed at the same time.
Nelson
On Mon, Feb 17, 2025 at 3:15 AM <andrew@andrewoates.com> wrote:
> From: Andrew Oates <andrew@andrewoates.com>
>
> The mapping symbol "$x" without an ISA string "means using ISA
> configuration from ELF attribute."[1]. Currently the code does not
> reset the subset_list. This means that a previous mapping symbol that
> overrides the ISA string will continue to be used, rather than the
> default string set in the ELF file's .riscv.attributes section. This
> can cause incorrect or failed instruction decodings.
>
> In practice, this causes problems when disassembling code generated by
> LLVM, which (unlike gas) does not emit explicit mapping symbols at the
> start of each section.
>
> This change stores the default architecture string seen at the beginning
> of disassembly in the global parse data struct, and restores that to
> subset_list whenever a bare "$x" symbol is seen.
>
> Test case object file generated per repro instructions in PR32705.
>
> * opcodes/riscv-dis.c
> (riscv_init_disasm_info): store default_arch in global
> riscv_private_data struct
> (riscv_update_map_state): used stored default when a "$x"
> symbol is seen
>
> binutils/testsuite/binutils-all/riscv/
> * riscv.exp: new test case
> * pr32705.o.bz2: new file (test input)
> * pr32705.o.dump: new file (test output)
>
> [1]
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#mapping-symbol
> ---
> .../binutils-all/riscv/pr32705.o.bz2 | Bin 0 -> 630 bytes
> .../binutils-all/riscv/pr32705.o.dump | 13 ++++++++++++
> .../testsuite/binutils-all/riscv/riscv.exp | 20 ++++++++++++++++++
> opcodes/riscv-dis.c | 18 ++++++++++------
> 4 files changed, 45 insertions(+), 6 deletions(-)
> create mode 100755 binutils/testsuite/binutils-all/riscv/pr32705.o.bz2
> create mode 100644 binutils/testsuite/binutils-all/riscv/pr32705.o.dump
>
> diff --git a/binutils/testsuite/binutils-all/riscv/pr32705.o.bz2
> b/binutils/testsuite/binutils-all/riscv/pr32705.o.bz2
> new file mode 100755
> index
> 0000000000000000000000000000000000000000..9b7fb4d67394b25908db2ffef51e14967dc51b4b
> GIT binary patch
> literal 630
> zcmV-+0*U=XT4*^jL0KkKStLa_B>(}`f9L=H_+D=F|MYjIEpos2|8md(KmY(h0ssO;
> zXaK+g+z7FTieybkiKHHrCWojE02w_%8&RMDX|)4G)P7Vn$Qr5Xh)ttS5S~p>Qy>O_
> zVgSMb42&QEG<rbbXblF9115k2Kmm!Nrh%q_8Z>ACXwYIrn^RLw9-?MVO)ws)**#Ml
> z2sFTGjExK$38oWGpoL1I;6{XUrUU<i_*sfOl+blFK>W@+^q(UANr>tE#k=>))e?0H
> zdNO9L0HBG($eqL&%+tA+I}j@(NZr=cwZYH*&W8dtg7#`Tzye1K1J)4nuu@^>{TyIk
> zM`8i!l#VXb5(G-lYY<4Fuemy%)+z={INK2pSbZDRt;u3BX$qi<!ulthyO!=))e%V;
> z)Q}j9pxW`gT__-sEQt+Wzi1yGb|TdqFnA`LQusWIO3J-hNNuS?AOVBQ!4?761eZ`#
> z<_1j`PVttis}@v+woe~45zSnZ<<zsL<ltmHWW5N6X5#_VlKFx1wE5(1*r_%y^dF+5
> z(xHNOg*nK8=?C7SPaN^5z1sduWu6o_NeW?Br54h=0YJjVC)#buLoRDX{B_!pz#&CT
> zexOBX;W#r_s}_D83hqAk$b<7H(8)dTWj6HJbJ5!?(imz@3-c-S)NOCJ;U!jeNh)nv
> z(yf_hdjz<l8PnHTRt1gMTnEMv1|5BABq>n?WT815icmfvhABZsk{V7Rr*Ni8$7Pcq
> zdP6G5T_*V>R9I$XNi+m6>kxXOr^5ln5gAV;BvOb$m>_ltxbEHAN&Ij_pkOwHAQ#zz
> Q{9oekNT&)C1c;{Opf{8mTL1t6
>
> literal 0
> HcmV?d00001
>
> diff --git a/binutils/testsuite/binutils-all/riscv/pr32705.o.dump
> b/binutils/testsuite/binutils-all/riscv/pr32705.o.dump
> new file mode 100644
> index 00000000000..6204da92aa3
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/riscv/pr32705.o.dump
> @@ -0,0 +1,13 @@
> +#objdump: -d
> +# Test handling of $x mapping symbols to ensure they reset the current
> +# architecture correctly.
> +
> +#...
> +Disassembly of section \.text:
> +
> +00000000000100e8 <func-0x4>:
> + 100e8: 00100513 li a0,1
> +
> +00000000000100ec <func>:
> + 100ec: 2505 addiw a0,a0,1
> + 100ee: 8082 ret
> diff --git a/binutils/testsuite/binutils-all/riscv/riscv.exp
> b/binutils/testsuite/binutils-all/riscv/riscv.exp
> index 8847af8ac9d..66caf7f532a 100644
> --- a/binutils/testsuite/binutils-all/riscv/riscv.exp
> +++ b/binutils/testsuite/binutils-all/riscv/riscv.exp
> @@ -27,3 +27,23 @@ foreach t $test_list {
> verbose [file rootname $t]
> run_dump_test [file rootname $t]
> }
> +
> +set test $srcdir/$subdir/pr32705.o.bz2
> +# We need to strip the ".bz2", but can leave the dirname.
> +set t $subdir/[file tail $test]
> +set testname [file rootname $t]
> +set tempfile tmpdir/pr32705.o
> +set dumpfile tmpdir/pr32705.out
> +set objfile [file rootname $test]
> +if {[catch "system \"bzip2 -dc $test > $tempfile\""] != 0} {
> + untested "bzip2 -dc ($testname)"
> +} else {
> + verbose [file rootname $t]
> + if {[catch "system \"$OBJDUMP -d $tempfile > $dumpfile\""] != 0} {
> + fail $testname
> + } else {
> + if {[ regexp_diff $dumpfile "${objfile}.dump" ]} {
> + fail $testname
> + }
> + }
> +}
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 84c6deef7b6..7065fdecc02 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -52,6 +52,8 @@ struct riscv_private_data
> enum riscv_spec_class default_priv_spec;
> /* Used for architecture parser. */
> riscv_parse_subset_t riscv_rps_dis;
> + /* Default architecture string for the object file. */
> + const char* default_arch;
> /* Used for mapping symbols. */
> int last_map_symbol;
> bfd_vma last_stop_offset;
> @@ -1065,10 +1067,14 @@ riscv_update_map_state (int n,
> return;
>
> name = bfd_asymbol_name(info->symtab[n]);
> - if (strcmp (name, "$x") == 0)
> - *state = MAP_INSN;
> - else if (strcmp (name, "$d") == 0)
> + if (strcmp (name, "$d") == 0)
> *state = MAP_DATA;
> + else if (strcmp (name, "$x") == 0)
> + {
> + *state = MAP_INSN;
> + riscv_release_subset_list (pd->riscv_rps_dis.subset_list);
> + riscv_parse_subset (&pd->riscv_rps_dis, pd->default_arch);
> + }
> else if (strncmp (name, "$xrv", 4) == 0)
> {
> *state = MAP_INSN;
> @@ -1372,7 +1378,7 @@ riscv_init_disasm_info (struct disassemble_info
> *info)
> pd->riscv_rps_dis.xlen = &pd->xlen;
> pd->riscv_rps_dis.isa_spec = &pd->default_isa_spec;
> pd->riscv_rps_dis.check_unknown_prefixed_ext = false;
> - const char *default_arch = "rv64gc";
> + pd->default_arch = "rv64gc";
> if (info->section != NULL)
> {
> bfd *abfd = info->section->owner;
> @@ -1390,12 +1396,12 @@ riscv_init_disasm_info (struct disassemble_info
> *info)
> attr[Tag_b].i,
> attr[Tag_c].i,
>
> &pd->default_priv_spec);
> - default_arch = attr[Tag_RISCV_arch].s;
> + pd->default_arch = attr[Tag_RISCV_arch].s;
> }
> }
> }
> riscv_release_subset_list (pd->riscv_rps_dis.subset_list);
> - riscv_parse_subset (&pd->riscv_rps_dis, default_arch);
> + riscv_parse_subset (&pd->riscv_rps_dis, pd->default_arch);
>
> pd->last_map_symbol = -1;
> pd->last_stop_offset = 0;
> --
> 2.47.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/binutils/attachments/20250217/9984b44b/attachment-0001.htm>
More information about the Binutils
mailing list