This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH/RFA] Do not emit "field_type" var if not needed on "maint print c-tdesc"
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>
- Date: Tue, 19 Dec 2017 15:51:44 -0500
- Subject: Re: [PATCH/RFA] Do not emit "field_type" var if not needed on "maint print c-tdesc"
- Authentication-results: sourceware.org; auth=none
- References: <20171219191817.7554-1-sergiodj@redhat.com> <9ed3a5f3-f8ea-9a87-d201-40cfb0abd67e@ericsson.com>
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 <sergiodj@redhat.com>
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 <sergiodj@redhat.com>
* 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)
<visit_1>: 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