From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26780 invoked by alias); 22 Apr 2010 22:30:54 -0000 Received: (qmail 26769 invoked by uid 22791); 22 Apr 2010 22:30:52 -0000 X-SWARE-Spam-Status: No, hits=-4.9 required=5.0 tests=BAYES_20,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Apr 2010 22:30:44 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o3MMUY0O032340 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 22 Apr 2010 18:30:34 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o3MMUO3t003662 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 22 Apr 2010 18:30:26 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o3MMUO2c016728; Fri, 23 Apr 2010 00:30:24 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o3MMUNNe016727; Fri, 23 Apr 2010 00:30:23 +0200 Date: Thu, 22 Apr 2010 22:30:00 -0000 From: Jan Kratochvil To: Pedro Alves Cc: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [patch] Fix dangling displays in separate debug Message-ID: <20100422223023.GA7062@host0.dyn.jankratochvil.net> References: <20100403095558.GA22520@host0.dyn.jankratochvil.net> <201004110227.35104.pedro@codesourcery.com> <20100419142503.GA23056@host0.dyn.jankratochvil.net> <201004201205.30049.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201004201205.30049.pedro@codesourcery.com> User-Agent: Mutt/1.5.20 (2009-08-17) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-04/txt/msg00769.txt.bz2 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