From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 123433 invoked by alias); 27 Oct 2016 12:39:30 -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 123416 invoked by uid 89); 27 Oct 2016 12:39:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL,BAYES_00,MIME_BASE64_BLANKS,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=peer, letter, Tel, tel X-HELO: mga09.intel.com Received: from mga09.intel.com (HELO mga09.intel.com) (134.134.136.24) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Oct 2016 12:39:19 +0000 Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP; 27 Oct 2016 05:39:17 -0700 X-ExtLoop1: 1 Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga003.jf.intel.com with ESMTP; 27 Oct 2016 05:39:17 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.49]) by IRSMSX107.ger.corp.intel.com ([169.254.10.56]) with mapi id 14.03.0248.002; Thu, 27 Oct 2016 13:39:15 +0100 From: "Metzger, Markus T" To: Yao Qi CC: "gdb-patches@sourceware.org" , "palves@redhat.com" Subject: RE: [PATCH 0/5] improve trace gap handling Date: Thu, 27 Oct 2016 12:39:00 -0000 Message-ID: References: <1469175120-19657-1-git-send-email-markus.t.metzger@intel.com> <8637ji83dd.fsf@gmail.com> In-Reply-To: <8637ji83dd.fsf@gmail.com> Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 X-IsSubscribed: yes X-SW-Source: 2016-10/txt/msg00747.txt.bz2 PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBZYW8gUWkg W21haWx0bzpxaXlhb2x0Y0BnbWFpbC5jb21dDQo+IFNlbnQ6IFRodXJzZGF5 LCBPY3RvYmVyIDI3LCAyMDE2IDEyOjU5IFBNDQo+IFRvOiBNZXR6Z2VyLCBN YXJrdXMgVCA8bWFya3VzLnQubWV0emdlckBpbnRlbC5jb20+DQo+IENjOiBn ZGItcGF0Y2hlc0Bzb3VyY2V3YXJlLm9yZzsgcGFsdmVzQHJlZGhhdC5jb20N Cj4gU3ViamVjdDogUmU6IFtQQVRDSCAwLzVdIGltcHJvdmUgdHJhY2UgZ2Fw IGhhbmRsaW5nDQoNCkhlbGxvIFlhbywNCg0KPiA+IEEgc21hbGwgcGF0Y2gg c2VyaWVzIHRoYXQgaW1wcm92ZXMgZGVhbGluZyB3aXRoIGdhcHMgaW4gcmVj b3JkZWQgdHJhY2UuDQo+IA0KPiBJIHRoaW5rIHlvdSBjYW4gc2VsZiBhcHBy b3ZlIHRoZXNlIGJ0cmFjZSBwYXRjaGVzLCB1bmxlc3MgdGhpcyBwYXRjaA0K PiBzZXJpZXMgYWRkcmVzcyBQZWRybyBvciBvdGhlciBwZW9wbGUncyByZXZp ZXcgY29tbWVudHMuICBUaGUgY292ZXINCj4gbGV0dGVyIGlzIHRvbyBzaW1w bGUgdG8gbGluayB0aGlzIHBhdGNoIHNlcmllcyB0byBwcmV2aW91cyByZXZp ZXdzLg0KDQpUaGUgc2VyaWVzIGhhcyBub3QgYmVlbiByZXZpZXdlZCwgeWV0 LiAgSXQgZG9lcyBub3QgcmVmZXIgdG8gYW55IHByZXZpb3VzDQpyZXZpZXcg Y29tbWVudC4NCg0KRG9uJ3Qgd2Ugd2FudCBwYXRjaGVzIHRvIGJlIHBlZXIg cmV2aWV3ZWQgaW4gZ2VuZXJhbD8gIE9yIGFyZSB5b3UNCnNheWluZyB0aGF0 IEkgY2FuIGFuZCBzaG91bGQgbWFrZSBjaGFuZ2VzIHRvIHJlY29yZC1idHJh Y2Ugd2l0aG91dA0KcmV2aWV3Pw0KDQp0aGFua3MsDQptYXJrdXMuDQoNCklu dGVsIERldXRzY2hsYW5kIEdtYkgKUmVnaXN0ZXJlZCBBZGRyZXNzOiBBbSBD YW1wZW9uIDEwLTEyLCA4NTU3OSBOZXViaWJlcmcsIEdlcm1hbnkKVGVsOiAr NDkgODkgOTkgODg1My0wLCB3d3cuaW50ZWwuZGUKTWFuYWdpbmcgRGlyZWN0 b3JzOiBDaHJpc3RpbiBFaXNlbnNjaG1pZCwgQ2hyaXN0aWFuIExhbXByZWNo dGVyCkNoYWlycGVyc29uIG9mIHRoZSBTdXBlcnZpc29yeSBCb2FyZDogTmlj b2xlIExhdQpSZWdpc3RlcmVkIE9mZmljZTogTXVuaWNoCkNvbW1lcmNpYWwg UmVnaXN0ZXI6IEFtdHNnZXJpY2h0IE11ZW5jaGVuIEhSQiAxODY5MjgK >From gdb-patches-return-134337-listarch-gdb-patches=sources.redhat.com@sourceware.org Thu Oct 27 13:03:49 2016 Return-Path: Delivered-To: listarch-gdb-patches@sources.redhat.com Received: (qmail 96040 invoked by alias); 27 Oct 2016 13:03:49 -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 Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 96025 invoked by uid 89); 27 Oct 2016 13:03:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=simonmarchipolymtlca, simon.marchi@polymtl.ca, Closure X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Oct 2016 13:03:37 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C440B23602C; Thu, 27 Oct 2016 13:03:36 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9RD3Zc3029564; Thu, 27 Oct 2016 09:03:35 -0400 Subject: Re: [PATCH] New function value_has_address To: Yao Qi , Simon Marchi References: <1477557571-17122-1-git-send-email-yao.qi@linaro.org> <329976bc32481db3faff9288f777431d@polymtl.ca> Cc: "gdb-patches@sourceware.org" From: Pedro Alves Message-ID: <94da1a10-ebad-b112-2d45-a8f8a52fa8b5@redhat.com> Date: Thu, 27 Oct 2016 13:03:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00748.txt.bz2 Content-length: 3430 On 10/27/2016 01:12 PM, Yao Qi wrote: > On Thu, Oct 27, 2016 at 12:29 PM, Simon Marchi wrote: >> That help clarifies the code, I like it. >> >>> diff --git a/gdb/value.c b/gdb/value.c >>> index b825aec..4b5cbde 100644 >>> --- a/gdb/value.c >>> +++ b/gdb/value.c >>> @@ -1538,12 +1538,20 @@ value_lval_const (const struct value *value) >>> return value->lval; >>> } >>> >>> +/* Return true if VALUE has address, otherwise return false. */ >> >> >> This comment is really obvious, it's really just stating the function name. >> Could you expand it a bit, perhaps saying what it means for a value to "have >> address"? I suppose it means that it has a memory address on the target? > > It is intended to avoid expanding the comments because I can't give a > clear meaning, like "has a memory address on the target", unless I fully > understand "value" in gdb. That is why I only make value_has_address > static. > > I find some inconsistencies across the code, > > /* The only lval kinds which do not live in target memory. */ > if (VALUE_LVAL (val) != not_lval > && VALUE_LVAL (val) != lval_internalvar > && VALUE_LVAL (val) != lval_xcallable) > return 0; > > lval_internalvar_component is not checked, > > /* 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); > > lval_xcallable is not checked, but lval_computed is checked. Before > we clearly document the meaning of value_has_address, we need to > figure out these things above first. > I think the answer is here: /* Location of value (if lval). */ union { /* If lval == lval_memory, this is the address in the inferior. If lval == lval_register, this is the byte offset into the registers structure. */ CORE_ADDR address; /* Pointer to internal variable. */ struct internalvar *internalvar; /* Pointer to xmethod worker. */ struct xmethod_worker *xm_worker; /* If lval == lval_computed, this is a set of function pointers to use to access and describe the value, and a closure pointer for them to use. */ struct { /* Functions to call. */ const struct lval_funcs *funcs; /* Closure for those functions to use. */ void *closure; } computed; } location; That's a union. We should only ever access the "address" on lval types that use the "address" field above. Note that we call value_address on lval_computed values and that incorrectly returns "value->location.address + value->offset", which is completely bogus. (Try running to main, and then doing p $_siginfo). The value printing routines want to pass around a value address, but since we actually pass the value pointer around nowadays too, I think that parameter could be eliminated, and the result is likely to be that value_address ends up called is much fewer places where it doesn't really make sense. IMO, struct value and its lval types would be nice candidates for converting to a class hierarchy, with lval_funcs expanded into virtual methods in struct value directly, that are then used by all value lval types, including lval_memory/lval_register. Thanks, Pedro Alves