From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97196 invoked by alias); 15 Dec 2017 17:24:00 -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 97185 invoked by uid 89); 15 Dec 2017 17:24:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_1,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= 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, 15 Dec 2017 17:23:58 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 011FA91FC0; Fri, 15 Dec 2017 17:23:57 +0000 (UTC) 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 0B5677C3AC; Fri, 15 Dec 2017 17:23:48 +0000 (UTC) Subject: Re: [PATCH v7 2/2] Implement pahole-like 'ptype /o' option To: Sergio Durigan Junior , GDB Patches References: <20171121160709.23248-1-sergiodj@redhat.com> <20171215011247.7396-1-sergiodj@redhat.com> <20171215011247.7396-3-sergiodj@redhat.com> Cc: Tom Tromey , Eli Zaretskii , Simon Marchi , Keith Seitz From: Pedro Alves Message-ID: Date: Fri, 15 Dec 2017 17:24: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: <20171215011247.7396-3-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00385.txt.bz2 Hi Sergio, I found a couple more nits to pick, but this is close. This is OK with the issues below addressed. Please address them, and push this in. Please post the end result for the archives. On 12/15/2017 01:12 AM, Sergio Durigan Junior wrote: > @@ -867,15 +913,121 @@ output_access_specifier (struct ui_file *stream, > if (last_access != s_public) > { > last_access = s_public; > - fprintfi_filtered (level + 2, stream, > - "public:\n"); > + print_spaces_filtered_with_print_options (level + 2, stream, flags); > + fprintf_filtered (stream, "public:\n"); > } > } > > return last_access; > } > > -/* Return true is an access label (i.e., "public:", "private:", > +/* Print information about field at index FIELD_IDX of the union type > + TYPE. Since union fields don't have the concept of offsets, we > + just print their sizes. > + > + The output is strongly based on pahole(1). */ > + > +static void > +c_print_type_union_field_offset (struct type *type, unsigned int field_idx, > + struct ui_file *stream) > +{ > + struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx)); > + > + fprintf_filtered (stream, "/* %4u */", TYPE_LENGTH (ftype)); > +} > + > +/* Print information about field at index FIELD_IDX of the struct type > + TYPE. > + > + END_BITPOS is the one-past-the-end bit position of the previous > + field (where we expect this field to be if there is no hole). At > + the end, ENDPOS is updated to the one-past-the-end bit position of > + the current field. There's still a reference to ENDPOS above that I assume should be END_BITPOS. Now that I'm looking at this, do we still need this parameter, given print_offset_data->end_bitpos exists? What's the relationship between the two? It's a bit confusing to have two things for the same. I think the comment could/should clarify this. > + > + OFFSET_BITPOS is the offset value we carry over when we are > + printing a struct that is inside another struct; this is useful so > + that the offset is constantly incremented (if we didn't carry it > + over, the offset would be reset to zero when printing the inner > + struct). This parameter no longer exists (it's a field of print_offset_data now, right?). The comment should be adjusted, moved elsewhere, or deleted. > + > + The output is strongly based on pahole(1). */ > + > +static void > +c_print_type_struct_field_offset (struct type *type, unsigned int field_idx, > + struct ui_file *stream, > + unsigned int *end_bitpos, > + struct print_offset_data *podata) > +{ > @@ -1143,9 +1347,16 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream, > && !is_full_physname_constructor /* " " */ > && !is_type_conversion_operator (type, i, j)) > { > - c_print_type (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)), > - "", stream, -1, 0, > - &local_flags); > + unsigned int old_po = local_flags.print_offsets; > + > + /* Temporarily disable print_offsets, because it > + would mess with indentation. */ > + local_flags.print_offsets = 0; > + c_print_type_1 (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)), > + "", stream, -1, 0, > + &local_flags, podata); > + local_flags.print_offsets = old_po; This pattern appears in several places. Would it make sense to add a c_print_type_no_offsets routine that handled the print_offsets save/restore? > +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp > @@ -0,0 +1,317 @@ > +# This testcase is part of GDB, the GNU debugger. > + > +# Copyright 2017 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# This testcase exercises the "ptype /o" feature, which can be used to > +# print the offsets and sizes of each field of a struct/union/class. > + > +standard_testfile .cc > + > +# Test only works on LP64 targets. That's how we guarantee that the > +# expected holes will be present in the struct. > +if { ![is_lp64_target] } { > + untested "test work only on lp64 targets" > + return 0 > +} > + > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \ > + { debug c++ }] } { > + return -1 > +} > + > +# Test general offset printing, ctor/dtor printing, union, formatting. > +gdb_test "ptype /o struct abc" \ > + [multi_line \ ... > +{ \}}] \ > + "ptype offset struct abc" I notice that you're replacing the "/o" in most test names/messages. Is there a reason for that? Why not just let the test name default to the command invocation? I.e., remove the 3rd argument in all these calls to gdb_test. > + > +# Test "ptype /oTM". > +gdb_test "ptype /oTM struct abc" \ > + [multi_line \ .. > + "ptype /oTM struct abc" Here the "/" persisted, so I'm curious why the other cases have "/o" replaced by "offset". I'd just remove the explicit test messages. It just seems like potential for getting out of sync otherwise. > +# Because dealing with bitfields and offsets is difficult, it can be > +# tricky to confirm that the output of the this command is accurate. typo: "of the this" > @@ -499,6 +514,11 @@ whatis_exp (const char *exp, int show) > real_type = value_rtti_type (val, &full, &top, &using_enc); > } > > + if (flags.print_offsets > + && (TYPE_CODE (type) == TYPE_CODE_STRUCT > + || TYPE_CODE (type) == TYPE_CODE_UNION)) > + fprintf_filtered (gdb_stdout, "/* offset | size */ "); I noticed that "whatis" also prints this header: (top-gdb) whatis/o minimal_symbol /* offset | size */ type = minimal_symbol I guess it shouldn't, for it's pointless. You could either use the "show" parameter here, or just ignore /o for whatis where the options are parsed, further above. And then please add a test covering that. :-) You'll have to use gdb_test_multiple with an anchor, because gdb_test "whatis/o minimal_symbol" \ "type = minimal_symbol" would match the bad output anyway. So something like: set test "whatis /o minimal_symbol" gdb_test_multiple $test $test { -ex "^$test\r\ntype = minimal_symbol\r\n$gdb_prompt $" { pass $test } As mentioned, OK with these issues addressed. Please push and post. Thanks, Pedro Alves