Re: [PATCH v2] arc: Select CPU model properly before disassembling

[Pedro Alves <palves at redhat dot com>]

On 06/14/2017 05:27 PM, Simon Marchi wrote:
> On 2017-06-14 16:02, Anton Kolesov wrote:
>> This is the same thing as in {arm,rs6000,s390-linux}-tdep.c - they all
>> use
>> global static variable, which would be shared across multiple gdbarch
>> instances. So when options are changed that would change that for any
>> gdbarch
>> of that architecture. RS6000 and S390 don't even assign this variable
>> any value
>> - it completely managed by the disasm.c. Although in ARC case there is
>> a memory
>> leak, because arc_disassembler_options is only assigned by arc-tdep.c and
>> never freed - that's because I didn't properly understood what other
>> arches
>> do - they don't free options as well, but they also don't allocate
>> them in
>> each gdbarch_init, because there usecase is somewhat different.
> 
> Indeed, set_disassembler_options takes care of the
> allocation/deallocation when the user changes the options.
> 
>> I'm not sure
>> how big of a problem this leak is, because outside of a GDB test suite
>> I don't
>> think there are a lot of practical cases where same GDB instance is
>> reused to
>> connect/disconnect, so leak wouldn't be noticeable. I think, I can make
>> things better for ARC, by moving arc_disassembler_options into the
>> gdbarch_tdep, then each gdbarch for ARC will have its own options, but
>> that
>> would mean behavior different from other arches which support
>> disassembler_options - there options are shared across gdbarches of
>> architecture. Should I do that in V3?
> 
> Ah, I did not understand previously that sharing the disassembler
> options between all gdbarches of an architecture is the intended
> behavior.  Having different behavior across architectures for this
> wouldn't be a good idea, I think.  Also, as of today, the disassembler
> options are very much user driven.  The architecture family can set some
> default at startup (ARM, for example, sets "reg-names-std"), but after
> that it's all up to the user.

Right, in a given debug session you can have different
gdbarch instances at play even for the same cpu architecture.
E.g., the gdbarch determined for the binary vs the gdbarch created
for the target description.

> 
> In your case, it's the opposite along those two axis: you want GDB to
> set disassembler options in the back of the user, and you want
> disassembler options (at least some of them) to be gdbarch-specific.  To
> support that correctly, I think we should first define what behavior we
> want from GDB.  For example:
> 
>  - if the user doesn't specify any disassembler-options, it's probably
> right to automatically set it for them.
>  - if the user provides a disassembler option other than cpu= and then
> connects to a gdbserver, we probably want to add the cpu= to what they
> have specified.
>  - if they connect first and then set another, unrelated option, do we
> keep the cpu= option or does it get lost?
>  - if they set a particular cpu= option and then connect to a gdbserver
> that reports something else, which one do we choose?
>  - if they connect to a gdbserver that reports an architecture and we
> add a cpu= for it, then they connect to a gdbserver that doesn't report
> an architecture, do we keep the previous cpu= or not?
>  - what happens with the various options when we switch gdbarch, do we
> keep the user-provided ones but flush the GDB-inferred ones?

I'd think that the "set/show disassembler-options" setting
wouldn't ever be changed by connecting to a target and
that whatever the user explicitly set should be respected.

I.e., gdb / opcodes would choose which cpu to use for disassembly
based on this logical sequence:

 - if user specified cpu, use that,
 - otherwise, if tdesc specified cpu, use that,
 - otherwise, infer cpu from bfd/elf section,
 - otherwise, use default cpu

Thanks,
Pedro Alves