This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Simon,

> > + if (info->disassembler_options == NULL)
> > +    {
> > +      struct obj_section * s = find_pc_section (addr);
> 
> Extra space after the *.

This was copied from mep-tdep.c. Should I make a separate patch that fixes it
there too?

> 
> > +      if (s != NULL)
> > +	info->section = s->the_bfd_section;
> > +    }
> 
> When printing multiple instructions in a row, is this function called multiple
> times with the same disassemble_info?  If so, are we going to make a
> find_pc_section call for each instruction?  Is this needed, given that the
> options won't change for a given ELF (on ARC at least)?

I think it might not work if there is a multi-arch target and disassembler
invocation tries to dump instructions across multiple sections which come from
different ELF files (not sure if shared libraries would count as a separate ELF
files). But usually disassembler works per-function and practically I don't
think that there would be a case of function that crosses section boundary.
I've put this optimization to my v3 patch.

> 
> I was a bit concerned with the fact that multiple gdbarch objects (that
> represent different architectures) can end up sharing the same pointer to
> arc_disassembler_options, so necessarily some of them will have the wrong
> value at a certain point.  But that doesn't look like an immediate problem,
> since you don't seem to recycle previously created gdbarch objects, like
> some other architectures do (does that mean that the gdbarch list keeps
> growing indefinitely if you connect multiple times, load new exec files?).  The
> gdbarch in use should always be the last that was created, so
> arc_disassembler_options should contain the good value for that gdbarch.  I
> was thinking that a situation like this could be problematic, if you were re-
> using gdbarch objects:
> 
> 1. You connect to a gdbserver that reports arch A.  A gdbarch is created and
> arc_disassembler_options is set for arch A.
> 2. You disconnect, and connect to a gdbserver that reports arch B.  A gdbarch
> is created and arc_disassembler_options is set for arch B.
> 3. You disconnect, and connect again to the first gdbserver.  This time, you
> would return the existing gdbarch for arch A, so the current gdbarch (arch A)
> would still use the disassembler options of arch B.
> 
> But like I said, that doesn't seem to be the case right now.
> 
> That makes me wonder why we pass a char ** and not simply an allocated
> char *, of which the gdbarch would take ownership, which would avoid this
> problem entirely.

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. 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?

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

> 
> Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]