From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30855 invoked by alias); 24 May 2016 20:36:44 -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 30324 invoked by uid 89); 24 May 2016 20:36:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=thoroughly, love X-HELO: mail-qk0-f170.google.com Received: from mail-qk0-f170.google.com (HELO mail-qk0-f170.google.com) (209.85.220.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 24 May 2016 20:36:33 +0000 Received: by mail-qk0-f170.google.com with SMTP id x7so20488519qkd.3 for ; Tue, 24 May 2016 13:36:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=r1fOHhiR+mK1eD0Z6a69tk8JIgmfWcpXYO0OiSKQTlU=; b=PSSKEBtvSpENyIdSgh/BsNh7Io0zwQt3DrRFa6SGO2wLHBCFExU8sC0wJbjxtv4FH8 7OdqSHBZSZzXXXOVIB5FQgpIqzkt1TvmLczv3b1/0A3wMSXDApbeOfv/u0q50FkybBND d44Npy4hUgt3fhXLWsWp8LJexqt+4waUACMjMOyWsHoDcLHo6QUWSqrZYB961KUUdIHt jbfCN+K5HY816juxwMF+JpCWjAZwG/BW934kFoihOfX21PY6GFbg3I1ScMnVKEy1VcF3 SVM1qVDojuswdSRwiPMzzandS2DcmmZSe9jHykOhrOSVeUYzSkc5LXLdZmdf9DSKMxuB oV9g== X-Gm-Message-State: ALyK8tLWbC7tK3RoWSmrp8A+DUd56Z8tQG2sUQguwxDQXI/EN9d1Hxs7ZhiPnweMd9Yjq6jl8Se9wQZcHAZ7UQGy X-Received: by 10.200.51.60 with SMTP id t57mr175920qta.26.1464122190990; Tue, 24 May 2016 13:36:30 -0700 (PDT) MIME-Version: 1.0 Received: by 10.55.16.211 with HTTP; Tue, 24 May 2016 13:35:51 -0700 (PDT) In-Reply-To: References: <1464019228-11131-1-git-send-email-martin.galvan@tallertechnologies.com> <04d07644-c6ed-88ae-f1de-cba46e875f2d@redhat.com> From: Martin Galvan Date: Tue, 24 May 2016 20:36:00 -0000 Message-ID: Subject: Re: [PATCH v2][PR gdb/19893] Fix handling of synthetic C++ references To: Pedro Alves Cc: gdb-patches Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2016-05/txt/msg00438.txt.bz2 I've just realized that my comment isn't entirely clear. The "@address" string is shown only for structure types, not for regular variables, arrays or such. It makes sense because only those are affected by set print object, but the comment doesn't explicitly say so. I don't know whether it'd be preferable to have everywhere as opposed to @address. I guess @address is more consistent with non-synthetic references, and it also hides the fact they're synthetic from the user, but there are cases (such as DW_AT_const_value) where @address wouldn't work/make sense. So I think before proceeding we should decide which output is better. Perhaps we could show @address whenever possible, and for the corner cases? On Tue, May 24, 2016 at 11:51 AM, Pedro Alves wrote: > But normal pointers don't print @address either, only references do. Yeah, I was referring to references :) > Not printing "@address" with "set print object off" seems like > hiding information from the user, information that we could show. > We always print it for non-synthetic references, AFAICS. Yes, we do. > Your comment in the patch, in generic_val_print_ref, reads: > > + if options->objectprint is true, c_value_print will call value_addr > + on the reference, which coerces synthetic references and returns a > + 'not_lval'. */ > > So if that works, I don't understand -- wouldn't calling value_addr > or coerce_ref in generic_val_print_ref if you have a synthetic > reference, or any reference even, be what you'd want? If I'm not mistaken, doing that would cause us to always print "@address". Which again, may be in fact the right thing to do. >> I *could*, however, manually call >> value->location.computed.funcs->check_synthetic_pointer in >> generic_val_print_ref instead of using value_bits_synthetic_pointer, >> thus avoiding the check for lval_computed. But that's a bit ugly IMHO. > > I don't understand this one. Only lval_computed values have a > "location.computed.funcs" to call. Yeah, you're right. For a moment I thought all values had an lval_funcs member, but only computed ones do. > So is the problem that this bit: > > if (options->addressprint) > { > CORE_ADDR addr > = extract_typed_address (valaddr + embedded_offset, type); > > doesn't work / doesn't make sense with synthetic pointers? Exactly. IIRC extract_typed_address would return zero (or maybe just garbage?), because valaddr would actually be a pointer to the synthetic pointer's lval_funcs instead of a target address. On a related note, it'd be great (for debugging at least!) if unions such as this had at least a discriminant of sorts. > Should we be calling value_addr instead? Perhaps. I stuck to calling extract_typed_address because I tried to change as little code as possible. I'm seeing that generic_val_print_ref has a path which coerces the reference anyway, so we could always coerce it, get its address, and only print the referenced value when required. I'd have to test it thoroughly to be sure, though. > Or are we perhaps missing a lval_funcs method? (Ideally, all > value properties/methods would go through a vtable like > lval_funcs; think "making struct value a proper C++ class" going > forward.) Right now I don't think so. The existing methods should be enough to handle these cases. Speaking of which, are there any plans to rewrite this sort of object-oriented code in C++? I'd love to take a shot at this in the future. > I don't know where, but I think this should indeed be covered by > tests somewhere. Ok.