From patchwork Mon Nov 14 12:51:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 17451 Received: (qmail 50984 invoked by alias); 14 Nov 2016 12:52:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 50965 invoked by uid 89); 14 Nov 2016 12:52:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=no version=3.3.2 spammy=fresh, WHOLE, doublest, resides X-HELO: mail-wm0-f66.google.com Received: from mail-wm0-f66.google.com (HELO mail-wm0-f66.google.com) (74.125.82.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Nov 2016 12:51:58 +0000 Received: by mail-wm0-f66.google.com with SMTP id a20so15112260wme.2 for ; Mon, 14 Nov 2016 04:51:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=bRSZrFs7MDCHH9pBZWPVTfOSNPe/nJuj407ynYvPgXM=; b=Ad2ef64JwNZkXMSBRIw9rjjbKD3TRg5+F7YP3OvdCo7XdZIOHnnX9P8Bq+YX6BV2SW CHunVt2nVkgOreg4Sj2rsH9OrG70EvWdRw3lMXWWl8e5E7+T17MKumAdjj+9kxYu1c5o c1/CeW7UG444M/MDt+GN+Db1texAdQR4WduZb4iMqk+vtHtDJ7DrVSZHIcWXRw/S+/vj jrCvKFj5+wPMw3q52au3fB1N+EkSsbOBWjWMLLJbMGS3b80s5aliLVWNy6a/GUirLW7C H+O20tK9xb8qgbO0ls3B+rp+CNG5AnEFR3GvpM03eD5hnwzWlB2zx+Z7w3bkA0Ctrzpg dCAw== X-Gm-Message-State: ABUngvc+DbHg2P6MxCuoXqxH2kGhgjLRDjzg6wl20qhKiLlvgGF+SpKr5s2WzXX8UYSVCw== X-Received: by 10.28.4.200 with SMTP id 191mr9659768wme.23.1479127916071; Mon, 14 Nov 2016 04:51:56 -0800 (PST) Received: from E107787-LIN (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id d65sm10227286wmh.11.2016.11.14.04.51.53 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 14 Nov 2016 04:51:55 -0800 (PST) Date: Mon, 14 Nov 2016 12:51:43 +0000 From: Yao Qi To: Ulrich Weigand Cc: "gdb-patches@sourceware.org" , Tom Tromey Subject: Re: set_value_component_location in apply_val_pretty_printer Message-ID: <20161114125143.GA22037@E107787-LIN> References: <20161028185834.0145C10B927@oc8523832656.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161028185834.0145C10B927@oc8523832656.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes On Fri, Oct 28, 2016 at 08:58:33PM +0200, Ulrich Weigand wrote: > Yao Qi wrote: > > > I don't understand this piece of code in apply_val_pretty_printer, > > why do we need to call set_value_component_location? > > > > set_value_component_location (value, val); > > /* set_value_component_location resets the address, so we may > > need to set it again. */ > > if (VALUE_LVAL (value) !=3D lval_internalvar > > && VALUE_LVAL (value) !=3D lval_internalvar_component > > && VALUE_LVAL (value) !=3D lval_computed) > > set_value_address (value, address + embedded_offset); > > > > It was added by Tom in > > https://sourceware.org/ml/gdb-patches/2010-06/msg00132.html > > There wasn't much information in email and ChangeLog. > > > It's reasonably simple to just create a new object of > the correct type and having the correct contents. However, > some of the value printers also want to use the value's > *location*. Just allocating a fresh object would have > no location information. That's why the code above > calls set_value_component_location to set the location > of the new value to the same as the location of the old. > > But this isn't really correct either, since we need the > location of the *subobject*. Now if the value resides > in inferior memory, we can get there simply by adding > the offset to the value address. But that's not actually > correct for values with other location types. I don't see why it is not correct to set the value's location to the same as the location of old. The 'whole' object and 'component' object should have the same location with different offsets (internalvar is an exception since the component's location is interanlvar_component), so set_value_component_location looks right to me. However, gdb{py,scm}_apply_val_pretty_printer are wrong to me that they use value_from_contents_and_address and set_value_address, like this, value = value_from_contents_and_address (type, valaddr + embedded_offset, address + embedded_offset); set_value_component_location (value, val); /* set_value_component_location resets the address, so we may need to set it again. */ if (VALUE_LVAL (value) != lval_internalvar && VALUE_LVAL (value) != lval_internalvar_component && VALUE_LVAL (value) != lval_computed) set_value_address (value, address + embedded_offset); because the 'whole' object may not be from memory, as you pointed out. I give a patch to this. > > I think we should either add an offset parameter to > set_value_component_location, and have it attempt to > do the best thing possible to describe the subobject > location, or maybe even provide a function that creates > the subobject directly in one go, along the lines of > value_primitive_field, except not based on struct > types but simply using an offset and subobject type. > value_primitive_field gives me some hints, and I use some in the patch. Regression tested on x86_64-linux. This patch fixes some asserts if I restrict value_has_address that only returns true for lval_memory and lval_register. value_has_address patch is here, https://sourceware.org/ml/gdb-patches/2016-10/msg00741.html diff --git a/gdb/guile/scm-pretty-print.c b/gdb/guile/scm-pretty-print.c index 5253def..648ca53 100644 --- a/gdb/guile/scm-pretty-print.c +++ b/gdb/guile/scm-pretty-print.c @@ -985,16 +985,7 @@ gdbscm_apply_val_pretty_printer (const struct extension_language_defn *extlang, cleanups = make_cleanup (null_cleanup, NULL); /* Instantiate the printer. */ - value = value_from_contents_and_address (type, valaddr + embedded_offset, - address + embedded_offset); - - set_value_component_location (value, val); - /* set_value_component_location resets the address, so we may - need to set it again. */ - if (VALUE_LVAL (value) != lval_internalvar - && VALUE_LVAL (value) != lval_internalvar_component - && VALUE_LVAL (value) != lval_computed) - set_value_address (value, address + embedded_offset); + value = value_from_component (val, type, embedded_offset); val_obj = vlscm_scm_from_value (value); if (gdbscm_is_exception (val_obj)) diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c index cbc168d..4f5e7f7 100644 --- a/gdb/python/py-prettyprint.c +++ b/gdb/python/py-prettyprint.c @@ -726,16 +726,7 @@ gdbpy_apply_val_pretty_printer (const struct extension_language_defn *extlang, cleanups = ensure_python_env (gdbarch, language); /* Instantiate the printer. */ - value = value_from_contents_and_address (type, valaddr + embedded_offset, - address + embedded_offset); - - set_value_component_location (value, val); - /* set_value_component_location resets the address, so we may - need to set it again. */ - if (VALUE_LVAL (value) != lval_internalvar - && VALUE_LVAL (value) != lval_internalvar_component - && VALUE_LVAL (value) != lval_computed) - set_value_address (value, address + embedded_offset); + value = value_from_component (val, type, embedded_offset); val_obj = value_to_value_object (value); if (! val_obj) diff --git a/gdb/value.c b/gdb/value.c index 9def1b3..1bb61f1 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -3804,6 +3804,31 @@ value_from_history_ref (const char *h, const char **endp) return access_value_history (index); } +/* Get the component value (offset by OFFSET bytes) of a struct or + union WHOLE. Component's type is TYPE. */ + +struct value * +value_from_component (struct value *whole, struct type *type, LONGEST offset) +{ + struct value *v; + + if (value_lazy (whole)) + v = allocate_value_lazy (type); + else + { + v = allocate_value (type); + value_contents_copy_raw (v, value_embedded_offset (v), + whole, value_embedded_offset (whole) + offset, + type_length_units (type)); + } + v->offset = value_offset (whole) + offset + value_embedded_offset (whole); + set_value_component_location (v, whole); + VALUE_REGNUM (v) = VALUE_REGNUM (whole); + VALUE_FRAME_ID (v) = VALUE_FRAME_ID (whole); + + return v; +} + struct value * coerce_ref_if_computed (const struct value *arg) { diff --git a/gdb/value.h b/gdb/value.h index f962508..c36eb6c 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -637,6 +637,8 @@ extern struct value *value_from_double (struct type *type, DOUBLEST num); extern struct value *value_from_decfloat (struct type *type, const gdb_byte *decbytes); extern struct value *value_from_history_ref (const char *, const char **); +extern struct value *value_from_component (struct value *, struct type *, + LONGEST); extern struct value *value_at (struct type *type, CORE_ADDR addr); extern struct value *value_at_lazy (struct type *type, CORE_ADDR addr);