Message ID | 20200213203035.30157-3-simon.marchi@efficios.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 29146 invoked by alias); 13 Feb 2020 20:30:49 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 29122 invoked by uid 89); 13 Feb 2020 20:30:47 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail.efficios.com Received: from mail.efficios.com (HELO mail.efficios.com) (167.114.26.124) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Feb 2020 20:30:45 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 3F575261448 for <gdb-patches@sourceware.org>; Thu, 13 Feb 2020 15:30:44 -0500 (EST) Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id Txf9ulyMilPi; Thu, 13 Feb 2020 15:30:44 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id BA8A7261618; Thu, 13 Feb 2020 15:30:42 -0500 (EST) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com BA8A7261618 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficios.com; s=default; t=1581625842; bh=b2U9/37rZayAuYxm7//WwNsgBxlTM38sw/XeFmWiaGM=; h=From:To:Date:Message-Id:MIME-Version; b=F8sltJo83iGccDBL3NmYUVZ4qlKPOSHrMKVvIYYHZvR/v+CgO62RqlaeinE/pRHhH jeamQ74P7ZYAakYLkYzoi5EyFsPn9xSzybLQj1C7mDIPCVNGwpeK8dr5xhIKzGvB5k hUYPrLQ+bZSViRAuHcRyOHATvO/PjBYT/gMgdJkjKnSqFyQu9IPyC6pmpiAAbzM0fv N87mM2rfmuSHkDqnmgaYU372MYMzURPszoHnCK8JSOfD92QbkhuQD2KhTWsxKcpSNC WEQihI5qEC3jCsOI28g4bs6vGfGiy22F5ZLLrC57FDRG4v4RK70h7sVmBfAT/hvMqd t2Ie/woiJTr4g== Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id 0vhVlFqIWaIT; Thu, 13 Feb 2020 15:30:42 -0500 (EST) Received: from smarchi-efficios.internal.efficios.com (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) by mail.efficios.com (Postfix) with ESMTPSA id 8D742261700; Thu, 13 Feb 2020 15:30:42 -0500 (EST) From: Simon Marchi <simon.marchi@efficios.com> To: gdb-patches@sourceware.org Cc: Simon Marchi <simon.marchi@efficios.com> Subject: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums Date: Thu, 13 Feb 2020 15:30:33 -0500 Message-Id: <20200213203035.30157-3-simon.marchi@efficios.com> In-Reply-To: <20200213203035.30157-1-simon.marchi@efficios.com> References: <20200213203035.30157-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable |
Commit Message
Simon Marchi
Feb. 13, 2020, 8:30 p.m. UTC
I have come across some uses cases where it would be desirable to treat an enum that has duplicate values as a "flag enum". For example, this one here [1]: enum membarrier_cmd { MEMBARRIER_CMD_QUERY = 0, MEMBARRIER_CMD_GLOBAL = (1 << 0), MEMBARRIER_CMD_GLOBAL_EXPEDITED = (1 << 1), MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED = (1 << 2), MEMBARRIER_CMD_PRIVATE_EXPEDITED = (1 << 3), MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED = (1 << 4), MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 5), MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 6), /* Alias for header backward compatibility. */ MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL, }; The last enumerator is kept for backwards compatibility. Without this patch, this enumeration wouldn't be considered a flag enum, because two enumerators collide. With this patch, it would be considered a flag enum, and the value 3 would be printed as: MEMBARRIER_CMD_GLOBAL | MEMBARRIER_CMD_GLOBAL_EXPEDITED Although if people prefer, we could display both MEMBARRIER_CMD_GLOBAL and MEMBARRIER_CMD_SHARED in the result. It wouldn't be wrong, and could perhaps be useful in case a bit may have multiple meanings (depending on some other bit value). [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/membarrier.h?id=0bf999f9c5e74c7ecf9dafb527146601e5c848b9#n125 --- gdb/dwarf2/read.c | 5 ----- gdb/testsuite/gdb.base/printcmds.c | 7 ++++--- 2 files changed, 4 insertions(+), 8 deletions(-)
Comments
On 2/13/20 5:30 PM, Simon Marchi wrote: > I have come across some uses cases where it would be desirable to treat > an enum that has duplicate values as a "flag enum". For example, this > one here [1]: > > enum membarrier_cmd { > MEMBARRIER_CMD_QUERY = 0, > MEMBARRIER_CMD_GLOBAL = (1 << 0), > MEMBARRIER_CMD_GLOBAL_EXPEDITED = (1 << 1), > MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED = (1 << 2), > MEMBARRIER_CMD_PRIVATE_EXPEDITED = (1 << 3), > MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED = (1 << 4), > MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 5), > MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 6), > > /* Alias for header backward compatibility. */ > MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL, > }; > > The last enumerator is kept for backwards compatibility. Without this > patch, this enumeration wouldn't be considered a flag enum, because two > enumerators collide. With this patch, it would be considered a flag > enum, and the value 3 would be printed as: > > MEMBARRIER_CMD_GLOBAL | MEMBARRIER_CMD_GLOBAL_EXPEDITED > Sounds reasonable to me. > Although if people prefer, we could display both MEMBARRIER_CMD_GLOBAL > and MEMBARRIER_CMD_SHARED in the result. It wouldn't be wrong, and > could perhaps be useful in case a bit may have multiple meanings > (depending on some other bit value). It would be fine either way for me. Since it is a backwards compatibility value, i guess it will be less and less used as time goes by. Not sure if it would be great to keep displaying it when it is no longer so useful. I'd keep just one value. If others want it, maybe we could enable it via a separate display option. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/membarrier.h?id=0bf999f9c5e74c7ecf9dafb527146601e5c848b9#n125 > --- > gdb/dwarf2/read.c | 5 ----- > gdb/testsuite/gdb.base/printcmds.c | 7 ++++--- > 2 files changed, 4 insertions(+), 8 deletions(-) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index b866cc2d5747..5c34667ef36d 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -15495,7 +15495,6 @@ update_enumeration_type_from_children (struct die_info *die, > struct die_info *child_die; > int unsigned_enum = 1; > int flag_enum = 1; > - ULONGEST mask = 0; > > auto_obstack obstack; > > @@ -15533,10 +15532,6 @@ update_enumeration_type_from_children (struct die_info *die, > > if (nbits != 0 && nbits && nbits != 1) > flag_enum = 0; > - else if ((mask & value) != 0) > - flag_enum = 0; > - else > - mask |= value; > } > > /* If we already know that the enum type is neither unsigned, nor > diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c > index f0b4fa4b86b1..81d2efe1a037 100644 > --- a/gdb/testsuite/gdb.base/printcmds.c > +++ b/gdb/testsuite/gdb.base/printcmds.c > @@ -99,9 +99,10 @@ volatile enum some_volatile_enum some_volatile_enum = enumvolval1; > /* An enum considered as a "flag enum". */ > enum flag_enum > { > - FE_NONE = 0x00, > - FE_ONE = 0x01, > - FE_TWO = 0x02, > + FE_NONE = 0x00, > + FE_ONE = 0x01, > + FE_TWO = 0x02, > + FE_TWO_LEGACY = 0x02, > }; > > enum flag_enum three = FE_ONE | FE_TWO; > LGTM.
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
Simon> I have come across some uses cases where it would be desirable to treat
Simon> an enum that has duplicate values as a "flag enum". For example, this
Simon> one here [1]:
Simon> /* Alias for header backward compatibility. */
Simon> MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL,
Simon> };
Simon> The last enumerator is kept for backwards compatibility. Without this
Simon> patch, this enumeration wouldn't be considered a flag enum, because two
Simon> enumerators collide. With this patch, it would be considered a flag
Simon> enum, and the value 3 would be printed as:
Does it always choose the first enumerator?
Simon> if (nbits != 0 && nbits && nbits != 1)
Simon> flag_enum = 0;
Simon> - else if ((mask & value) != 0)
Simon> - flag_enum = 0;
Simon> - else
Simon> - mask |= value;
I wonder if this allows too much, though.
Maybe instead it should check for duplicate enumerator values and allow
those, while still disallowing enums with conflicts, like:
enum x {
one = 0x11,
two = 0x10,
three = 0x01
};
... which probably isn't a sensible flag enum.
Tom
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> Maybe instead it should check for duplicate enumerator values and allow
Tom> those, while still disallowing enums with conflicts, like:
Tom> enum x {
Tom> one = 0x11,
Tom> two = 0x10,
Tom> three = 0x01
Tom> };
Tom> ... which probably isn't a sensible flag enum.
You can ignore this, I see it was already mentioned in patch 2.
Tom
On 2020-02-18 3:38 p.m., Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes: > > Simon> I have come across some uses cases where it would be desirable to treat > Simon> an enum that has duplicate values as a "flag enum". For example, this > Simon> one here [1]: > > Simon> /* Alias for header backward compatibility. */ > Simon> MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL, > Simon> }; > > Simon> The last enumerator is kept for backwards compatibility. Without this > Simon> patch, this enumeration wouldn't be considered a flag enum, because two > Simon> enumerators collide. With this patch, it would be considered a flag > Simon> enum, and the value 3 would be printed as: > > Does it always choose the first enumerator? Yes. generic_val_print_enum_1 iterates on the enum type's fields (so, enumerators) and clears those bits in the value as it goes. So if multiple enumerators match, the first one in the enum type's fields will be printed. Is this list of fields guaranteed to be in the same order as what's in the code though? > > Simon> if (nbits != 0 && nbits && nbits != 1) > Simon> flag_enum = 0; > Simon> - else if ((mask & value) != 0) > Simon> - flag_enum = 0; > Simon> - else > Simon> - mask |= value; > > I wonder if this allows too much, though. > > Maybe instead it should check for duplicate enumerator values and allow > those, while still disallowing enums with conflicts, like: > > enum x { > one = 0x11, > two = 0x10, > three = 0x01 > }; > > ... which probably isn't a sensible flag enum. Not sure what you mean here. With the current patch, this enum would not bne marked as a flag enum, as `one` has multiple bits set. Simon
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
Simon> Is this list of fields guaranteed to be
Simon> in the same order as what's in the code though?
Yeah, should be, of course subject to what the compiler decides.
Simon> Not sure what you mean here. With the current patch, this enum would not
Simon> bne marked as a flag enum, as `one` has multiple bits set.
I read the patches in the wrong order. FAOD this one also looks good to me.
Tom
On 2020-02-18 4:57 p.m., Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes: > > Simon> Is this list of fields guaranteed to be > Simon> in the same order as what's in the code though? > > Yeah, should be, of course subject to what the compiler decides. > > Simon> Not sure what you mean here. With the current patch, this enum would not > Simon> bne marked as a flag enum, as `one` has multiple bits set. > > I read the patches in the wrong order. FAOD this one also looks good to me. Thanks, I'll push the series then. Simon
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index b866cc2d5747..5c34667ef36d 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -15495,7 +15495,6 @@ update_enumeration_type_from_children (struct die_info *die, struct die_info *child_die; int unsigned_enum = 1; int flag_enum = 1; - ULONGEST mask = 0; auto_obstack obstack; @@ -15533,10 +15532,6 @@ update_enumeration_type_from_children (struct die_info *die, if (nbits != 0 && nbits && nbits != 1) flag_enum = 0; - else if ((mask & value) != 0) - flag_enum = 0; - else - mask |= value; } /* If we already know that the enum type is neither unsigned, nor diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c index f0b4fa4b86b1..81d2efe1a037 100644 --- a/gdb/testsuite/gdb.base/printcmds.c +++ b/gdb/testsuite/gdb.base/printcmds.c @@ -99,9 +99,10 @@ volatile enum some_volatile_enum some_volatile_enum = enumvolval1; /* An enum considered as a "flag enum". */ enum flag_enum { - FE_NONE = 0x00, - FE_ONE = 0x01, - FE_TWO = 0x02, + FE_NONE = 0x00, + FE_ONE = 0x01, + FE_TWO = 0x02, + FE_TWO_LEGACY = 0x02, }; enum flag_enum three = FE_ONE | FE_TWO;