Re: [PATCH v8 0/7] RISC-V: Fix address printer on the disassembler

[Nelson Chu <nelson@rivosinc.com>]

Thanks, committed patches 1-5 for pr29342.

Nelson

On Sat, Aug 27, 2022 at 8:11 AM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> Hello,
>
> Tracker on GitHub:
> <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_fix_addr>
>
> Original Issue as reported by H. Peter Anvin:
> <https://sourceware.org/bugzilla/show_bug.cgi?id=29342>
>
> Previous Information on v1...v7:
> <https://sourceware.org/pipermail/binutils/2022-August/122552.html>
> See this e-mail for background.
>
>
>
> Based on Nelson's feedback, I functionally splitted the patchset to four
> mostly independent parts.  Tests are self-contained to each part (unlike
> dis-addr-2.s in PATCH v7) so that we can review each part independently.
>
> (1) [PATCH 1-1] Additional tests for existing part: sequence with ADDIW
>                 (because we will test other cases in detail on other parts)
> (2) [PATCH 2-3] Fix to PR29342 and a new JALR issue
>                 (both discovered by H. Peter Anvin)
> (3) [PATCH 4-5] Make the disassembler able to print the highest address
>                 and GP-relative addresses even if the GP is the top addr
> (4) [PATCH 6-7] Code Tidying: Rename and retype `wide' parameter of
>                 `maybe_print_address' for further clarification of the code
>
>
> On 2022/08/24 20:22, Nelson Chu wrote:
> > I think there are three dis-assembler issues here,
> >
> > 1. PR29342
> > 2. The target address of jump
> > 3. Should we show the target address when it is -1
>
> cf. <https://sourceware.org/pipermail/binutils/2022-August/122573.html>
>
> Based on Nelson's classification, part (2) corresponds [1.] and [2.] and
> (3) corresponds [3.].  About issues [1.] and [2.], PATCH 2 fixes [2.] and
> PATCH 3 fixes [1.] but tests for [2.] is in the PATCH 3 (not PATCH 2).
>
> This is because PATCH 3 tests address printing in various ways including
> JALR instructions.  So splitting them to PATCH 2 seemed too redundant.
>
>
> And, I couldn't come up with a good explanation to `binutils/NEWS'
> describing the changes in the part (3).  Could someone help me?  It enables
> printing following addresses on disassembling:
>
> -   The highest address (-1)
> -   GP-relative addresses if GP has the highest address (-1)
>
>
>
> [Extraction Guide]
>
> If we extract each part ((1) through (4)) using git rebase, we may
> encounter some conflicts and test failures.  Even so, to extract (3) and
> (4), I recommend to apply at least (2) through (4) first and then extract
> using git rebase.
>
> (1)
>     This is completely independent from the rest.
> (2)
>     This is independent from (1).
> (3)
>     This is independent from (1).
>     This is MOSTLY independent from (2) but extracting causes one conflict
>     and two test errors (that depend on the bugfix on (2)):
>         # Conflict: keep those lines
>           else
>             return;  /* Don't print the address.  */
>           pd->to_print_addr = true;
>         # Test Failures: two changes are required to pass check-gas
>             # gas/testsuite/gas/riscv/dis-addr-topaddr-32.d
>                 old: <addr_top>
>                 new: <addr_top\+0x0>
>             # gas/testsuite/gas/riscv/dis-addr-topaddr-gp-32.d
>                 old: <addr_rel_gp_pos>
>                 new: <__global_pointer\$\+0x5>
> (4)
>     This is independent from (1).
>     This is independent from (3).
>     This is MOSTLY independent from (2) but extracting causes one conflict.
>         # Conflict: change like this and keep (to NOT keep the bugfix in (2))
>         old: maybe_print_address (pd, rs1, 0, 0);
>         new: maybe_print_address (pd, rs1, 0, false);
>
>
>
>
> Tsukasa OI (7):
>   RISC-V: Add address printer tests with ADDIW
>   RISC-V: Fix JALR target address computation
>   RISC-V: Fix RV32 disassembler address computation
>   RISC-V: Print highest address on the disassembler
>   RISC-V: Print top GP-relative addresses on the disassembler
>   RISC-V: Clarify that `wide' is only used for ADDIW
>   RISC-V: Make `is_addiw' parameter bool
>
>  gas/testsuite/gas/riscv/dis-addr-addiw-a.d    | 18 +++++
>  gas/testsuite/gas/riscv/dis-addr-addiw-b.d    | 18 +++++
>  gas/testsuite/gas/riscv/dis-addr-addiw.s      | 28 ++++++++
>  .../gas/riscv/dis-addr-overflow-32.d          | 30 ++++++++
>  .../gas/riscv/dis-addr-overflow-64.d          | 34 +++++++++
>  gas/testsuite/gas/riscv/dis-addr-overflow.s   | 70 +++++++++++++++++++
>  gas/testsuite/gas/riscv/dis-addr-topaddr-32.d | 11 +++
>  gas/testsuite/gas/riscv/dis-addr-topaddr-64.d | 11 +++
>  .../gas/riscv/dis-addr-topaddr-gp-32.d        | 12 ++++
>  .../gas/riscv/dis-addr-topaddr-gp-64.d        | 12 ++++
>  gas/testsuite/gas/riscv/dis-addr-topaddr-gp.s | 15 ++++
>  gas/testsuite/gas/riscv/dis-addr-topaddr.s    | 10 +++
>  gas/testsuite/gas/riscv/lla32.d               |  2 +-
>  opcodes/riscv-dis.c                           | 46 +++++++-----
>  14 files changed, 300 insertions(+), 17 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-addiw-a.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-addiw-b.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-addiw.s
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-overflow-32.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-overflow-64.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-overflow.s
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-topaddr-32.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-topaddr-64.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-topaddr-gp-32.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-topaddr-gp-64.d
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-topaddr-gp.s
>  create mode 100644 gas/testsuite/gas/riscv/dis-addr-topaddr.s
>
>
> base-commit: 46e59b72f21029f2a863e3828cec76a03283b49d
> --
> 2.34.1
>