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

[Simon Marchi <simon.marchi@polymtl.ca>]

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.

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?

Once we know what behavior we want from GDB, I think it will be easier 
to decide what to put where.

In the mean time, if you think the current patch makes the situation 
better for ARC users in the typical usage scenarios, I'm fine with going 
with it.  Note that there will be a change for your users: connecting to 
a target will overwrite whatever option they might have set before 
connecting.  Also, to avoid the leak, you could xfree the previous 
arc_disassembler_options before assigning it in arc_gdbarch_init.

> TBH, that would be a moot for ARC right now, because today our 
> disassembler
> has a global state and it doesn't support simultaneous disassembly of
> different ARC architectures anyway, but it might make sense to try to 
> make
> sure that GDB part handles this correctly, if in the future 
> disassembler
> would be upgraded to avoid global state.
> 
> The reuse of gdbarch instances seems like a good idea, will try to add 
> that
> in the future for ARC.
> 
> Anton

Thanks,

Simon