From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 52039 invoked by alias); 13 Dec 2017 16:20:06 -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 51864 invoked by uid 89); 13 Dec 2017 16:20:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=BAYES_00,KAM_ASCII_DIVIDERS,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no 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; Wed, 13 Dec 2017 16:20:03 +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 8A88F19CF81; Wed, 13 Dec 2017 16:20:02 +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 52E4C7BA50; Wed, 13 Dec 2017 16:19:54 +0000 (UTC) Subject: Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option To: Sergio Durigan Junior , GDB Patches References: <20171121160709.23248-1-sergiodj@redhat.com> <20171213031724.22721-1-sergiodj@redhat.com> <20171213031724.22721-3-sergiodj@redhat.com> Cc: Tom Tromey , Eli Zaretskii , Simon Marchi , Keith Seitz From: Pedro Alves Message-ID: <2c84dc10-a131-31bb-8db5-74c3b3d3a95e@redhat.com> Date: Wed, 13 Dec 2017 16:20: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: <20171213031724.22721-3-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00312.txt.bz2 [Sending user-interface suggestions separately from core functionality review.] On 12/13/2017 03:17 AM, Sergio Durigan Junior wrote: > (gdb) ptype /o stap_probe > /* offset | size */ > struct stap_probe { > /* 0 | 40 */ struct probe { > /* 0 | 8 */ const probe_ops *pops; > /* 8 | 8 */ gdbarch *arch; > /* 16 | 8 */ const char *name; > /* 24 | 8 */ const char *provider; > /* 32 | 8 */ CORE_ADDR address; > } /* total size: 40 bytes */ p; > /* 40 | 8 */ CORE_ADDR sem_addr; > /* 48:31 | 4 */ unsigned int args_parsed : 1; > /* XXX 7-bit hole */ > /* XXX 7-byte hole */ > /* 56 | 8 */ union { > /* 8 */ const char *text; > /* 8 */ VEC_stap_probe_arg_s *vec; > } /* total size: 8 bytes */ args_u; > } /* total size: 64 bytes */ I've experimented with (different versions of) of this patch against gdb a bit, and I have to say that I'm finding myself a bit confused by the currently produced output. It feels like the output should be a bit more obvious and easier to skim quickly. For example, I was looking at gdb's minimal_symbol (what I had used when I noticed the bitfield handling in earlier versions was incorrect): (top-gdb) ptype/tom minimal_symbol /* offset | size */ type = struct minimal_symbol { /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; /* 8 | 8 */ union { /* 8 */ LONGEST ivalue; /* 8 */ const block *block; /* 8 */ const gdb_byte *bytes; /* 8 */ CORE_ADDR address; /* 8 */ const common_block *common_block; /* 8 */ symbol *chain; } /* total size: 8 bytes */ value; /* 16 | 8 */ union { /* 8 */ obstack *obstack; /* 8 */ const char *demangled_name; } /* total size: 8 bytes */ language_specific; /* 24:27 | 4 */ language language : 5; /* 24:26 | 4 */ unsigned int ada_mangled : 1; /* XXX 2-bit hole */ /* XXX 1-byte hole */ /* 26 | 2 */ short section; } /* total size: 32 bytes */ mginfo; /* 32 | 8 */ unsigned long size; /* 40 | 8 */ const char *filename; /* 48:28 | 4 */ minimal_symbol_type type : 4; /* 48:27 | 4 */ unsigned int created_by_gdb : 1; /* 48:26 | 4 */ unsigned int target_flag_1 : 1; /* 48:25 | 4 */ unsigned int target_flag_2 : 1; /* 48:24 | 4 */ unsigned int has_size : 1; /* XXX 7-byte hole */ /* 56 | 8 */ minimal_symbol *hash_next; /* 64 | 8 */ minimal_symbol *demangled_hash_next; } /* total size: 72 bytes */ (top-gdb) and I have the following observations: #1 - left vs right We print the extra offset/size info on the leftside of the screen, shifting the whole type to the right. If we were to put the new info on the right, then a "ptype S" vs "ptype/o S" would look mostly the same, except for the extra info that appears on the right, and it'd map better to what you'd write in actual code (who puts comments on the left side, right?) pahole(1) prints the info on the right side. Was there a particular reason gdb's version prints it on the left hand side? Maybe the Python version did that too (did it? I haven't tried it) because it'd be difficult with the Python implementation otherwise? But maybe it's just me. On to the next point in such case. #2 - shift "type =" header to the right as well? Currently, the leading "type =" and ending "}" lines are aligned on the left, along with the offset/size info. I think shifting those to the right as well might make the output nicer. The idea being that /o would split the output in two more-clearly-separated columns, with the new info panned hard on the left column, and old ptype info on the right column. I.e., currently we get: ~~~~~~~~~~~~~~~~~~~~~~~~~~~ (top-gdb) ptype/o minimal_symbol /* offset | size */ type = struct minimal_symbol { /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; ... } /* total size: 72 bytes */ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ And I'd propose instead: ~~~~~~~~~~~~~~~~~~~~~~~~~~~ (top-gdb) ptype/o minimal_symbol /* offset | size */ type = struct minimal_symbol { /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; ... } /* total size: 72 bytes */ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Or even saving one line: ~~~~~~~~~~~~~~~~~~~~~~~~~~~ (top-gdb) ptype/o minimal_symbol /* offset | size */ type = struct minimal_symbol { /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; ... } /* total size: 72 bytes */ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Or in full (compare to the version further above): (top-gdb) ptype/o minimal_symbol /* offset | size */ type = struct minimal_symbol { /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; /* 8 | 8 */ union { /* 8 */ LONGEST ivalue; /* 8 */ const block *block; /* 8 */ const gdb_byte *bytes; /* 8 */ CORE_ADDR address; /* 8 */ const common_block *common_block; /* 8 */ symbol *chain; } /* total size: 8 bytes */ value; /* 16 | 8 */ union { /* 8 */ obstack *obstack; /* 8 */ const char *demangled_name; } /* total size: 8 bytes */ language_specific; /* 24:27 | 4 */ language language : 5; /* 24:26 | 4 */ unsigned int ada_mangled : 1; /* XXX 2-bit hole */ /* XXX 1-byte hole */ /* 26 | 2 */ short section; } /* total size: 32 bytes */ mginfo; /* 32 | 8 */ unsigned long size; /* 40 | 8 */ const char *filename; /* 48:28 | 4 */ minimal_symbol_type type : 4; /* 48:27 | 4 */ unsigned int created_by_gdb : 1; /* 48:26 | 4 */ unsigned int target_flag_1 : 1; /* 48:25 | 4 */ unsigned int target_flag_2 : 1; /* 48:24 | 4 */ unsigned int has_size : 1; /* XXX 7-byte hole */ /* 56 | 8 */ minimal_symbol *hash_next; /* 64 | 8 */ minimal_symbol *demangled_hash_next; } /* total size: 72 bytes */ (top-gdb) #3 - I also find the position of those "/* total size: 8 bytes */" markers a bit hard to grok/spot. I'd suggest considering putting them on the left column along with the rest of the offset/size info as well, like: (top-gdb) ptype/tom minimal_symbol /* offset | size */ type = struct minimal_symbol { /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; /* 8 | 8 */ union { /* 8 */ LONGEST ivalue; /* 8 */ const block *block; /* 8 */ const gdb_byte *bytes; /* 8 */ CORE_ADDR address; /* 8 */ const common_block *common_block; /* 8 */ symbol *chain; /* total size: 8 */ } value; /* 16 | 8 */ union { /* 8 */ obstack *obstack; /* 8 */ const char *demangled_name; /* total size: 8 */ } language_specific; /* 24:27 | 4 */ language language : 5; /* 24:26 | 4 */ unsigned int ada_mangled : 1; /* XXX 2-bit hole */ /* XXX 1-byte hole */ /* 26 | 2 */ short section; /* total size: 32 */ } mginfo; /* 32 | 8 */ unsigned long size; /* 40 | 8 */ const char *filename; /* 48:28 | 4 */ minimal_symbol_type type : 4; /* 48:27 | 4 */ unsigned int created_by_gdb : 1; /* 48:26 | 4 */ unsigned int target_flag_1 : 1; /* 48:25 | 4 */ unsigned int target_flag_2 : 1; /* 48:24 | 4 */ unsigned int has_size : 1; /* XXX 7-byte hole */ /* 56 | 8 */ minimal_symbol *hash_next; /* 64 | 8 */ minimal_symbol *demangled_hash_next; } /* total size: 72 bytes */ (top-gdb) Or if you don't like it, consider putting that "total size:" info on a separate line before the struct/union is closed, like pahole does: /* size: 128, cachelines: 2, members: 4 */ /* sum members: 124, holes: 1, sum holes: 4 */ }; That may be even better because it provides a place to put extra info, like pahole does (cacheline info etc.). Thanks, Pedro Alves