This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] arc: Select CPU model properly before disassembling
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