From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29196 invoked by alias); 19 Dec 2017 20:51:49 -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 29183 invoked by uid 89); 19 Dec 2017 20:51:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,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; Tue, 19 Dec 2017 20:51:46 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F254061470; Tue, 19 Dec 2017 20:51:44 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AB4969230E; Tue, 19 Dec 2017 20:51:44 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches Subject: Re: [PATCH/RFA] Do not emit "field_type" var if not needed on "maint print c-tdesc" References: <20171219191817.7554-1-sergiodj@redhat.com> <9ed3a5f3-f8ea-9a87-d201-40cfb0abd67e@ericsson.com> Date: Tue, 19 Dec 2017 20:51:00 -0000 In-Reply-To: <9ed3a5f3-f8ea-9a87-d201-40cfb0abd67e@ericsson.com> (Simon Marchi's message of "Tue, 19 Dec 2017 15:15:45 -0500") Message-ID: <87y3ly4f1b.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: 2017-12/txt/msg00437.txt.bz2 On Tuesday, December 19 2017, Simon Marchi wrote: > On 2017-12-19 02:18 PM, Sergio Durigan Junior wrote: >> While fiddling a bit with -Wunused-variable, I noticed that "maint >> print c-tdesc" was always generating code for the >> "tdesc_type *field_type" variable, even when it wasn't used. This is >> caught by GCC when using -Wunused-variable, of course. So I modified >> the "print_c_tdesc" class to check whether "field_type" will be needed >> or not. >> >> In order to do the check, I basically copied the logic implemented on >> "void visit (const tdesc_type_with_fields *type)", specifically the >> "switch" part, and simplified it to determine which types need >> "field_type". It's on a new simple function called >> "need_field_type". Then, we can simply call this function when >> deciding whether to print "field_type", and that's it. >> >> I've also regenerated all the C files under gdb/features/, and as >> expected only those that don't need "field_type" were modified. > > I fiddled with this when I made my series about C++ifying tdesc, but removed it > from the series since I thought it wasn't that important. I'm not against fixing > it though. I think it would be nice if we used > "-Wunused-variable -Wno-error=unused-variable", in which case your patch would > avoid some warnings/noise. > > However, I don't really like the fact that this approach duplicates the logic. > A simpler way would be to emit the field lazily when we actually need it, see patch > below for an example. There's a bit more collateral damage in the generated files, > since some declarations change place. We would need to do the same for the other > variable declarations. Yeah, TBH I wasn't entirely happy with duplicating the logic, but I thought it'd be OK because it's a small function anyway. Having said that, I think your solution is good. Yet another solution that I thought would be to make a "visit_1" function that generates the entire text to be printf_unfiltered'd, along with a boolean signaling whether "field_type" was needed. Then, visit_1's caller would emit "field_type" as needed, and print the string returned by visit_1. > Another solution I thought of would be to use a new scope every time we have an > assignment and declare the variable in each scope. For example, this > > tdesc_type_with_fields *type_with_fields; > type_with_fields = tdesc_create_union (feature, "vec128"); > tdesc_type *field_type; > field_type = tdesc_named_type (feature, "v4f"); > tdesc_add_field (type_with_fields, "v4_float", field_type); > field_type = tdesc_named_type (feature, "v2d"); > tdesc_add_field (type_with_fields, "v2_double", field_type); > field_type = tdesc_named_type (feature, "v16i8"); > tdesc_add_field (type_with_fields, "v16_int8", field_type); > field_type = tdesc_named_type (feature, "v8i16"); > tdesc_add_field (type_with_fields, "v8_int16", field_type); > field_type = tdesc_named_type (feature, "v4i32"); > tdesc_add_field (type_with_fields, "v4_int32", field_type); > field_type = tdesc_named_type (feature, "v2i64"); > tdesc_add_field (type_with_fields, "v2_int64", field_type); > field_type = tdesc_named_type (feature, "uint128"); > tdesc_add_field (type_with_fields, "uint128", field_type); > > would become > > { > tdesc_type_with_fields *type_with_fields = tdesc_create_union (feature, "vec128"); > { > tdesc_type *field_type = tdesc_named_type (feature, "v4f"); > tdesc_add_field (type_with_fields, "v4_float", field_type); > } > { > tdesc_type *field_type = tdesc_named_type (feature, "v2d"); > tdesc_add_field (type_with_fields, "v2_double", field_type); > } > ... > } > > That should avoid the need to any logic to track which variables have > been declared. It would make the generated code a bit more verbose, > but I don't think it matters. IMHO it'd not be ideal to introduce more syntax (which can ultimately lead to difficulties understanding the code flow) just to avoid silencing a warning. I think your solution below addresses this problem just fine. I'm attaching the patch I mentioned above here, in case you want to go with it. But as I said, your approach is OK too. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ >From 9ee58633a45961f72eff233509f4b6d34b1e2399 Mon Sep 17 00:00:00 2001 From: Sergio Durigan Junior Date: Tue, 19 Dec 2017 14:00:08 -0500 Subject: [PATCH/RFA] Do not emit "field_type" var if not needed on "maint print c-tdesc" While fiddling a bit with -Wunused-variable, I noticed that "maint print c-tdesc" was always generating code for the "tdesc_type *field_type" variable, even when it wasn't used. This is caught by GCC when using -Wunused-variable, of course. So I modified the "print_c_tdesc" class to check whether "field_type" will be needed or not. I've also regenerated all the C files under gdb/features/, and as expected only those that don't need "field_type" were modified. yyyy-mm-dd Sergio Durigan Junior * features/aarch64-core.c: Regenerate. * features/arc-arcompact.c: Regenerate. * features/arc-v2.c: Regenerate. * features/i386/32bit-core.c: Regenerate. * features/i386/64bit-core.c: Regenerate. * features/i386/x32-core.c: Regenerate. * features/or1k.c: Regenerate. * target-descriptions.c (class print_c_tdesc) : New method. --- gdb/features/aarch64-core.c | 1 - gdb/features/arc-arcompact.c | 1 - gdb/features/arc-v2.c | 1 - gdb/features/i386/32bit-core.c | 1 - gdb/features/i386/64bit-core.c | 1 - gdb/features/i386/x32-core.c | 1 - gdb/features/or1k.c | 1 - gdb/target-descriptions.c | 127 ++++++++++++++++++++++++++--------------- 8 files changed, 80 insertions(+), 54 deletions(-) diff --git a/gdb/features/aarch64-core.c b/gdb/features/aarch64-core.c index 618a7ef787..3707b7e055 100644 --- a/gdb/features/aarch64-core.c +++ b/gdb/features/aarch64-core.c @@ -10,7 +10,6 @@ create_feature_aarch64_core (struct target_desc *result, long regnum) feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.core", "aarch64-core.xml"); tdesc_type_with_fields *type_with_fields; - tdesc_type *field_type; type_with_fields = tdesc_create_flags (feature, "cpsr_flags", 4); tdesc_add_flag (type_with_fields, 0, "SP"); tdesc_add_flag (type_with_fields, 1, ""); diff --git a/gdb/features/arc-arcompact.c b/gdb/features/arc-arcompact.c index 79b6889172..f81f0a26ba 100644 --- a/gdb/features/arc-arcompact.c +++ b/gdb/features/arc-arcompact.c @@ -52,7 +52,6 @@ initialize_tdesc_arc_arcompact (void) feature = tdesc_create_feature (result, "org.gnu.gdb.arc.aux-minimal"); tdesc_type_with_fields *type_with_fields; - tdesc_type *field_type; type_with_fields = tdesc_create_flags (feature, "status32_type", 4); tdesc_add_flag (type_with_fields, 0, "H"); tdesc_add_bitfield (type_with_fields, "E", 1, 2); diff --git a/gdb/features/arc-v2.c b/gdb/features/arc-v2.c index 9908b4c5ec..b2254b293c 100644 --- a/gdb/features/arc-v2.c +++ b/gdb/features/arc-v2.c @@ -52,7 +52,6 @@ initialize_tdesc_arc_v2 (void) feature = tdesc_create_feature (result, "org.gnu.gdb.arc.aux-minimal"); tdesc_type_with_fields *type_with_fields; - tdesc_type *field_type; type_with_fields = tdesc_create_flags (feature, "status32_type", 4); tdesc_add_flag (type_with_fields, 0, "H"); tdesc_add_bitfield (type_with_fields, "E", 1, 4); diff --git a/gdb/features/i386/32bit-core.c b/gdb/features/i386/32bit-core.c index de2ce474d5..294e86d81e 100644 --- a/gdb/features/i386/32bit-core.c +++ b/gdb/features/i386/32bit-core.c @@ -10,7 +10,6 @@ create_feature_i386_32bit_core (struct target_desc *result, long regnum) feature = tdesc_create_feature (result, "org.gnu.gdb.i386.core", "32bit-core.xml"); tdesc_type_with_fields *type_with_fields; - tdesc_type *field_type; type_with_fields = tdesc_create_flags (feature, "i386_eflags", 4); tdesc_add_flag (type_with_fields, 0, "CF"); tdesc_add_flag (type_with_fields, 1, ""); diff --git a/gdb/features/i386/64bit-core.c b/gdb/features/i386/64bit-core.c index f4cad06e66..9e39ee42d9 100644 --- a/gdb/features/i386/64bit-core.c +++ b/gdb/features/i386/64bit-core.c @@ -10,7 +10,6 @@ create_feature_i386_64bit_core (struct target_desc *result, long regnum) feature = tdesc_create_feature (result, "org.gnu.gdb.i386.core", "64bit-core.xml"); tdesc_type_with_fields *type_with_fields; - tdesc_type *field_type; type_with_fields = tdesc_create_flags (feature, "i386_eflags", 4); tdesc_add_flag (type_with_fields, 0, "CF"); tdesc_add_flag (type_with_fields, 1, ""); diff --git a/gdb/features/i386/x32-core.c b/gdb/features/i386/x32-core.c index acafc7dace..c268e11bea 100644 --- a/gdb/features/i386/x32-core.c +++ b/gdb/features/i386/x32-core.c @@ -10,7 +10,6 @@ create_feature_i386_x32_core (struct target_desc *result, long regnum) feature = tdesc_create_feature (result, "org.gnu.gdb.i386.core", "x32-core.xml"); tdesc_type_with_fields *type_with_fields; - tdesc_type *field_type; type_with_fields = tdesc_create_flags (feature, "i386_eflags", 4); tdesc_add_flag (type_with_fields, 0, "CF"); tdesc_add_flag (type_with_fields, 1, ""); diff --git a/gdb/features/or1k.c b/gdb/features/or1k.c index 929a5f9208..9169cae940 100644 --- a/gdb/features/or1k.c +++ b/gdb/features/or1k.c @@ -16,7 +16,6 @@ initialize_tdesc_or1k (void) feature = tdesc_create_feature (result, "org.gnu.gdb.or1k.group0"); tdesc_type_with_fields *type_with_fields; - tdesc_type *field_type; type_with_fields = tdesc_create_flags (feature, "sr_flags", 4); tdesc_add_flag (type_with_fields, 0, "SM"); tdesc_add_flag (type_with_fields, 1, "TEE"); diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 88ac55f404..0316a7fcc5 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -1888,36 +1888,73 @@ public: void visit (const tdesc_type_with_fields *type) override { + bool need_field_type; + if (!m_printed_type_with_fields) { printf_unfiltered (" tdesc_type_with_fields *type_with_fields;\n"); m_printed_type_with_fields = true; } + std::string text = visit_1 (type, &need_field_type); + if (!type->fields.empty () - && !m_printed_field_type) + && !m_printed_field_type + && need_field_type) { printf_unfiltered (" tdesc_type *field_type;\n"); m_printed_field_type = true; } + printf_unfiltered ("%s", text.c_str ()); + + printf_unfiltered ("\n"); + } + + void visit (const tdesc_reg *reg) override + { + printf_unfiltered (" tdesc_create_reg (feature, \"%s\", %ld, %d, ", + reg->name.c_str (), reg->target_regnum, + reg->save_restore); + if (!reg->group.empty ()) + printf_unfiltered ("\"%s\", ", reg->group.c_str ()); + else + printf_unfiltered ("NULL, "); + printf_unfiltered ("%d, \"%s\");\n", reg->bitsize, reg->type.c_str ()); + } + +protected: + std::string m_filename_after_features; + +private: + std::string visit_1 (const tdesc_type_with_fields *type, + bool *need_field_type) + { + std::string ret; + + *need_field_type = false; + switch (type->kind) { case TDESC_TYPE_STRUCT: case TDESC_TYPE_FLAGS: if (type->kind == TDESC_TYPE_STRUCT) { - printf_unfiltered - (" type_with_fields = tdesc_create_struct (feature, \"%s\");\n", + string_appendf + (ret, + " type_with_fields = tdesc_create_struct (feature, \"%s\");\n", type->name.c_str ()); if (type->size != 0) - printf_unfiltered - (" tdesc_set_struct_size (type_with_fields, %d);\n", type->size); + string_appendf + (ret, + " tdesc_set_struct_size (type_with_fields, %d);\n", + type->size); } else { - printf_unfiltered - (" type_with_fields = tdesc_create_flags (feature, \"%s\", %d);\n", + string_appendf + (ret, + " type_with_fields = tdesc_create_flags (feature, \"%s\", %d);\n", type->name.c_str (), type->size); } for (const tdesc_type_field &f : type->fields) @@ -1935,25 +1972,30 @@ public: if (f.type->kind == TDESC_TYPE_BOOL) { gdb_assert (f.start == f.end); - printf_unfiltered - (" tdesc_add_flag (type_with_fields, %d, \"%s\");\n", + string_appendf + (ret, + " tdesc_add_flag (type_with_fields, %d, \"%s\");\n", f.start, f.name.c_str ()); } else if ((type->size == 4 && f.type->kind == TDESC_TYPE_UINT32) || (type->size == 8 && f.type->kind == TDESC_TYPE_UINT64)) { - printf_unfiltered - (" tdesc_add_bitfield (type_with_fields, \"%s\", %d, %d);\n", + string_appendf + (ret, + " tdesc_add_bitfield (type_with_fields, \"%s\", %d, %d);\n", f.name.c_str (), f.start, f.end); } else { - printf_unfiltered - (" field_type = tdesc_named_type (feature, \"%s\");\n", + *need_field_type = true; + string_appendf + (ret, + " field_type = tdesc_named_type (feature, \"%s\");\n", type_name); - printf_unfiltered - (" tdesc_add_typed_bitfield (type_with_fields, \"%s\"," + string_appendf + (ret, + " tdesc_add_typed_bitfield (type_with_fields, \"%s\"," " %d, %d, field_type);\n", f.name.c_str (), f.start, f.end); } @@ -1962,62 +2004,53 @@ public: { gdb_assert (f.end == -1); gdb_assert (type->kind == TDESC_TYPE_STRUCT); - printf_unfiltered - (" field_type = tdesc_named_type (feature," + *need_field_type = true; + string_appendf + (ret, " field_type = tdesc_named_type (feature," " \"%s\");\n", type_name); - printf_unfiltered - (" tdesc_add_field (type_with_fields, \"%s\", field_type);\n", + string_appendf + (ret, + " tdesc_add_field (type_with_fields, \"%s\", field_type);\n", f.name.c_str ()); } } break; case TDESC_TYPE_UNION: - printf_unfiltered - (" type_with_fields = tdesc_create_union (feature, \"%s\");\n", + string_appendf + (ret, + " type_with_fields = tdesc_create_union (feature, \"%s\");\n", type->name.c_str ()); + *need_field_type = true; for (const tdesc_type_field &f : type->fields) { - printf_unfiltered - (" field_type = tdesc_named_type (feature, \"%s\");\n", + string_appendf + (ret, + " field_type = tdesc_named_type (feature, \"%s\");\n", f.type->name.c_str ()); - printf_unfiltered - (" tdesc_add_field (type_with_fields, \"%s\", field_type);\n", + string_appendf + (ret, + " tdesc_add_field (type_with_fields, \"%s\", field_type);\n", f.name.c_str ()); } break; case TDESC_TYPE_ENUM: - printf_unfiltered - (" type_with_fields = tdesc_create_enum (feature, \"%s\", %d);\n", + string_appendf + (ret, + " type_with_fields = tdesc_create_enum (feature, \"%s\", %d);\n", type->name.c_str (), type->size); for (const tdesc_type_field &f : type->fields) - printf_unfiltered - (" tdesc_add_enum_value (type_with_fields, %d, \"%s\");\n", + string_appendf + (ret, + " tdesc_add_enum_value (type_with_fields, %d, \"%s\");\n", f.start, f.name.c_str ()); break; default: error (_("C output is not supported type \"%s\"."), type->name.c_str ()); } - - printf_unfiltered ("\n"); - } - - void visit (const tdesc_reg *reg) override - { - printf_unfiltered (" tdesc_create_reg (feature, \"%s\", %ld, %d, ", - reg->name.c_str (), reg->target_regnum, - reg->save_restore); - if (!reg->group.empty ()) - printf_unfiltered ("\"%s\", ", reg->group.c_str ()); - else - printf_unfiltered ("NULL, "); - printf_unfiltered ("%d, \"%s\");\n", reg->bitsize, reg->type.c_str ()); + return ret; } -protected: - std::string m_filename_after_features; - -private: char *m_function; /* Did we print "struct tdesc_type *element_type;" yet? */ -- 2.14.3