From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9866 invoked by alias); 4 Jul 2013 16:41:01 -0000 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 Received: (qmail 9857 invoked by uid 89); 4 Jul 2013 16:41:00 -0000 X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,TW_XZ autolearn=ham version=3.3.1 Received: from mms3.broadcom.com (HELO mms3.broadcom.com) (216.31.210.19) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 04 Jul 2013 16:40:58 +0000 Received: from [10.9.208.57] by mms3.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.5)); Thu, 04 Jul 2013 09:31:22 -0700 X-Server-Uuid: B86B6450-0931-4310-942E-F00ED04CA7AF Received: from IRVEXCHSMTP2.corp.ad.broadcom.com (10.9.207.52) by IRVEXCHCAS08.corp.ad.broadcom.com (10.9.208.57) with Microsoft SMTP Server (TLS) id 14.1.438.0; Thu, 4 Jul 2013 09:40:46 -0700 Received: from mail-irva-13.broadcom.com (10.10.10.20) by IRVEXCHSMTP2.corp.ad.broadcom.com (10.9.207.52) with Microsoft SMTP Server id 14.1.438.0; Thu, 4 Jul 2013 09:40:45 -0700 Received: from [10.177.73.66] (unknown [10.177.73.66]) by mail-irva-13.broadcom.com (Postfix) with ESMTP id F20AAF2D72; Thu, 4 Jul 2013 09:40:44 -0700 (PDT) Message-ID: <51D5A58C.8040305@broadcom.com> Date: Thu, 04 Jul 2013 16:41:00 -0000 From: "Andrew Burgess" User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 MIME-Version: 1.0 To: "gdb-patches@sourceware.org" cc: "Pedro Alves" Subject: Re: [3/3] [PATCH] value_optimized_out and value_fetch_lazy References: <51B5A95F.7090400@broadcom.com> <51C1D347.3020906@redhat.com> <51D1C52A.1070603@broadcom.com> <51D4822B.2010504@redhat.com> In-Reply-To: <51D4822B.2010504@redhat.com> Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00175.txt.bz2 On 03/07/2013 8:57 PM, Pedro Alves wrote: > On 07/01/2013 07:06 PM, Andrew Burgess wrote: >> On 19/06/2013 4:50 PM, Pedro Alves wrote: >>> On 06/10/2013 11:24 AM, Andrew Burgess wrote: >>> >>>> I ran into an issue with gdb that appears to be caused by an incorrect >>>> use of value_optimized_out. >>>> >>> >>> I'm finding the patch a bit hard to read though. Could you >>> split it up? >> >> This tiny patch notices that when we mark a value as optimized out >> we can also mark the value as no longer lazy. >> This patch is not required, but felt like a good thing to me, not >> sure if everyone will agree though. >> >> Should I apply? >> >> Thanks >> Andrew >> >> >> >> >> gdb/ChangeLog >> >> 2013-07-01 Andrew Burgess >> >> * value.c (set_value_optimized_out): A value that is optimized out >> is no longer lazy. > > Hmm. > > I tried going a step further. If the value is optimized > out, then we don't really need its contents buffer. > > That is: > > gdb/value.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/gdb/value.c b/gdb/value.c > index 970fb66..50f8889 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -690,6 +690,8 @@ allocate_value_lazy (struct type *type) > void > allocate_value_contents (struct value *val) > { > + gdb_assert (!val->optimized_out); > + > if (!val->contents) > val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type)); > } > @@ -1064,6 +1066,13 @@ void > set_value_optimized_out (struct value *value, int val) > { > value->optimized_out = val; > + > + if (val) > + { > + set_value_lazy (value, 0); > + xfree (value->contents); > + value->contents = NULL; > + } > } > > and then ran the testsuite. Sure enough, that catches a gdb crash. > In gdb.dwarf2/dw2-op-out-param.exp: > > #0 0x000000000056bed0 in extract_signed_integer (addr=0x0, len=8, byte_order=BFD_ENDIAN_LITTLE) at ../../src/gdb/findvar.c:78 > #1 0x000000000057e761 in unpack_long (type=0x1793f80, valaddr=0x0) at ../../src/gdb/value.c:2453 > #2 0x000000000059baae in print_scalar_formatted (valaddr=0x0, type=0x1793f80, options=0x7fffdb2376c0, size=0, stream=0x1929a00) at ../../src/gdb/printcmd.c:404 > #3 0x000000000059763b in val_print_scalar_formatted (type=0x1793f80, valaddr=0x0, embedded_offset=0, val=0x19dfcd0, options=0x7fffdb2376c0, size=0, stream=0x1929a00) > at ../../src/gdb/valprint.c:970 > #4 0x00000000006d1f2f in c_val_print (type=0x1793f80, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=3, original_value=0x19dfcd0, options= > 0x7fffdb237860) at ../../src/gdb/c-valprint.c:407 > #5 0x0000000000596f4a in val_print (type=0x1793f80, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=3, val=0x19dfcd0, options=0x7fffdb237940, > language=0x8d34a0) at ../../src/gdb/valprint.c:778 > #6 0x00000000006d3491 in cp_print_value_fields (type=0x1796110, real_type=0x1796110, valaddr=0x0, offset=0, address=9225056, stream=0x1929a00, recurse=2, val=0x19dfcd0, > options=0x7fffdb237d40, dont_print_vb=0x0, dont_print_statmem=0) at ../../src/gdb/cp-valprint.c:363 > #7 0x00000000006d37ef in cp_print_value_fields_rtti (type=0x1796110, valaddr=0x0, offset=0, address=9225056, stream=0x1929a00, recurse=2, val=0x19dfcd0, options= > 0x7fffdb237d40, dont_print_vb=0x0, dont_print_statmem=0) at ../../src/gdb/cp-valprint.c:458 > #8 0x00000000006d1e45 in c_val_print (type=0x1796110, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=2, original_value=0x19dfcd0, options= > 0x7fffdb237d40) at ../../src/gdb/c-valprint.c:393 > #9 0x0000000000596f4a in val_print (type=0x1796110, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=2, val=0x19dfcd0, options=0x7fffdb237e60, > language=0x8d34a0) at ../../src/gdb/valprint.c:778 > #10 0x0000000000597157 in common_val_print (val=0x19dfcd0, stream=0x1929a00, recurse=2, options=0x7fffdb237e60, language=0x8d34a0) at ../../src/gdb/valprint.c:840 > #11 0x00000000005d9ea6 in print_frame_arg (arg=0x7fffdb237f30) at ../../src/gdb/stack.c:284 > #12 0x00000000005daa49 in print_frame_args (func=0x1796af0, frame=0x1b5b800, num=-1, stream=0x168fd10) at ../../src/gdb/stack.c:645 > #13 0x00000000005db9df in print_frame (frame=0x1b5b800, print_level=1, print_what=LOCATION, print_args=1, sal=...) at ../../src/gdb/stack.c:1172 > #14 0x00000000005daf95 in print_frame_info (frame=0x1b5b800, print_level=1, print_what=LOCATION, print_args=1) at ../../src/gdb/stack.c:824 > #15 0x00000000005dcdc5 in backtrace_command_1 (count_exp=0x0, show_locals=0, no_filters=0, from_tty=1) at ../../src/gdb/stack.c:1763 > #16 0x00000000005dd1b6 in backtrace_command (arg=0x0, from_tty=1) at ../../src/gdb/stack.c:1860 > #17 0x00000000004dc53f in do_cfunc (c=0x151bec0, args=0x0, from_tty=1) at ../../src/gdb/cli/cli-decode.c:113 > #18 0x00000000004df5d4 in cmd_func (cmd=0x151bec0, args=0x0, from_tty=1) at ../../src/gdb/cli/cli-decode.c:1888 > #19 0x00000000006e43d1 in execute_command (p=0x14623a2 "", from_tty=1) at ../../src/gdb/top.c:478 > > Odd that this bit of val_print_scalar_formatted: > > /* A scalar object that does not have all bits available can't be > printed, because all bits contribute to its representation. */ > if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset, > TARGET_CHAR_BIT * TYPE_LENGTH (type))) > val_print_optimized_out (stream); > > didn't catch this. I then added this further. > > --- c/gdb/value.c > +++ w/gdb/value.c > @@ -1078,6 +1078,8 @@ set_value_optimized_out (struct value *value, int val) > int > value_entirely_optimized_out (const struct value *value) > { > + gdb_assert (value->optimized_out || !value->lazy); > + > if (!value->optimized_out) > return 0; > if (value->lval != lval_computed > @@ -1089,6 +1091,8 @@ value_entirely_optimized_out (const struct value *value) > int > value_bits_valid (const struct value *value, int offset, int length) > { > + gdb_assert (value->optimized_out || !value->lazy); > + > if (!value->optimized_out) > return 1; > if (value->lval != lval_computed > > And I see: > > Breakpoint 2, 0x000000000040058f in breakpt () > (gdb) PASS: gdb.dwarf2/dw2-op-out-param.exp: continue to breakpoint: Stop in breakpt for struct_param_two_reg_pieces > bt > #0 0x000000000040058f in breakpt () > #1 0x00000000004005c2 in struct_param_two_reg_pieces (operand0=../../src/gdb/value.c:1081: internal-error: value_entirely_optimized_out: Assertion `value->optimized_out || !value->lazy' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) FAIL: gdb.dwarf2/dw2-op-out-param.exp: Backtrace for test struct_param_two_reg_pieces (GDB internal error) > Resyncing due to internal error. > > ... and then I ran out of time. Gotta run an errand. :-) > The issue here is that optimized_out does not mean "the value is optimized out", but instead (in the general case) means this value is at least partially optimized out. The problem case are computed values, where if part of the value is missing then the optimized_out flag is set, but we still rely on being able to access the contents of the value, with the result that your patch above, freeing the contents, causes problems. I don't include the patch here, but as an experiment I made a quick hack where all the calls to set_value_optimized_out inside dwarf2loc.c call a new method set_value_partially_optimized_out, this new method sets the optimized_out flag but does not free the contents buffer, all the tests now pass. I know this is a hack, we create computed values in other situations, but the case of computed value and partially optimized out values only appear to be tested for the dwarf case. So... I wonder, there appears to be some overlap between the value unavailable data and the value optimized_out flag. I believe I understand the difference: * unavailable - data was not collected at a tracepoint. * optimized_out - insufficient debug information, or the debug info specifically states the value is gone. However, at a higher level there does seem to be some overlap. Imagine if we collapsed together the unavailable and optimized_out properties of a value, when handling a value gdb would then walk the following path: is-value-available? ----- yes -----> process-as-normal | '-- no --- > why-not? --- optimized_out ---> opt-out-case | '-------- unavailable ---> unavailable-case The benefit however would be all the code we currently have for propagating unavailable / optimized_out status between values would be unified, we would gain the ability to support partially optimized_out values, and from a quick look at the code I believe there are a couple of places where we handle optimized_out but not unavailable, this would be fixed. What do people think? Cheers, Andrew