[PATCH] RISC-V: Dump CSR according to the elf privileged spec attributes.

[Nelson Chu]

On Thu, Dec 10, 2020 at 8:32 AM Jim Wilson <jimw@sifive.com> wrote:
>
> On Tue, Dec 8, 2020 at 1:59 AM Nelson Chu <nelson.chu@sifive.com> wrote:
>>
>> opcodes/
>>     * disassemble.h (riscv_get_disassembler): Declare.
>>     * disassemble.c (disassembler): Changed to riscv_get_disassembler.
>>     * riscv-dis.c (riscv_get_disassembler): Check the elf privileged spec
>>     attributes before calling print_insn_riscv.
>>     (parse_riscv_dis_option): Same as the assembler, the priority of elf
>>     attributes are higher than the options.  If we find the privileged
>>     attributes, but the -Mpriv-spec= is different, then output error/warning
>>     and still use the elf attributes set.
>
>
> This looks OK to me.

Committed, thanks.

On Thu, Dec 10, 2020 at 8:31 AM Jim Wilson <jimw@sifive.com> wrote:
> The comments at the top of opcodes/disassembler.h and include/dis-asm.h explain their purpose.  disassembler.h is for internal use in opcodes.  dis-asm.h is for communication between opcodes and users like gdb.

Thanks for pointing this out!

> The arc get_disassembler is in dis-asm.h because gdb used to call it directly.  If I look at gdb-5.2.1 I see that gdb/arc-tdep.c has a arc_print_insn function that calls arc_get_disassembler.  The gdb arc port was dropped in 2002.  Then a new one was added in 2016.  The new one does not call arc_get_disassembler.  It looks like the declaration in dis-asm.h is an old artifact, and it should be moved to disassembler.h as a separate patch, or maybe file a bug report for the arc maintainer to fix.  The story with cris in simpler, I can see the cris_get_disassembler call being removed from gdb in an infrastructure change in 2017.  However, there is a cris gdb sim that still calls it, so it looks like we still need the declaration in dis-asm.h.  Unless we rewrite the simulator to avoid the call.

Got it, so since riscv-gdb doesn't call the get_disassembler, and the
declarations of get_disassembler should be old implementations, it's
fine to declare the get_disassembler in the opcodes/disassembler.h
only.

> As for gdb, I'd say that there are 3 sources for the priv spec, the priv spec specified by the target (gdb server/openocd), the priv spec specified by the elf file, and the priv spec specified by the user.  The one returned by the target probably should take precedence.  However, currently, I think there is no way for the user to specify this.  And probably no way for the target xml file to specify this.  If this xml feature is added later, gdb should probably use disassembler_options to pass the info to the disassembler, so your patch should still work.  There is just the precedence issue, but if you are always warning it should be OK.

Thanks for clarifying this again.  As for the precedence issue, yeah
Kito also gives the same suggestion - always warning, in case we want
to change the rules but forgot where to update...

Anyway, thanks for the detailed description about the old history artifact :)

Nelson