From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33847 invoked by alias); 14 Jul 2017 14:56:32 -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 33827 invoked by uid 89); 14 Jul 2017 14:56:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Regard 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; Fri, 14 Jul 2017 14:56:30 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 78E69A2877; Fri, 14 Jul 2017 14:56:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 78E69A2877 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 78E69A2877 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id ADA3960618; Fri, 14 Jul 2017 14:56:28 +0000 (UTC) Subject: Re: [RFA 1/2] Fix two regressions in scalar printing To: Tom Tromey , gdb-patches@sourceware.org References: <20170713123400.28917-1-tom@tromey.com> <20170713123400.28917-2-tom@tromey.com> From: Pedro Alves Message-ID: <22c48f9e-ec2c-850d-91d3-c6a3ea8cdb11@redhat.com> Date: Fri, 14 Jul 2017 14:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170713123400.28917-2-tom@tromey.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-07/txt/msg00182.txt.bz2 On 07/13/2017 01:33 PM, Tom Tromey wrote: > PR gdb/21675 points out a few regressions in scalar printing. > > One type of regression is due to not carrying over the old handling of > floating point printing -- where a format like "/x" causes a floating > point number to first be cast to integer. While this behavior does not > seem very useful to me, apparently at least one person is testing for > it, and we did agree in the earlier thread to preserve this. So, this > patch extends this behavior to the 'd' and 'u' formats. I'm open to changing the behavior, if we can come up with something that is useful and generally makes more sense. The manual does say, for "/x": Regard the bits of the value as an integer, and print the integer in hexadecimal. and for "/f": Regard the bits of the value as a floating point number and print using typical floating point syntax. which seems to suggest the intention was to reinterpret the variable's raw contents as integer. However, I suspect this was written with the "x" command in mind though, where you're reinterpreting raw memory disregarding anything about the types of the objects that may exist on the examined memory (and also where you can also request a specific width). A question like "how to print a float type object in hexadecimal format" just doesn't really leave much doubt for the "x" command. There, it's clearly raw bits interpretation you want, so you can do things like: float global_f = 3.14f; (gdb) x /4xb &global_f 0x601038 : 0xc3 0xf5 0x48 0x40 (gdb) x /fw &global_f 0x601038 : 3.1400001 "print" is different because it's working with objects with size and type, and can print aggregates/structs with subobjects of different types, even. Playing devil's advocate, I could see some justification for the converting behavior if you look at /x /d /u as behaving like a printf %x/%d/%u format strings with an implicit cast. That view doesn't work with the current implementation of "%f" though, which reinterprets with print, instead of converting, though with such a view, that would be seen as a bug... What a mess. :-/ Meanwhile, it's good to avoid behavior changes until we have a clear plan. > The other regression is a longstanding bug in print_octal_chars: one of > the constants was wrong. This patch fixes the constant and adds static > asserts to help catch this sort of error. > > @@ -427,11 +429,14 @@ print_scalar_formatted (const gdb_byte *valaddr, struct type *type, > case 'o': > print_octal_chars (stream, valaddr, len, byte_order); > break; > + case 'd': > case 'u': > - print_decimal_chars (stream, valaddr, len, false, byte_order); > + { > + bool is_signed = options->format != 'u' || !TYPE_UNSIGNED (type); I think you meant "&&" instead of "||". ( Maybe checking against 'd' would be clearer: bool is_signed = options->format == 'd' && !TYPE_UNSIGNED (type); or even separate case switches. ) As is, you'll get this: (gdb) p /u -1 $1 = -1 instead of current master's output: (gdb) p /u -1 $1 = 4294967295 which doesn't look right. Also, I'm also not quite sure about the TYPE_UNSIGNED check. The manual says: @item d Print as integer in signed decimal. @item u Print as integer in unsigned decimal. And I see this with a gdb from before the recent print_scalar_formatted changes: (gdb) p /d (unsigned long long) -1 $1 = -1 while we see this with either current master, or your patch: (gdb) p /d (unsigned long long) -1 $1 = 18446744073709551615 which also doesn't look right to me. And here: (gdb) p /d (unsigned) -1 $2 = 4294967295 I'd expect "-1", but we don't get it with any gdb version (before original print_scalar_formatted changes, or current master, or your current patch), which also looks like a bug to me. I think this would all be fixed by simply having separate 'u'/'d' cases with fixed signness: case 'u': print_decimal_chars (stream, valaddr, len, false, byte_order); break; case 'd': print_decimal_chars (stream, valaddr, len, true, byte_order); break; > + print_decimal_chars (stream, valaddr, len, is_signed, byte_order); Thanks, Pedro Alves