From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46824 invoked by alias); 23 Jun 2018 23:51:04 -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 46808 invoked by uid 89); 23 Jun 2018 23:51:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=nit X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 23 Jun 2018 23:51:01 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AA128401EF01; Sat, 23 Jun 2018 23:50:59 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 83A4E111AF16; Sat, 23 Jun 2018 23:50:59 +0000 (UTC) From: Sergio Durigan Junior To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA 1/2] Move ptype/o printing code to typeprint.c References: <20180623202227.17259-1-tom@tromey.com> <20180623202227.17259-2-tom@tromey.com> Date: Sat, 23 Jun 2018 23:51:00 -0000 In-Reply-To: <20180623202227.17259-2-tom@tromey.com> (Tom Tromey's message of "Sat, 23 Jun 2018 14:22:26 -0600") Message-ID: <87sh5d9i58.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-06/txt/msg00558.txt.bz2 On Saturday, June 23 2018, Tom Tromey wrote: > This moves the hole-printing support code for ptype/o from > c-typeprint.c to be methods on print_offset_data. This allows the > code to be used from non-C languages. Thanks, Tom. I looked at the patch, and it seems OK to me. Just one small nit. > gdb/ChangeLog > 2018-06-08 Tom Tromey > > * typeprint.h (struct print_offset_data) maybe_print_hole>: New methods. > : New constant. > * typeprint.c (print_offset_data::indentation): Define. > (print_offset_data::maybe_print_hole, print_offset_data::update) > (print_offset_data::finish): Move from c-typeprint.c and rename. > * c-typeprint.c (OFFSET_SPC_LEN): Remove. > (print_spaces_filtered_with_print_options): Update. > (c_print_type_union_field_offset, maybe_print_hole) > (c_print_type_struct_field_offset): Move to typeprint.c and > rename. > (c_type_print_base_struct_union): Update. > --- > gdb/ChangeLog | 15 +++++ > gdb/c-typeprint.c | 160 ++---------------------------------------------------- > gdb/typeprint.c | 122 +++++++++++++++++++++++++++++++++++++++++ > gdb/typeprint.h | 33 +++++++++++ > 4 files changed, 174 insertions(+), 156 deletions(-) > > diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c > index d7eaa5433dd..c167e212ded 100644 > --- a/gdb/c-typeprint.c > +++ b/gdb/c-typeprint.c > @@ -32,14 +32,6 @@ > #include "cp-abi.h" > #include "cp-support.h" > > -/* When printing the offsets of a struct and its fields (i.e., 'ptype > - /o'; type_print_options::print_offsets), we use this many > - characters when printing the offset information at the beginning of > - the line. This is needed in order to generate the correct amount > - of whitespaces when no offset info should be printed for a certain > - field. */ > -#define OFFSET_SPC_LEN 23 > - > /* A list of access specifiers used for printing. */ > > enum access_specifier > @@ -913,7 +905,7 @@ print_spaces_filtered_with_print_options > if (!flags->print_offsets) > print_spaces_filtered (level, stream); > else > - print_spaces_filtered (level + OFFSET_SPC_LEN, stream); > + print_spaces_filtered (level + print_offset_data::indentation, stream); > } > > /* Output an access specifier to STREAM, if needed. LAST_ACCESS is the > @@ -956,127 +948,6 @@ output_access_specifier (struct ui_file *stream, > return last_access; > } > > -/* 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)); > -} > - > -/* Helper function for ptype/o implementation that prints information > - about a hole, if necessary. STREAM is where to print. BITPOS is > - the bitpos of the current field. PODATA is the offset-printing > - state. FOR_WHAT is a string describing the purpose of the > - hole. */ > - > -static void > -maybe_print_hole (struct ui_file *stream, unsigned int bitpos, > - struct print_offset_data *podata, const char *for_what) > -{ > - /* We check for PODATA->END_BITPOS > 0 because there is a specific > - scenario when PODATA->END_BITPOS can be zero and BITPOS can be > > - 0: when we are dealing with a struct/class with a virtual method. > - Because of the vtable, the first field of the struct/class will > - have an offset of sizeof (void *) (the size of the vtable). If > - we do not check for PODATA->END_BITPOS > 0 here, GDB will report > - a hole before the first field, which is not accurate. */ > - if (podata->end_bitpos > 0 && podata->end_bitpos < bitpos) > - { > - /* If PODATA->END_BITPOS is smaller than the current type's > - bitpos, it means there's a hole in the struct, so we report > - it here. */ > - unsigned int hole = bitpos - podata->end_bitpos; > - unsigned int hole_byte = hole / TARGET_CHAR_BIT; > - unsigned int hole_bit = hole % TARGET_CHAR_BIT; > - > - if (hole_bit > 0) > - fprintf_filtered (stream, "/* XXX %2u-bit %s */\n", hole_bit, > - for_what); > - > - if (hole_byte > 0) > - fprintf_filtered (stream, "/* XXX %2u-byte %s */\n", hole_byte, > - for_what); > - } > -} > - > -/* Print information about field at index FIELD_IDX of the struct type > - TYPE. > - > - PODATA->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. > - > - PODATA->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). > - > - 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, > - struct print_offset_data *podata) > -{ > - struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx)); > - unsigned int bitpos = TYPE_FIELD_BITPOS (type, field_idx); > - unsigned int fieldsize_byte = TYPE_LENGTH (ftype); > - unsigned int fieldsize_bit = fieldsize_byte * TARGET_CHAR_BIT; > - > - maybe_print_hole (stream, bitpos, podata, "hole"); > - > - if (TYPE_FIELD_PACKED (type, field_idx)) > - { > - /* We're dealing with a bitfield. Print how many bits are left > - to be used. */ > - unsigned int bitsize = TYPE_FIELD_BITSIZE (type, field_idx); > - /* The bitpos relative to the beginning of our container > - field. */ > - unsigned int relative_bitpos; > - > - /* The following was copied from > - value.c:value_primitive_field. */ > - if ((bitpos % fieldsize_bit) + bitsize <= fieldsize_bit) > - relative_bitpos = bitpos % fieldsize_bit; > - else > - relative_bitpos = bitpos % TARGET_CHAR_BIT; > - > - /* This is the exact offset (in bits) of this bitfield. */ > - unsigned int bit_offset > - = (bitpos - relative_bitpos) + podata->offset_bitpos; > - > - /* The position of the field, relative to the beginning of the > - struct, and how many bits are left to be used in this > - container. */ > - fprintf_filtered (stream, "/* %4u:%2u", bit_offset / TARGET_CHAR_BIT, > - fieldsize_bit - (relative_bitpos + bitsize)); > - fieldsize_bit = bitsize; > - } > - else > - { > - /* The position of the field, relative to the beginning of the > - struct. */ > - fprintf_filtered (stream, "/* %4u", > - (bitpos + podata->offset_bitpos) / TARGET_CHAR_BIT); > - > - fprintf_filtered (stream, " "); > - } > - > - fprintf_filtered (stream, " | %4u */", fieldsize_byte); > - > - podata->end_bitpos = bitpos + fieldsize_bit; > -} > - > /* Return true if an access label (i.e., "public:", "private:", > "protected:") needs to be printed for TYPE. */ > > @@ -1289,20 +1160,7 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream, > bool is_static = field_is_static (&TYPE_FIELD (type, i)); > > if (flags->print_offsets) > - { > - if (!is_static) > - { > - if (TYPE_CODE (type) == TYPE_CODE_STRUCT) > - { > - c_print_type_struct_field_offset > - (type, i, stream, podata); > - } > - else if (TYPE_CODE (type) == TYPE_CODE_UNION) > - c_print_type_union_field_offset (type, i, stream); > - } > - else > - print_spaces_filtered (OFFSET_SPC_LEN, stream); > - } > + podata->update (type, i, stream); > > print_spaces_filtered (level + 4, stream); > if (is_static) > @@ -1560,19 +1418,9 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream, > if (flags->print_offsets) > { > if (show > 0) > - { > - unsigned int bitpos = TYPE_LENGTH (type) * TARGET_CHAR_BIT; > - maybe_print_hole (stream, bitpos, podata, "padding"); > - > - fputs_filtered ("\n", stream); > - print_spaces_filtered_with_print_options (level + 4, > - stream, > - flags); > - fprintf_filtered (stream, "/* total size (bytes): %4u */\n", > - TYPE_LENGTH (type)); > - } > + podata->finish (type, level, stream); > > - print_spaces_filtered (OFFSET_SPC_LEN, stream); > + print_spaces_filtered (print_offset_data::indentation, stream); > if (level == 0) > print_spaces_filtered (2, stream); > } > diff --git a/gdb/typeprint.c b/gdb/typeprint.c > index 222fc0a06b1..66ba0a87c6a 100644 > --- a/gdb/typeprint.c > +++ b/gdb/typeprint.c > @@ -65,6 +65,128 @@ static struct type_print_options default_ptype_flags = > > > > +const int print_offset_data::indentation = 23; A "/* See typeprint.h. */" comment would be nice here, IMO. > + > + > +/* See typeprint.h. */ > + > +void > +print_offset_data::maybe_print_hole (struct ui_file *stream, > + unsigned int bitpos, > + const char *for_what) > +{ > + /* We check for END_BITPOS > 0 because there is a specific > + scenario when END_BITPOS can be zero and BITPOS can be > > + 0: when we are dealing with a struct/class with a virtual method. > + Because of the vtable, the first field of the struct/class will > + have an offset of sizeof (void *) (the size of the vtable). If > + we do not check for END_BITPOS > 0 here, GDB will report > + a hole before the first field, which is not accurate. */ > + if (end_bitpos > 0 && end_bitpos < bitpos) > + { > + /* If END_BITPOS is smaller than the current type's > + bitpos, it means there's a hole in the struct, so we report > + it here. */ > + unsigned int hole = bitpos - end_bitpos; > + unsigned int hole_byte = hole / TARGET_CHAR_BIT; > + unsigned int hole_bit = hole % TARGET_CHAR_BIT; > + > + if (hole_bit > 0) > + fprintf_filtered (stream, "/* XXX %2u-bit %s */\n", hole_bit, > + for_what); > + > + if (hole_byte > 0) > + fprintf_filtered (stream, "/* XXX %2u-byte %s */\n", hole_byte, > + for_what); > + } > +} > + > +/* See typeprint.h. */ > + > +void > +print_offset_data::update (struct type *type, unsigned int field_idx, > + struct ui_file *stream) > +{ > + if (field_is_static (&TYPE_FIELD (type, field_idx))) > + { > + print_spaces_filtered (indentation, stream); > + return; > + } > + > + struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx)); > + if (TYPE_CODE (type) == TYPE_CODE_UNION) > + { > + /* Since union fields don't have the concept of offsets, we just > + print their sizes. */ > + fprintf_filtered (stream, "/* %4u */", TYPE_LENGTH (ftype)); > + return; > + } > + > + unsigned int bitpos = TYPE_FIELD_BITPOS (type, field_idx); > + unsigned int fieldsize_byte = TYPE_LENGTH (ftype); > + unsigned int fieldsize_bit = fieldsize_byte * TARGET_CHAR_BIT; > + > + maybe_print_hole (stream, bitpos, "hole"); > + > + if (TYPE_FIELD_PACKED (type, field_idx)) > + { > + /* We're dealing with a bitfield. Print how many bits are left > + to be used. */ > + unsigned int bitsize = TYPE_FIELD_BITSIZE (type, field_idx); > + /* The bitpos relative to the beginning of our container > + field. */ > + unsigned int relative_bitpos; > + > + /* The following was copied from > + value.c:value_primitive_field. */ > + if ((bitpos % fieldsize_bit) + bitsize <= fieldsize_bit) > + relative_bitpos = bitpos % fieldsize_bit; > + else > + relative_bitpos = bitpos % TARGET_CHAR_BIT; > + > + /* This is the exact offset (in bits) of this bitfield. */ > + unsigned int bit_offset > + = (bitpos - relative_bitpos) + offset_bitpos; > + > + /* The position of the field, relative to the beginning of the > + struct, and how many bits are left to be used in this > + container. */ > + fprintf_filtered (stream, "/* %4u:%2u", bit_offset / TARGET_CHAR_BIT, > + fieldsize_bit - (relative_bitpos + bitsize)); > + fieldsize_bit = bitsize; > + } > + else > + { > + /* The position of the field, relative to the beginning of the > + struct. */ > + fprintf_filtered (stream, "/* %4u", > + (bitpos + offset_bitpos) / TARGET_CHAR_BIT); > + > + fprintf_filtered (stream, " "); > + } > + > + fprintf_filtered (stream, " | %4u */", fieldsize_byte); > + > + end_bitpos = bitpos + fieldsize_bit; > +} > + > +/* See typeprint.h. */ > + > +void > +print_offset_data::finish (struct type *type, int level, > + struct ui_file *stream) > +{ > + unsigned int bitpos = TYPE_LENGTH (type) * TARGET_CHAR_BIT; > + maybe_print_hole (stream, bitpos, "padding"); > + > + fputs_filtered ("\n", stream); > + print_spaces_filtered (level + 4 + print_offset_data::indentation, stream); > + fprintf_filtered (stream, "/* total size (bytes): %4u */\n", > + TYPE_LENGTH (type)); > +} > + > + > + > /* A hash function for a typedef_field. */ > > static hashval_t > diff --git a/gdb/typeprint.h b/gdb/typeprint.h > index 74006b3058f..edd8c396c87 100644 > --- a/gdb/typeprint.h > +++ b/gdb/typeprint.h > @@ -38,6 +38,39 @@ struct print_offset_data > field (where we expect the current field to be if there is no > hole). */ > unsigned int end_bitpos = 0; > + > + /* Print information about field at index FIELD_IDX of the struct type > + TYPE and update this object. > + > + If the field is static, it simply prints the correct number of > + spaces. > + > + The output is strongly based on pahole(1). */ > + void update (struct type *type, unsigned int field_idx, > + struct ui_file *stream); > + > + /* Call when all fields have been printed. This will print > + information about any padding that may exist. LEVEL is the > + desired indentation level. */ > + void finish (struct type *type, int level, struct ui_file *stream); > + > + /* When printing the offsets of a struct and its fields (i.e., > + 'ptype /o'; type_print_options::print_offsets), we use this many > + characters when printing the offset information at the beginning > + of the line. This is needed in order to generate the correct > + amount of whitespaces when no offset info should be printed for a > + certain field. */ > + static const int indentation; > + > +private: > + > + /* Helper function for ptype/o implementation that prints > + information about a hole, if necessary. STREAM is where to > + print. BITPOS is the bitpos of the current field. FOR_WHAT is a > + string describing the purpose of the hole. */ > + > + void maybe_print_hole (struct ui_file *stream, unsigned int bitpos, > + const char *for_what); > }; > > struct type_print_options > -- > 2.13.6 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/