This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Fix dangling displays in separate debug
On Tue, 20 Apr 2010 13:05:30 +0200, Pedro Alves wrote:
> On Monday 19 April 2010 15:25:03, Jan Kratochvil wrote:
> > With the extended requirements I rather pushed the former patch posted at:
> > Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
> > http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html
>
> Yikes. I don't understand what is the extended requirement, but anyway,
As you have requested to support both separate and embedded debug info forms
I "had to" include there the general stripping framework (written formerly for
break-interp.exp). This meant I also "had to" keep there the "NO" case which
uses only ELF symbols. ELF symbols opened a can of worms of more issues in
incomplete display_uses_solib_p which I was aware of from the patch a year ago
above.
OTOH hopefully the more complete fix gets in now that way.
> > +/* Helper for exp_uses_objfile. */
> > +
> > +static int
> > +exp_uses_objfile_iter (struct objfile *exp_objfile, void *objfile_voidp)
> > +{
> > + struct objfile *objfile = objfile_voidp;
> > +
> > + if (exp_objfile->separate_debug_objfile_backlink)
> > + exp_objfile = exp_objfile->separate_debug_objfile_backlink;
> > +
> > + return exp_objfile == objfile;
> > +}
>
> Most things look good. This is get's a little blury though. You have
> this nice iterator that can determine if a given expression involved an
> obfile, so, you could accurately now disable a "display" when an objfile
> is unloaded from gdb --- _any_ objfile, even in principly when you
> do "file" to unload the main exec's symbols, say, so that _all_ cases
> of dangling displays would be cured.
Yes, the OBJFILE based iterator was originally written with this intention.
> Still, you only call this when an solib is unloaded.
...
> Doesn't this just
> beg for a new `objfile_unloaded' observer (called from, say free_objfile)?
That patch
> > Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
> > http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html
was based on more general objfile callback rather than the current solib
callback, using the new observer from:
[patch 1/8] Types GC [unloading observer]
http://sourceware.org/ml/gdb-patches/2009-05/msg00544.html
But the patch series did not make it in for some reasons which may be
only/also on my side. So trying to get it at least a part of the series now.
It can be incrementally completed later without much additional work.
Probability of getting a whole patch series in equals p^n where p is
probability for a single patch and n is the number of patches in the series.
For low p and high n it does not work well. :-)
> Can't we in principle be unloading a separate debug info objfile, but still
> have the main one loaded, hence OBJFILE_VOIDP could be a separate debug
> objfile?
I find the internal GDB API standardizes on the normalized base objfile (that
is following the separate_debug_objfile_backlink pointer if there is one).
I have seen this as a common practice at least in:
lookup_objfile_from_block dwarf2_per_cu_objfile elf_lookup_lib_symbol
etc.
Therefore OBJFILE_VOIDP cannot be the separate debug objfile as the value has
been verified as normalized via separate_debug_objfile_backlink already by
exp_uses_objfile and it was previously normalized via
separate_debug_objfile_backlink by clear_dangling_display_expressions.
It is already stated in the exp_uses_objfile comment.
> An expression could in principle be using symbols from either,
Yes, therefore exp_uses_objfile_iter normalizes EXP_OBJFILE.
> Anyway, I'm not going to require you do that for this patch, your patch
> looks good already. Just dumping my thoughts, on what looks like an
> opportunity to keep the solib/objfile concepts cleanly separate, and get
> rid of all the objfile backlinking here.
That's true but I can remove the backlinking when the objfile unloading
observer gets checked in one day.
> > - if (d->block != NULL
> > - && d->pspace == solib->pspace
> > - && solib_contains_address_p (solib, d->block->startaddr))
> > - return 1;
>
> Is there any `solib_contains_address_p' check left after the patch?
> What about displays that don't involve symbols, like:
>
> `display *0xaddr_in_solib'.
>
> They'll probably still disable themselves when memory reading
> fails, but, will we now see an ugly error, or something?
It is unrelated. d->block specifies scope of the expression. Absolute
address dereference requires no scope and thus GDB sets d->block = NULL.
GDB tries to never use this INNERMOST_BLOCK if it does not need to.
It still prints:
Disabling display 1 to avoid infinite recursion.
1: *0x7ffff7ddd53c = Cannot access memory at address 0x7ffff7ddd53c
d->block is the function f for `display x' in:
void f (void)
{
int x;
}
In that case I cannot imagine when solib_contains_address_p could return for f
TRUE while lookup_objfile_from_block would not match BLOCK with OBJFILE.
After/if one day Apple's "watch -location" gets accepted there may be a problem
watch -location global_var_in_solib
may not catch unloading of the library as it will have neither BLOCK nor EXP
binding to that OBJFILE. But "-location" is not present in current FSF GDB.
Apple "watch -location" should do something like:
p/x &arg
watch *(typeof (arg)) printed_address
> > -if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
> > - || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
> > - untested "Could not compile $binfile_lib or $binfile."
> > - return -1
> > +set save_pf_prefix $pf_prefix
> > +# SEP must be last for the possible `unsupported' error path.
> > +foreach libsepdebug {NO IN SEP} {
>
> Could you please add $libsepdebug or something of the sorts
> to each test within the loop, so that when we analyise the logs we
> can distinguish FAIL/PASSes for each iteration?
It already prints unique test names using $pf_prefix there:
Running ./gdb.base/solib-display.exp ...
PASS: gdb.base/solib-display.exp: NO: display a_global
PASS: gdb.base/solib-display.exp: NO: display b_global
PASS: gdb.base/solib-display.exp: NO: display c_global
PASS: gdb.base/solib-display.exp: NO: after rerun
PASS: gdb.base/solib-display.exp: NO: after rerun (2)
PASS: gdb.base/solib-display.exp: NO: break 25
PASS: gdb.base/solib-display.exp: NO: continue
PASS: gdb.base/solib-display.exp: NO: display main_global
PASS: gdb.base/solib-display.exp: NO: display a_local
PASS: gdb.base/solib-display.exp: NO: display a_static
PASS: gdb.base/solib-display.exp: NO: break 25
PASS: gdb.base/solib-display.exp: NO: continue
PASS: gdb.base/solib-display.exp: IN: display a_global
...
PASS: gdb.base/solib-display.exp: IN: continue
PASS: gdb.base/solib-display.exp: SEP: split solib
PASS: gdb.base/solib-display.exp: SEP: display a_global
...
PASS: gdb.base/solib-display.exp: SEP: continue
I hope it is OK this way and you just missed the pf_prefix updates there.
> Otherwise, looks good to me.
Please advice if the comments are OK or the patch needs some updates.
Thanks,
Jan