[PATCH 1/5] gnulib: import count-one-bits module and use it

[PATCH 1/5] gnulib: import count-one-bits module and use it

[Simon Marchi]

In a following patch, I need a function that counts the number of set
bits in a word.  There is  __builtin_popcount that is supported by gcc
and clang, but there is also a gnulib module that wraps it and provides
a fallback for other compilers, so we might as well use that.

I also noticed that there is a bitcount function in arch/arm.c, so as a
first step, this patch replaces the uses of that function with gnulib's
function.

The gnulib module actually provides multiple functions, with various
parameter length (unsigned int, unsigned long int, unsigned long long
int), I chose the one that made sense for each call site based on the
argument type.

gnulib/ChangeLog:

	* update-gnulib.sh (IMPORTED_GNULIB_MODULES): Import
	count-one-bits module.
	* configure: Re-generate.
	* aclocal.m4: Re-generate.
	* Makefile.in: Re-generate.
	* import/count-one-bits.c: New file.
	* import/count-one-bits.h: New file.
	* import/Makefile.am: Re-generate.
	* import/Makefile.in: Re-generate.
	* import/m4/gnulib-cache.m4: Re-generate.
	* import/m4/gnulib-comp.m4: Re-generate.
	* import/m4/count-one-bits.m4: New file.

gdb/ChangeLog:

	* arm-tdep.c: Include count-one-bits.h.
	(cleanup_block_store_pc): Use count_one_bits.
	(cleanup_block_load_pc): Use count_one_bits.
	(arm_copy_block_xfer): Use count_one_bits.
	(thumb2_copy_block_xfer): Use count_one_bits.
	(thumb_copy_pop_pc_16bit): Use count_one_bits.
	* arch/arm-get-next-pcs.c: Include count-one-bits.h.
	(thumb_get_next_pcs_raw): Use count_one_bits.
	(arm_get_next_pcs_raw): Use count_one_bits_l.
	* arch/arm.c (bitcount): Remove.
	* arch/arm.h (bitcount): Remove.
---
 gdb/arch/arm-get-next-pcs.c        |   9 +-
 gdb/arch/arm.c                     |  11 ---
 gdb/arch/arm.h                     |   3 -
 gdb/arm-tdep.c                     |  12 +--
 gnulib/Makefile.in                 |   5 +-
 gnulib/aclocal.m4                  |   1 +
 gnulib/configure                   | 118 +++++++++++++------------
 gnulib/import/Makefile.am          |   9 ++
 gnulib/import/Makefile.in          |  50 ++++++-----
 gnulib/import/count-one-bits.c     |   7 ++
 gnulib/import/count-one-bits.h     | 136 +++++++++++++++++++++++++++++
 gnulib/import/m4/count-one-bits.m4 |  12 +++
 gnulib/import/m4/gnulib-cache.m4   |   2 +
 gnulib/import/m4/gnulib-comp.m4    |   5 ++
 gnulib/update-gnulib.sh            |   1 +
 15 files changed, 276 insertions(+), 105 deletions(-)
 create mode 100644 gnulib/import/count-one-bits.c
 create mode 100644 gnulib/import/count-one-bits.h
 create mode 100644 gnulib/import/m4/count-one-bits.m4

diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
index fc541332aabd..0c49a77245bf 100644
--- a/gdb/arch/arm-get-next-pcs.c
+++ b/gdb/arch/arm-get-next-pcs.c
@@ -22,6 +22,7 @@
 #include "gdbsupport/common-regcache.h"
 #include "arm.h"
 #include "arm-get-next-pcs.h"
+#include "count-one-bits.h"
 
 /* See arm-get-next-pcs.h.  */
 
@@ -408,8 +409,8 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 
       /* Fetch the saved PC from the stack.  It's stored above
          all of the other registers.  */
-      unsigned long offset = bitcount (bits (inst1, 0, 7))
-			     * ARM_INT_REGISTER_SIZE;
+      unsigned long offset
+	= count_one_bits (bits (inst1, 0, 7)) * ARM_INT_REGISTER_SIZE;
       sp = regcache_raw_get_unsigned (regcache, ARM_SP_REGNUM);
       nextpc = self->ops->read_mem_uint (sp + offset, 4, byte_order);
     }
@@ -496,7 +497,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 	      /* LDMIA or POP */
 	      if (!bit (inst2, 15))
 		load_pc = 0;
-	      offset = bitcount (inst2) * 4 - 4;
+	      offset = count_one_bits (inst2) * 4 - 4;
 	    }
 	  else if (!bit (inst1, 7) && bit (inst1, 8))
 	    {
@@ -864,7 +865,7 @@ arm_get_next_pcs_raw (struct arm_get_next_pcs *self)
 		    {
 		      /* up */
 		      unsigned long reglist = bits (this_instr, 0, 14);
-		      offset = bitcount (reglist) * 4;
+		      offset = count_one_bits_l (reglist) * 4;
 		      if (bit (this_instr, 24))		/* pre */
 			offset += 4;
 		    }
diff --git a/gdb/arch/arm.c b/gdb/arch/arm.c
index 60d9f85889db..faa2b4fbd4b2 100644
--- a/gdb/arch/arm.c
+++ b/gdb/arch/arm.c
@@ -41,17 +41,6 @@ thumb_insn_size (unsigned short inst1)
 
 /* See arm.h.  */
 
-int
-bitcount (unsigned long val)
-{
-  int nbits;
-  for (nbits = 0; val != 0; nbits++)
-    val &= val - 1;		/* Delete rightmost 1-bit in val.  */
-  return nbits;
-}
-
-/* See arm.h.  */
-
 int
 condition_true (unsigned long cond, unsigned long status_reg)
 {
diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index 2d9e87eb428e..b0eb2ae445f2 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -160,9 +160,6 @@ int thumb_insn_size (unsigned short inst1);
 /* Returns true if the condition evaluates to true.  */
 int condition_true (unsigned long cond, unsigned long status_reg);
 
-/* Return number of 1-bits in VAL.  */
-int bitcount (unsigned long val);
-
 /* Return 1 if THIS_INSTR might change control flow, 0 otherwise.  */
 int arm_instruction_changes_pc (uint32_t this_instr);
 
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 4efd7585a0ee..175c5b956e7a 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -45,6 +45,7 @@
 #include "target-descriptions.h"
 #include "user-regs.h"
 #include "observable.h"
+#include "count-one-bits.h"
 
 #include "arch/arm.h"
 #include "arch/arm-get-next-pcs.h"
@@ -5798,7 +5799,8 @@ cleanup_block_store_pc (struct gdbarch *gdbarch, struct regcache *regs,
 {
   uint32_t status = displaced_read_reg (regs, dsc, ARM_PS_REGNUM);
   int store_executed = condition_true (dsc->u.block.cond, status);
-  CORE_ADDR pc_stored_at, transferred_regs = bitcount (dsc->u.block.regmask);
+  CORE_ADDR pc_stored_at, transferred_regs
+    = count_one_bits (dsc->u.block.regmask);
   CORE_ADDR stm_insn_addr;
   uint32_t pc_val;
   long offset;
@@ -5850,7 +5852,7 @@ cleanup_block_load_pc (struct gdbarch *gdbarch,
   uint32_t status = displaced_read_reg (regs, dsc, ARM_PS_REGNUM);
   int load_executed = condition_true (dsc->u.block.cond, status);
   unsigned int mask = dsc->u.block.regmask, write_reg = ARM_PC_REGNUM;
-  unsigned int regs_loaded = bitcount (mask);
+  unsigned int regs_loaded = count_one_bits (mask);
   unsigned int num_to_shuffle = regs_loaded, clobbered;
 
   /* The method employed here will fail if the register list is fully populated
@@ -5982,7 +5984,7 @@ arm_copy_block_xfer (struct gdbarch *gdbarch, uint32_t insn,
 	     contiguous chunk r0...rX before doing the transfer, then shuffling
 	     registers into the correct places in the cleanup routine.  */
 	  unsigned int regmask = insn & 0xffff;
-	  unsigned int num_in_list = bitcount (regmask), new_regmask;
+	  unsigned int num_in_list = count_one_bits (regmask), new_regmask;
 	  unsigned int i;
 
 	  for (i = 0; i < num_in_list; i++)
@@ -6084,7 +6086,7 @@ thumb2_copy_block_xfer (struct gdbarch *gdbarch, uint16_t insn1, uint16_t insn2,
       else
 	{
 	  unsigned int regmask = dsc->u.block.regmask;
-	  unsigned int num_in_list = bitcount (regmask), new_regmask;
+	  unsigned int num_in_list = count_one_bits (regmask), new_regmask;
 	  unsigned int i;
 
 	  for (i = 0; i < num_in_list; i++)
@@ -7102,7 +7104,7 @@ thumb_copy_pop_pc_16bit (struct gdbarch *gdbarch, uint16_t insn1,
     }
   else
     {
-      unsigned int num_in_list = bitcount (dsc->u.block.regmask);
+      unsigned int num_in_list = count_one_bits (dsc->u.block.regmask);
       unsigned int i;
       unsigned int new_regmask;
 
diff --git a/gnulib/Makefile.in b/gnulib/Makefile.in
index 8530f3dcf527..67045edc785c 100644
--- a/gnulib/Makefile.in
+++ b/gnulib/Makefile.in
@@ -1,7 +1,7 @@
 # Makefile.in generated by automake 1.15.1 from Makefile.am.
 # @configure_input@
 
-# Copyright (C) 1994-2020 Free Software Foundation, Inc.
+# Copyright (C) 1994-2017 Free Software Foundation, Inc.
 
 # This Makefile.in is free software; the Free Software Foundation
 # gives unlimited permission to copy and/or distribute it,
@@ -14,7 +14,7 @@
 
 @SET_MAKE@
 
-# Copyright (C) 2019 Free Software Foundation, Inc.
+# Copyright (C) 2019-2020 Free Software Foundation, Inc.
 
 # This file is part of GDB.
 
@@ -123,6 +123,7 @@ am__aclocal_m4_deps = $(top_srcdir)/../config/depstand.m4 \
 	$(top_srcdir)/import/m4/close.m4 \
 	$(top_srcdir)/import/m4/closedir.m4 \
 	$(top_srcdir)/import/m4/codeset.m4 \
+	$(top_srcdir)/import/m4/count-one-bits.m4 \
 	$(top_srcdir)/import/m4/d-ino.m4 \
 	$(top_srcdir)/import/m4/d-type.m4 \
 	$(top_srcdir)/import/m4/dirent_h.m4 \
diff --git a/gnulib/aclocal.m4 b/gnulib/aclocal.m4
index ed1f4f7659c4..4ad6a3dcc867 100644
--- a/gnulib/aclocal.m4
+++ b/gnulib/aclocal.m4
@@ -1195,6 +1195,7 @@ m4_include([import/m4/chdir-long.m4])
 m4_include([import/m4/close.m4])
 m4_include([import/m4/closedir.m4])
 m4_include([import/m4/codeset.m4])
+m4_include([import/m4/count-one-bits.m4])
 m4_include([import/m4/d-ino.m4])
 m4_include([import/m4/d-type.m4])
 m4_include([import/m4/dirent_h.m4])
diff --git a/gnulib/configure b/gnulib/configure
index a0fb5c6b6c6c..90513dc143b9 100644
--- a/gnulib/configure
+++ b/gnulib/configure
@@ -6234,6 +6234,7 @@ fi
   # Code from module cloexec:
   # Code from module close:
   # Code from module closedir:
+  # Code from module count-one-bits:
   # Code from module d-ino:
   # Code from module d-type:
   # Code from module dirent:
@@ -7779,6 +7780,63 @@ $as_echo "#define HAVE_MSVC_INVALID_PARAMETER_HANDLER 1" >>confdefs.h
   REPLACE_FDOPENDIR=0;
 
 
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for unsigned long long int" >&5
+$as_echo_n "checking for unsigned long long int... " >&6; }
+if ${ac_cv_type_unsigned_long_long_int+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_cv_type_unsigned_long_long_int=yes
+     if test "x${ac_cv_prog_cc_c99-no}" = xno; then
+       cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+  /* For now, do not test the preprocessor; as of 2007 there are too many
+         implementations with broken preprocessors.  Perhaps this can
+         be revisited in 2012.  In the meantime, code should not expect
+         #if to work with literals wider than 32 bits.  */
+      /* Test literals.  */
+      long long int ll = 9223372036854775807ll;
+      long long int nll = -9223372036854775807LL;
+      unsigned long long int ull = 18446744073709551615ULL;
+      /* Test constant expressions.   */
+      typedef int a[((-9223372036854775807LL < 0 && 0 < 9223372036854775807ll)
+                     ? 1 : -1)];
+      typedef int b[(18446744073709551615ULL <= (unsigned long long int) -1
+                     ? 1 : -1)];
+      int i = 63;
+int
+main ()
+{
+/* Test availability of runtime routines for shift and division.  */
+      long long int llmax = 9223372036854775807ll;
+      unsigned long long int ullmax = 18446744073709551615ull;
+      return ((ll << 63) | (ll >> 63) | (ll < i) | (ll > i)
+              | (llmax / ll) | (llmax % ll)
+              | (ull << 63) | (ull >> 63) | (ull << i) | (ull >> i)
+              | (ullmax / ull) | (ullmax % ull));
+  ;
+  return 0;
+}
+
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+
+else
+  ac_cv_type_unsigned_long_long_int=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+     fi
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_type_unsigned_long_long_int" >&5
+$as_echo "$ac_cv_type_unsigned_long_long_int" >&6; }
+  if test $ac_cv_type_unsigned_long_long_int = yes; then
+
+$as_echo "#define HAVE_UNSIGNED_LONG_LONG_INT 1" >>confdefs.h
+
+  fi
+
+
 
 
 
@@ -11015,63 +11073,6 @@ $as_echo "$gl_cv_type_wint_t_too_small" >&6; }
 
 
 
-  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for unsigned long long int" >&5
-$as_echo_n "checking for unsigned long long int... " >&6; }
-if ${ac_cv_type_unsigned_long_long_int+:} false; then :
-  $as_echo_n "(cached) " >&6
-else
-  ac_cv_type_unsigned_long_long_int=yes
-     if test "x${ac_cv_prog_cc_c99-no}" = xno; then
-       cat confdefs.h - <<_ACEOF >conftest.$ac_ext
-/* end confdefs.h.  */
-
-  /* For now, do not test the preprocessor; as of 2007 there are too many
-         implementations with broken preprocessors.  Perhaps this can
-         be revisited in 2012.  In the meantime, code should not expect
-         #if to work with literals wider than 32 bits.  */
-      /* Test literals.  */
-      long long int ll = 9223372036854775807ll;
-      long long int nll = -9223372036854775807LL;
-      unsigned long long int ull = 18446744073709551615ULL;
-      /* Test constant expressions.   */
-      typedef int a[((-9223372036854775807LL < 0 && 0 < 9223372036854775807ll)
-                     ? 1 : -1)];
-      typedef int b[(18446744073709551615ULL <= (unsigned long long int) -1
-                     ? 1 : -1)];
-      int i = 63;
-int
-main ()
-{
-/* Test availability of runtime routines for shift and division.  */
-      long long int llmax = 9223372036854775807ll;
-      unsigned long long int ullmax = 18446744073709551615ull;
-      return ((ll << 63) | (ll >> 63) | (ll < i) | (ll > i)
-              | (llmax / ll) | (llmax % ll)
-              | (ull << 63) | (ull >> 63) | (ull << i) | (ull >> i)
-              | (ullmax / ull) | (ullmax % ull));
-  ;
-  return 0;
-}
-
-_ACEOF
-if ac_fn_c_try_link "$LINENO"; then :
-
-else
-  ac_cv_type_unsigned_long_long_int=no
-fi
-rm -f core conftest.err conftest.$ac_objext \
-    conftest$ac_exeext conftest.$ac_ext
-     fi
-fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_type_unsigned_long_long_int" >&5
-$as_echo "$ac_cv_type_unsigned_long_long_int" >&6; }
-  if test $ac_cv_type_unsigned_long_long_int = yes; then
-
-$as_echo "#define HAVE_UNSIGNED_LONG_LONG_INT 1" >>confdefs.h
-
-  fi
-
-
 
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking for long long int" >&5
 $as_echo_n "checking for long long int... " >&6; }
@@ -16279,6 +16280,9 @@ $as_echo "#define GNULIB_TEST_CLOSEDIR 1" >>confdefs.h
 
 
 
+
+
+
       { $as_echo "$as_me:${as_lineno-$LINENO}: checking for d_ino member in directory struct" >&5
 $as_echo_n "checking for d_ino member in directory struct... " >&6; }
 if ${gl_cv_struct_dirent_d_ino+:} false; then :
diff --git a/gnulib/import/Makefile.am b/gnulib/import/Makefile.am
index 4ddaf4fb5836..094447360b63 100644
--- a/gnulib/import/Makefile.am
+++ b/gnulib/import/Makefile.am
@@ -35,6 +35,7 @@
 #  --no-vc-files \
 #  alloca \
 #  canonicalize-lgpl \
+#  count-one-bits \
 #  dirent \
 #  dirfd \
 #  errno \
@@ -236,6 +237,14 @@ EXTRA_libgnu_a_SOURCES += closedir.c
 
 ## end   gnulib module closedir
 
+## begin gnulib module count-one-bits
+
+libgnu_a_SOURCES += count-one-bits.c
+
+EXTRA_DIST += count-one-bits.h
+
+## end   gnulib module count-one-bits
+
 ## begin gnulib module dirent
 
 BUILT_SOURCES += dirent.h
diff --git a/gnulib/import/Makefile.in b/gnulib/import/Makefile.in
index 7059d4c361d0..f5ad783ddb2f 100644
--- a/gnulib/import/Makefile.in
+++ b/gnulib/import/Makefile.in
@@ -49,6 +49,7 @@
 #  --no-vc-files \
 #  alloca \
 #  canonicalize-lgpl \
+#  count-one-bits \
 #  dirent \
 #  dirfd \
 #  errno \
@@ -178,6 +179,7 @@ am__aclocal_m4_deps = $(top_srcdir)/../config/depstand.m4 \
 	$(top_srcdir)/import/m4/close.m4 \
 	$(top_srcdir)/import/m4/closedir.m4 \
 	$(top_srcdir)/import/m4/codeset.m4 \
+	$(top_srcdir)/import/m4/count-one-bits.m4 \
 	$(top_srcdir)/import/m4/d-ino.m4 \
 	$(top_srcdir)/import/m4/d-type.m4 \
 	$(top_srcdir)/import/m4/dirent_h.m4 \
@@ -326,14 +328,15 @@ am__v_AR_1 =
 libgnu_a_AR = $(AR) $(ARFLAGS)
 am__DEPENDENCIES_1 =
 am__dirstamp = $(am__leading_dot)dirstamp
-am_libgnu_a_OBJECTS = cloexec.$(OBJEXT) dirname-lgpl.$(OBJEXT) \
-	basename-lgpl.$(OBJEXT) stripslash.$(OBJEXT) \
-	exitfail.$(OBJEXT) fd-hook.$(OBJEXT) fd-safer-flag.$(OBJEXT) \
-	dup-safer-flag.$(OBJEXT) filenamecat-lgpl.$(OBJEXT) \
-	getprogname.$(OBJEXT) hard-locale.$(OBJEXT) \
-	localcharset.$(OBJEXT) glthread/lock.$(OBJEXT) \
-	malloca.$(OBJEXT) math.$(OBJEXT) openat-die.$(OBJEXT) \
-	save-cwd.$(OBJEXT) malloc/scratch_buffer_grow.$(OBJEXT) \
+am_libgnu_a_OBJECTS = cloexec.$(OBJEXT) count-one-bits.$(OBJEXT) \
+	dirname-lgpl.$(OBJEXT) basename-lgpl.$(OBJEXT) \
+	stripslash.$(OBJEXT) exitfail.$(OBJEXT) fd-hook.$(OBJEXT) \
+	fd-safer-flag.$(OBJEXT) dup-safer-flag.$(OBJEXT) \
+	filenamecat-lgpl.$(OBJEXT) getprogname.$(OBJEXT) \
+	hard-locale.$(OBJEXT) localcharset.$(OBJEXT) \
+	glthread/lock.$(OBJEXT) malloca.$(OBJEXT) math.$(OBJEXT) \
+	openat-die.$(OBJEXT) save-cwd.$(OBJEXT) \
+	malloc/scratch_buffer_grow.$(OBJEXT) \
 	malloc/scratch_buffer_grow_preserve.$(OBJEXT) \
 	malloc/scratch_buffer_set_array_size.$(OBJEXT) \
 	stat-time.$(OBJEXT) strnlen1.$(OBJEXT) sys_socket.$(OBJEXT) \
@@ -1620,15 +1623,15 @@ noinst_LTLIBRARIES =
 EXTRA_DIST = m4/gnulib-cache.m4 alloca.c alloca.in.h arpa_inet.in.h \
 	assure.h openat-priv.h openat-proc.c canonicalize-lgpl.c \
 	chdir-long.c chdir-long.h cloexec.h close.c closedir.c \
-	dirent-private.h dirent.in.h dirfd.c dirname.h dosname.h dup.c \
-	dup2.c errno.in.h error.c error.h exitfail.h fchdir.c fcntl.c \
-	fcntl.in.h fd-hook.h fdopendir.c filename.h filenamecat.h \
-	flexmember.h float.c float.in.h itold.c fnmatch.c \
-	fnmatch_loop.c fnmatch.in.h fpucw.h frexp.c frexp.c frexpl.c \
-	fstat.c stat-w32.c stat-w32.h at-func.c fstatat.c getcwd.c \
-	getcwd-lgpl.c getdtablesize.c getlogin_r.c gettimeofday.c \
-	glob.c glob_internal.h glob_pattern_p.c globfree.c glob-libc.h \
-	glob.in.h hard-locale.h \
+	dirent-private.h count-one-bits.h dirent.in.h dirfd.c \
+	dirname.h dosname.h dup.c dup2.c errno.in.h error.c error.h \
+	exitfail.h fchdir.c fcntl.c fcntl.in.h fd-hook.h fdopendir.c \
+	filename.h filenamecat.h flexmember.h float.c float.in.h \
+	itold.c fnmatch.c fnmatch_loop.c fnmatch.in.h fpucw.h frexp.c \
+	frexp.c frexpl.c fstat.c stat-w32.c stat-w32.h at-func.c \
+	fstatat.c getcwd.c getcwd-lgpl.c getdtablesize.c getlogin_r.c \
+	gettimeofday.c glob.c glob_internal.h glob_pattern_p.c \
+	globfree.c glob-libc.h glob.in.h hard-locale.h \
 	$(top_srcdir)/import/extra/config.rpath inet_ntop.c intprops.h \
 	inttypes.in.h float+.h isnan.c isnand-nolibm.h isnand.c \
 	float+.h isnan.c isnanl-nolibm.h isnanl.c cdefs.h \
@@ -1682,12 +1685,12 @@ DISTCLEANFILES =
 MAINTAINERCLEANFILES = 
 AM_CPPFLAGS = 
 AM_CFLAGS = 
-libgnu_a_SOURCES = cloexec.c dirname-lgpl.c basename-lgpl.c \
-	stripslash.c exitfail.c fd-hook.c fd-safer-flag.c \
-	dup-safer-flag.c filenamecat-lgpl.c getprogname.h \
-	getprogname.c gettext.h hard-locale.c localcharset.c \
-	glthread/lock.h glthread/lock.c malloca.c math.c openat-die.c \
-	save-cwd.c malloc/scratch_buffer_grow.c \
+libgnu_a_SOURCES = cloexec.c count-one-bits.c dirname-lgpl.c \
+	basename-lgpl.c stripslash.c exitfail.c fd-hook.c \
+	fd-safer-flag.c dup-safer-flag.c filenamecat-lgpl.c \
+	getprogname.h getprogname.c gettext.h hard-locale.c \
+	localcharset.c glthread/lock.h glthread/lock.c malloca.c \
+	math.c openat-die.c save-cwd.c malloc/scratch_buffer_grow.c \
 	malloc/scratch_buffer_grow_preserve.c \
 	malloc/scratch_buffer_set_array_size.c stat-time.c strnlen1.h \
 	strnlen1.c sys_socket.c tempname.c glthread/threadlib.c \
@@ -1827,6 +1830,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/cloexec.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/close.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/closedir.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/count-one-bits.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/dirfd.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/dirname-lgpl.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/dup-safer-flag.Po@am__quote@
diff --git a/gnulib/import/count-one-bits.c b/gnulib/import/count-one-bits.c
new file mode 100644
index 000000000000..66341d77cda8
--- /dev/null
+++ b/gnulib/import/count-one-bits.c
@@ -0,0 +1,7 @@
+#include <config.h>
+#define COUNT_ONE_BITS_INLINE _GL_EXTERN_INLINE
+#include "count-one-bits.h"
+
+#if 1500 <= _MSC_VER && (defined _M_IX86 || defined _M_X64)
+int popcount_support = -1;
+#endif
diff --git a/gnulib/import/count-one-bits.h b/gnulib/import/count-one-bits.h
new file mode 100644
index 000000000000..00569941885d
--- /dev/null
+++ b/gnulib/import/count-one-bits.h
@@ -0,0 +1,136 @@
+/* count-one-bits.h -- counts the number of 1-bits in a word.
+   Copyright (C) 2007-2019 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+/* Written by Ben Pfaff.  */
+
+#ifndef COUNT_ONE_BITS_H
+#define COUNT_ONE_BITS_H 1
+
+#include <limits.h>
+#include <stdlib.h>
+
+#ifndef _GL_INLINE_HEADER_BEGIN
+ #error "Please include config.h first."
+#endif
+_GL_INLINE_HEADER_BEGIN
+#ifndef COUNT_ONE_BITS_INLINE
+# define COUNT_ONE_BITS_INLINE _GL_INLINE
+#endif
+
+/* Expand to code that computes the number of 1-bits of the local
+   variable 'x' of type TYPE (an unsigned integer type) and return it
+   from the current function.  */
+#define COUNT_ONE_BITS_GENERIC(TYPE)                                   \
+    do                                                                  \
+      {                                                                 \
+        int count = 0;                                                  \
+        int bits;                                                       \
+        for (bits = 0; bits < sizeof (TYPE) * CHAR_BIT; bits += 32)     \
+          {                                                             \
+            count += count_one_bits_32 (x);                             \
+            x = x >> 31 >> 1;                                           \
+          }                                                             \
+        return count;                                                   \
+      }                                                                 \
+    while (0)
+
+/* Assuming the GCC builtin is BUILTIN and the MSC builtin is MSC_BUILTIN,
+   expand to code that computes the number of 1-bits of the local
+   variable 'x' of type TYPE (an unsigned integer type) and return it
+   from the current function.  */
+#if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)
+# define COUNT_ONE_BITS(BUILTIN, MSC_BUILTIN, TYPE) return BUILTIN (x)
+#else
+
+/* Compute and return the number of 1-bits set in the least
+   significant 32 bits of X. */
+COUNT_ONE_BITS_INLINE int
+count_one_bits_32 (unsigned int x)
+{
+  x = ((x & 0xaaaaaaaaU) >> 1) + (x & 0x55555555U);
+  x = ((x & 0xccccccccU) >> 2) + (x & 0x33333333U);
+  x = (x >> 16) + (x & 0xffff);
+  x = ((x & 0xf0f0) >> 4) + (x & 0x0f0f);
+  return (x >> 8) + (x & 0x00ff);
+}
+
+# if 1500 <= _MSC_VER && (defined _M_IX86 || defined _M_X64)
+
+/* While gcc falls back to its own generic code if the machine
+   on which it's running doesn't support popcount, with Microsoft's
+   compiler we need to detect and fallback ourselves.  */
+#  pragma intrinsic __cpuid
+#  pragma intrinsic __popcnt
+#  pragma intrinsic __popcnt64
+
+/* Return nonzero if popcount is supported.  */
+
+/* 1 if supported, 0 if not supported, -1 if unknown.  */
+extern int popcount_support;
+
+COUNT_ONE_BITS_INLINE int
+popcount_supported (void)
+{
+  if (popcount_support < 0)
+    {
+      int cpu_info[4];
+      __cpuid (cpu_info, 1);
+      popcount_support = (cpu_info[2] >> 23) & 1;  /* See MSDN.  */
+    }
+  return popcount_support;
+}
+
+#  define COUNT_ONE_BITS(BUILTIN, MSC_BUILTIN, TYPE)    \
+     do                                                 \
+       {                                                \
+         if (popcount_supported ())                     \
+           return MSC_BUILTIN (x);                      \
+         else                                           \
+           COUNT_ONE_BITS_GENERIC (TYPE);               \
+       }                                                \
+     while (0)
+# else
+#  define COUNT_ONE_BITS(BUILTIN, MSC_BUILTIN, TYPE)	\
+     COUNT_ONE_BITS_GENERIC (TYPE)
+# endif
+#endif
+
+/* Compute and return the number of 1-bits set in X. */
+COUNT_ONE_BITS_INLINE int
+count_one_bits (unsigned int x)
+{
+  COUNT_ONE_BITS (__builtin_popcount, __popcnt, unsigned int);
+}
+
+/* Compute and return the number of 1-bits set in X. */
+COUNT_ONE_BITS_INLINE int
+count_one_bits_l (unsigned long int x)
+{
+  COUNT_ONE_BITS (__builtin_popcountl, __popcnt, unsigned long int);
+}
+
+#if HAVE_UNSIGNED_LONG_LONG_INT
+/* Compute and return the number of 1-bits set in X. */
+COUNT_ONE_BITS_INLINE int
+count_one_bits_ll (unsigned long long int x)
+{
+  COUNT_ONE_BITS (__builtin_popcountll, __popcnt64, unsigned long long int);
+}
+#endif
+
+_GL_INLINE_HEADER_END
+
+#endif /* COUNT_ONE_BITS_H */
diff --git a/gnulib/import/m4/count-one-bits.m4 b/gnulib/import/m4/count-one-bits.m4
new file mode 100644
index 000000000000..b4721b549cc3
--- /dev/null
+++ b/gnulib/import/m4/count-one-bits.m4
@@ -0,0 +1,12 @@
+# count-one-bits.m4 serial 3
+dnl Copyright (C) 2007, 2009-2019 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_COUNT_ONE_BITS],
+[
+  dnl We don't need (and can't compile) count_one_bits_ll
+  dnl unless the type 'unsigned long long int' exists.
+  AC_REQUIRE([AC_TYPE_UNSIGNED_LONG_LONG_INT])
+])
diff --git a/gnulib/import/m4/gnulib-cache.m4 b/gnulib/import/m4/gnulib-cache.m4
index 428f5c965f35..03101b642070 100644
--- a/gnulib/import/m4/gnulib-cache.m4
+++ b/gnulib/import/m4/gnulib-cache.m4
@@ -40,6 +40,7 @@
 #  --no-vc-files \
 #  alloca \
 #  canonicalize-lgpl \
+#  count-one-bits \
 #  dirent \
 #  dirfd \
 #  errno \
@@ -79,6 +80,7 @@ gl_LOCAL_DIR([])
 gl_MODULES([
   alloca
   canonicalize-lgpl
+  count-one-bits
   dirent
   dirfd
   errno
diff --git a/gnulib/import/m4/gnulib-comp.m4 b/gnulib/import/m4/gnulib-comp.m4
index be1fd390272b..fe1da67d4c75 100644
--- a/gnulib/import/m4/gnulib-comp.m4
+++ b/gnulib/import/m4/gnulib-comp.m4
@@ -57,6 +57,7 @@ AC_DEFUN([gl_EARLY],
   # Code from module cloexec:
   # Code from module close:
   # Code from module closedir:
+  # Code from module count-one-bits:
   # Code from module d-ino:
   # Code from module d-type:
   # Code from module dirent:
@@ -250,6 +251,7 @@ AC_DEFUN([gl_INIT],
     AC_LIBOBJ([closedir])
   fi
   gl_DIRENT_MODULE_INDICATOR([closedir])
+  gl_COUNT_ONE_BITS
   gl_CHECK_TYPE_STRUCT_DIRENT_D_INO
   gl_CHECK_TYPE_STRUCT_DIRENT_D_TYPE
   gl_DIRENT_H
@@ -855,6 +857,8 @@ AC_DEFUN([gl_FILE_LIST], [
   lib/cloexec.h
   lib/close.c
   lib/closedir.c
+  lib/count-one-bits.c
+  lib/count-one-bits.h
   lib/dirent-private.h
   lib/dirent.in.h
   lib/dirfd.c
@@ -1045,6 +1049,7 @@ AC_DEFUN([gl_FILE_LIST], [
   m4/close.m4
   m4/closedir.m4
   m4/codeset.m4
+  m4/count-one-bits.m4
   m4/d-ino.m4
   m4/d-type.m4
   m4/dirent_h.m4
diff --git a/gnulib/update-gnulib.sh b/gnulib/update-gnulib.sh
index 6fa7a56ce2f1..5e4608096864 100755
--- a/gnulib/update-gnulib.sh
+++ b/gnulib/update-gnulib.sh
@@ -32,6 +32,7 @@
 IMPORTED_GNULIB_MODULES="\
     alloca \
     canonicalize-lgpl \
+    count-one-bits \
     dirent \
     dirfd \
     errno \
-- 
2.25.0

[PATCH 5/5] gdb: change print format of flag enums with value 0

[Simon Marchi]

If a flag enum has value 0 and the enumeration type does not have an
enumerator with value 0, we currently print:

  $1 = (unknown: 0x0)

I don't like the display of "unknown" here, since for flags, 0 is a
an expected value.  It just means that no flags are set.  This patch
makes it so that we print it as a simple 0 in this situation:

  $1 = 0

If there is an enumerator with value 0, it is still printed using that
enumerator, for example (from the test):

  $1 = FE_NONE

gdb/ChangeLog:

	* valprint.c (generic_val_print_enum_1): When printing a flag
	enum with value 0 and there is no enumerator with value 0, print
	just "0" instead of "(unknown: 0x0)".

gdb/testsuite/ChangeLog:

	* gdb.base/printcmds.exp (test_print_enums): Update expected
	output.
---
 gdb/testsuite/gdb.base/printcmds.exp |  2 +-
 gdb/valprint.c                       | 31 +++++++++++++++++++++-------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index d6f5c75650bf..bd2afc8696f0 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -743,7 +743,7 @@ proc test_print_enums {} {
     gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"]
 
     # Print a flag enum with value 0, where no enumerator has value 0.
-    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0x0)"]
+    gdb_test "print flag_enum_without_zero" [string_to_regexp " = 0"]
 
     # Print a flag enum with unknown bits set.
     gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 0xf0)"]
diff --git a/gdb/valprint.c b/gdb/valprint.c
index bd21be69e1bf..6e62d420c0f4 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -634,7 +634,6 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
       /* We have a "flag" enum, so we try to decompose it into
 	 pieces as appropriate.  A flag enum has disjoint
 	 constants by definition.  */
-      fputs_filtered ("(", stream);
       for (i = 0; i < len; ++i)
 	{
 	  QUIT;
@@ -646,24 +645,42 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
 
 	  if ((val & enumval) != 0)
 	    {
-	      if (!first)
+	      if (first)
+		{
+		  fputs_filtered ("(", stream);
+		  first = 0;
+		}
+	      else
 		fputs_filtered (" | ", stream);
-	      first = 0;
 
 	      val &= ~TYPE_FIELD_ENUMVAL (type, i);
 	      fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
 	    }
 	}
 
-      if (first || val != 0)
+      if (val != 0)
 	{
-	  if (!first)
+	  /* There are leftover bits, print them.  */
+	  if (first)
+	    fputs_filtered ("(", stream);
+	  else
 	    fputs_filtered (" | ", stream);
+
 	  fputs_filtered ("unknown: 0x", stream);
 	  print_longest (stream, 'x', 0, val);
+	  fputs_filtered (")", stream);
+	}
+      else if (first)
+	{
+	  /* Nothing has been printed and the value is 0, the enum value must
+	     have been 0.  */
+	  fputs_filtered ("0", stream);
+	}
+      else
+	{
+	  /* Something has been printed, close the parenthesis.  */
+	  fputs_filtered (")", stream);
 	}
-
-      fputs_filtered (")", stream);
     }
   else
     print_longest (stream, 'd', 0, val);
-- 
2.25.0

[PATCH 3/5] gdb: allow duplicate enumerators in flag enums

[Simon Marchi]

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(-)

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;
-- 
2.25.0

[PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators

[Simon Marchi]

GDB has this feature where if an enum looks like it is meant to
represent binary flags, it will present the values of that type as a
bitwise OR of the flags that are set in the value.

The original motivation for this patch is to fix this behavior:

  enum hello { AAA = 0x1, BBB = 0xf0 };

  (gdb) p (enum hello) 0x11
  $1 = (AAA | BBB)

This is wrong because the bits set in BBB (0xf0) are not all set in the
value 0x11, but GDB presents it as if they all were.

I think that enumerations with enumerators that have more than one bit
set should simply not qualify as "flag enum", as far as this
heuristic is concerned.  I'm not sure what it means to have flags of
more than one bit.  So this is what this patch implements.

I have added an assert in generic_val_print_enum_1 to make sure the flag
enum types respect that, in case they are used by other debug info
readers, in the future.

I've enhanced the gdb.base/printcmds.exp test to cover this case.  I've
also added tests for printing flag enums with value 0, both when the
enumeration has and doesn't have an enumerator for value 0.

gdb/ChangeLog:

	* dwarf2/read.c: Include "count-one-bits.h".
	(update_enumeration_type_from_children): If an enumerator has
	multiple bits set, don't treat the enumeration as a "flag enum".
	* valprint.c (generic_val_print_enum_1): Assert that enumerators
	of flag enums have 0 or 1 bit set.

gdb/testsuite/ChangeLog:

	* gdb.base/printcmds.c (enum flag_enum): Prefix enumerators with
	FE_, add FE_NONE.
	(three): Update.
	(enum flag_enum_without_zero): New enum.
	(flag_enum_without_zero): New variable.
	(enum not_flag_enum): New enum.
	(three_not_flag): New variable.
	* gdb.base/printcmds.exp (test_artificial_arrays): Update.
	(test_print_enums): Add more tests for printing flag enums.
---
 gdb/dwarf2/read.c                    | 14 ++++++++++---
 gdb/testsuite/gdb.base/printcmds.c   | 30 ++++++++++++++++++++++++++--
 gdb/testsuite/gdb.base/printcmds.exp | 20 ++++++++++++++++---
 gdb/valprint.c                       |  8 +++++++-
 4 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 7edbd9d7dfa4..b866cc2d5747 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -82,6 +82,7 @@
 #include "gdbsupport/selftest.h"
 #include "rust-lang.h"
 #include "gdbsupport/pathstuff.h"
+#include "count-one-bits.h"
 
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
@@ -15526,10 +15527,17 @@ update_enumeration_type_from_children (struct die_info *die,
 	  unsigned_enum = 0;
 	  flag_enum = 0;
 	}
-      else if ((mask & value) != 0)
-	flag_enum = 0;
       else
-	mask |= value;
+	{
+	  int nbits = count_one_bits_ll (value);
+
+	  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
 	 a flag type, no need to look at the rest of the enumerates.  */
diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
index 57e04e6c01f3..f0b4fa4b86b1 100644
--- a/gdb/testsuite/gdb.base/printcmds.c
+++ b/gdb/testsuite/gdb.base/printcmds.c
@@ -96,9 +96,35 @@ enum some_volatile_enum { enumvolval1, enumvolval2 };
    name.  See PR11827.  */
 volatile enum some_volatile_enum some_volatile_enum = enumvolval1;
 
-enum flag_enum { ONE = 1, TWO = 2 };
+/* An enum considered as a "flag enum".  */
+enum flag_enum
+{
+  FE_NONE = 0x00,
+  FE_ONE  = 0x01,
+  FE_TWO  = 0x02,
+};
+
+enum flag_enum three = FE_ONE | FE_TWO;
+
+/* Another enum considered as a "flag enum", but with enumerator with value
+   0.  */
+enum flag_enum_without_zero
+{
+  FEWZ_ONE = 0x01,
+  FEWZ_TWO = 0x02,
+};
+
+enum flag_enum_without_zero flag_enum_without_zero = 0;
+
+/* Not a flag enum, an enumerator value has multiple bits sets.  */
+enum not_flag_enum
+{
+  NFE_ONE = 0x01,
+  NFE_TWO = 0x02,
+  NFE_F0  = 0xf0,
+};
 
-enum flag_enum three = ONE | TWO;
+enum not_flag_enum three_not_flag = NFE_ONE | NFE_TWO;
 
 /* A structure with an embedded array at an offset > 0.  The array has
    all elements with the same repeating value, which must not be the
diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index 6e98b7943ba3..6afb965af066 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -653,9 +653,9 @@ proc test_artificial_arrays {} {
     gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@2${ctrlv}@3" \
 	"({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \
 	{p int1dim[0]@2@3}
-    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO" " = {0, 1}" \
+    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO" " = {0, 1}" \
         {p int1dim[0]@TWO}
-    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO${ctrlv}@three" \
+    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO${ctrlv}@three" \
 	"({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \
 	{p int1dim[0]@TWO@three}
     gdb_test_escape_braces {p/x (short [])0x12345678} \
@@ -736,7 +736,21 @@ proc test_print_enums {} {
     # Regression test for PR11827.
     gdb_test "print some_volatile_enum" "enumvolval1"
 
-    gdb_test "print three" " = \\\(ONE \\| TWO\\\)"
+    # Print a flag enum.
+    gdb_test "print three" [string_to_regexp " = (FE_ONE | FE_TWO)"]
+
+    # Print a flag enum with value 0, where an enumerator has value 0.
+    gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"]
+
+    # Print a flag enum with value 0, where no enumerator has value 0.
+    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0)"]
+
+    # Print a flag enum with unknown bits set.
+    gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 240)"]
+
+    # Test printing an enum not considered a "flag enum" (because one of its
+    # enumerators has multiple bits set).
+    gdb_test "print three_not_flag" [string_to_regexp " = 3"]
 }
 
 proc test_printf {} {
diff --git a/gdb/valprint.c b/gdb/valprint.c
index f26a87da3bd4..77b9a4993d79 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -39,6 +39,7 @@
 #include "cli/cli-option.h"
 #include "gdbarch.h"
 #include "cli/cli-style.h"
+#include "count-one-bits.h"
 
 /* Maximum number of wchars returned from wchar_iterate.  */
 #define MAX_WCHARS 4
@@ -638,7 +639,12 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
 	{
 	  QUIT;
 
-	  if ((val & TYPE_FIELD_ENUMVAL (type, i)) != 0)
+	  ULONGEST enumval = TYPE_FIELD_ENUMVAL (type, i);
+	  int nbits = count_one_bits_ll (enumval);
+
+	  gdb_assert (nbits == 0 || nbits == 1);
+
+	  if ((val & enumval) != 0)
 	    {
 	      if (!first)
 		fputs_filtered (" | ", stream);
-- 
2.25.0

[PATCH 4/5] gdb: print unknown part of flag enum in hex

[Simon Marchi]

When we print the "unknown" part of a flag enum, it is printed in
decimal.  I think it would be more useful if it was printed in hex, as
it helps to determine which bits are set more than a decimal value.

gdb/ChangeLog:

	* valprint.c (generic_val_print_enum_1): Print unknown part of
	flag enum in hex.

gdb/testsuite/ChangeLog:

	* gdb.base/printcmds.exp (test_print_enums): Expect hex values
	for "unknown".
---
 gdb/testsuite/gdb.base/printcmds.exp | 4 ++--
 gdb/valprint.c                       | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index 6afb965af066..d6f5c75650bf 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -743,10 +743,10 @@ proc test_print_enums {} {
     gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"]
 
     # Print a flag enum with value 0, where no enumerator has value 0.
-    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0)"]
+    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0x0)"]
 
     # Print a flag enum with unknown bits set.
-    gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 240)"]
+    gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 0xf0)"]
 
     # Test printing an enum not considered a "flag enum" (because one of its
     # enumerators has multiple bits set).
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 77b9a4993d79..bd21be69e1bf 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -659,8 +659,8 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
 	{
 	  if (!first)
 	    fputs_filtered (" | ", stream);
-	  fputs_filtered ("unknown: ", stream);
-	  print_longest (stream, 'd', 0, val);
+	  fputs_filtered ("unknown: 0x", stream);
+	  print_longest (stream, 'x', 0, val);
 	}
 
       fputs_filtered (")", stream);
-- 
2.25.0

Re: [PATCH 1/5] gnulib: import count-one-bits module and use it

[Simon Marchi]

On 2020-02-13 3:30 p.m., Simon Marchi wrote:
> In a following patch, I need a function that counts the number of set
> bits in a word.  There is  __builtin_popcount that is supported by gcc
> and clang, but there is also a gnulib module that wraps it and provides
> a fallback for other compilers, so we might as well use that.
> 
> I also noticed that there is a bitcount function in arch/arm.c, so as a
> first step, this patch replaces the uses of that function with gnulib's
> function.
> 
> The gnulib module actually provides multiple functions, with various
> parameter length (unsigned int, unsigned long int, unsigned long long
> int), I chose the one that made sense for each call site based on the
> argument type.
> 
> gnulib/ChangeLog:
> 
> 	* update-gnulib.sh (IMPORTED_GNULIB_MODULES): Import
> 	count-one-bits module.
> 	* configure: Re-generate.
> 	* aclocal.m4: Re-generate.
> 	* Makefile.in: Re-generate.
> 	* import/count-one-bits.c: New file.
> 	* import/count-one-bits.h: New file.
> 	* import/Makefile.am: Re-generate.
> 	* import/Makefile.in: Re-generate.
> 	* import/m4/gnulib-cache.m4: Re-generate.
> 	* import/m4/gnulib-comp.m4: Re-generate.
> 	* import/m4/count-one-bits.m4: New file.
> 
> gdb/ChangeLog:
> 
> 	* arm-tdep.c: Include count-one-bits.h.
> 	(cleanup_block_store_pc): Use count_one_bits.
> 	(cleanup_block_load_pc): Use count_one_bits.
> 	(arm_copy_block_xfer): Use count_one_bits.
> 	(thumb2_copy_block_xfer): Use count_one_bits.
> 	(thumb_copy_pop_pc_16bit): Use count_one_bits.
> 	* arch/arm-get-next-pcs.c: Include count-one-bits.h.
> 	(thumb_get_next_pcs_raw): Use count_one_bits.
> 	(arm_get_next_pcs_raw): Use count_one_bits_l.
> 	* arch/arm.c (bitcount): Remove.
> 	* arch/arm.h (bitcount): Remove.

Sorry, I had forgotten that I had sent this patch separately from this series:

  https://sourceware.org/ml/gdb-patches/2020-02/msg00410.html

And it is now merged, so you can ignore patch 1/5.

Simon

Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators

[Luis Machado]

On 2/13/20 5:30 PM, Simon Marchi wrote:
> GDB has this feature where if an enum looks like it is meant to
> represent binary flags, it will present the values of that type as a
> bitwise OR of the flags that are set in the value.
> 
> The original motivation for this patch is to fix this behavior:
> 
>    enum hello { AAA = 0x1, BBB = 0xf0 };
> 
>    (gdb) p (enum hello) 0x11
>    $1 = (AAA | BBB)
> 
> This is wrong because the bits set in BBB (0xf0) are not all set in the
> value 0x11, but GDB presents it as if they all were.
> 
> I think that enumerations with enumerators that have more than one bit
> set should simply not qualify as "flag enum", as far as this
> heuristic is concerned.  I'm not sure what it means to have flags of
> more than one bit.  So this is what this patch implements.
> 
> I have added an assert in generic_val_print_enum_1 to make sure the flag
> enum types respect that, in case they are used by other debug info
> readers, in the future.
> 
> I've enhanced the gdb.base/printcmds.exp test to cover this case.  I've
> also added tests for printing flag enums with value 0, both when the
> enumeration has and doesn't have an enumerator for value 0.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/read.c: Include "count-one-bits.h".
> 	(update_enumeration_type_from_children): If an enumerator has
> 	multiple bits set, don't treat the enumeration as a "flag enum".
> 	* valprint.c (generic_val_print_enum_1): Assert that enumerators
> 	of flag enums have 0 or 1 bit set.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/printcmds.c (enum flag_enum): Prefix enumerators with
> 	FE_, add FE_NONE.
> 	(three): Update.
> 	(enum flag_enum_without_zero): New enum.
> 	(flag_enum_without_zero): New variable.
> 	(enum not_flag_enum): New enum.
> 	(three_not_flag): New variable.
> 	* gdb.base/printcmds.exp (test_artificial_arrays): Update.
> 	(test_print_enums): Add more tests for printing flag enums.
> ---
>   gdb/dwarf2/read.c                    | 14 ++++++++++---
>   gdb/testsuite/gdb.base/printcmds.c   | 30 ++++++++++++++++++++++++++--
>   gdb/testsuite/gdb.base/printcmds.exp | 20 ++++++++++++++++---
>   gdb/valprint.c                       |  8 +++++++-
>   4 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 7edbd9d7dfa4..b866cc2d5747 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -82,6 +82,7 @@
>   #include "gdbsupport/selftest.h"
>   #include "rust-lang.h"
>   #include "gdbsupport/pathstuff.h"
> +#include "count-one-bits.h"
>   
>   /* When == 1, print basic high level tracing messages.
>      When > 1, be more verbose.
> @@ -15526,10 +15527,17 @@ update_enumeration_type_from_children (struct die_info *die,
>   	  unsigned_enum = 0;
>   	  flag_enum = 0;
>   	}
> -      else if ((mask & value) != 0)
> -	flag_enum = 0;
>         else
> -	mask |= value;
> +	{
> +	  int nbits = count_one_bits_ll (value);
> +
> +	  if (nbits != 0 && nbits && nbits != 1)

Isn't this the same as nbits >= 2? popcount shouldn't return a negative 
number, should it?

> +	    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
>   	 a flag type, no need to look at the rest of the enumerates.  */
> diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
> index 57e04e6c01f3..f0b4fa4b86b1 100644
> --- a/gdb/testsuite/gdb.base/printcmds.c
> +++ b/gdb/testsuite/gdb.base/printcmds.c
> @@ -96,9 +96,35 @@ enum some_volatile_enum { enumvolval1, enumvolval2 };
>      name.  See PR11827.  */
>   volatile enum some_volatile_enum some_volatile_enum = enumvolval1;
>   
> -enum flag_enum { ONE = 1, TWO = 2 };
> +/* An enum considered as a "flag enum".  */
> +enum flag_enum
> +{
> +  FE_NONE = 0x00,
> +  FE_ONE  = 0x01,
> +  FE_TWO  = 0x02,
> +};
> +
> +enum flag_enum three = FE_ONE | FE_TWO;
> +
> +/* Another enum considered as a "flag enum", but with enumerator with value
> +   0.  */
> +enum flag_enum_without_zero
> +{
> +  FEWZ_ONE = 0x01,
> +  FEWZ_TWO = 0x02,
> +};
> +

Typo maybe? There is no enum with value 0 in flag_enum_without_zero. 
Maybe you meant flag_enum to contain a 0 value with FE_NONE?

> +enum flag_enum_without_zero flag_enum_without_zero = 0;
> +

Or maybe you were referring to the above?

> +/* Not a flag enum, an enumerator value has multiple bits sets.  */
> +enum not_flag_enum
> +{
> +  NFE_ONE = 0x01,
> +  NFE_TWO = 0x02,
> +  NFE_F0  = 0xf0,
> +};
>   
> -enum flag_enum three = ONE | TWO;
> +enum not_flag_enum three_not_flag = NFE_ONE | NFE_TWO;
>   
>   /* A structure with an embedded array at an offset > 0.  The array has
>      all elements with the same repeating value, which must not be the
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index 6e98b7943ba3..6afb965af066 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -653,9 +653,9 @@ proc test_artificial_arrays {} {
>       gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@2${ctrlv}@3" \
>   	"({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \
>   	{p int1dim[0]@2@3}
> -    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO" " = {0, 1}" \
> +    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO" " = {0, 1}" \
>           {p int1dim[0]@TWO}
> -    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO${ctrlv}@three" \
> +    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO${ctrlv}@three" \
>   	"({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \
>   	{p int1dim[0]@TWO@three}
>       gdb_test_escape_braces {p/x (short [])0x12345678} \
> @@ -736,7 +736,21 @@ proc test_print_enums {} {
>       # Regression test for PR11827.
>       gdb_test "print some_volatile_enum" "enumvolval1"
>   
> -    gdb_test "print three" " = \\\(ONE \\| TWO\\\)"
> +    # Print a flag enum.
> +    gdb_test "print three" [string_to_regexp " = (FE_ONE | FE_TWO)"]
> +
> +    # Print a flag enum with value 0, where an enumerator has value 0.
> +    gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"]
> +
> +    # Print a flag enum with value 0, where no enumerator has value 0.
> +    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0)"]
> +
> +    # Print a flag enum with unknown bits set.
> +    gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 240)"]
> +
> +    # Test printing an enum not considered a "flag enum" (because one of its
> +    # enumerators has multiple bits set).
> +    gdb_test "print three_not_flag" [string_to_regexp " = 3"]
>   }
>   
>   proc test_printf {} {
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index f26a87da3bd4..77b9a4993d79 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -39,6 +39,7 @@
>   #include "cli/cli-option.h"
>   #include "gdbarch.h"
>   #include "cli/cli-style.h"
> +#include "count-one-bits.h"
>   
>   /* Maximum number of wchars returned from wchar_iterate.  */
>   #define MAX_WCHARS 4
> @@ -638,7 +639,12 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
>   	{
>   	  QUIT;
>   
> -	  if ((val & TYPE_FIELD_ENUMVAL (type, i)) != 0)
> +	  ULONGEST enumval = TYPE_FIELD_ENUMVAL (type, i);
> +	  int nbits = count_one_bits_ll (enumval);
> +
> +	  gdb_assert (nbits == 0 || nbits == 1);
> +
> +	  if ((val & enumval) != 0)
>   	    {
>   	      if (!first)
>   		fputs_filtered (" | ", stream);
> 

Otherwise LGTM.

Re: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums

[Luis Machado]

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.

Re: [PATCH 4/5] gdb: print unknown part of flag enum in hex

[Luis Machado]

On 2/13/20 5:30 PM, Simon Marchi wrote:
> When we print the "unknown" part of a flag enum, it is printed in
> decimal.  I think it would be more useful if it was printed in hex, as
> it helps to determine which bits are set more than a decimal value.
> 

Would it be better to mention the offending bit position explicitly? The 
hex value could be displayed along with it as well.

I mean, in both decimal and hex you'd need to do some calculations to 
figure out the bit position anyway. Might as well make it easier for 
developers by displaying the information.

> gdb/ChangeLog:
> 
> 	* valprint.c (generic_val_print_enum_1): Print unknown part of
> 	flag enum in hex.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/printcmds.exp (test_print_enums): Expect hex values
> 	for "unknown".
> ---
>   gdb/testsuite/gdb.base/printcmds.exp | 4 ++--
>   gdb/valprint.c                       | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index 6afb965af066..d6f5c75650bf 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -743,10 +743,10 @@ proc test_print_enums {} {
>       gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"]
>   
>       # Print a flag enum with value 0, where no enumerator has value 0.
> -    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0)"]
> +    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0x0)"]
>   
>       # Print a flag enum with unknown bits set.
> -    gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 240)"]
> +    gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 0xf0)"]
>   
>       # Test printing an enum not considered a "flag enum" (because one of its
>       # enumerators has multiple bits set).
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index 77b9a4993d79..bd21be69e1bf 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -659,8 +659,8 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
>   	{
>   	  if (!first)
>   	    fputs_filtered (" | ", stream);
> -	  fputs_filtered ("unknown: ", stream);
> -	  print_longest (stream, 'd', 0, val);
> +	  fputs_filtered ("unknown: 0x", stream);
> +	  print_longest (stream, 'x', 0, val);
>   	}
>   
>         fputs_filtered (")", stream);
> 

Re: [PATCH 5/5] gdb: change print format of flag enums with value 0

[Luis Machado]

On 2/13/20 5:30 PM, Simon Marchi wrote:
> If a flag enum has value 0 and the enumeration type does not have an
> enumerator with value 0, we currently print:
> 
>    $1 = (unknown: 0x0)
> 
> I don't like the display of "unknown" here, since for flags, 0 is a
> an expected value.  It just means that no flags are set.  This patch
> makes it so that we print it as a simple 0 in this situation:

Should we print "no flags set" alongside the 0 for this case then?

> 
>    $1 = 0
> 
> If there is an enumerator with value 0, it is still printed using that
> enumerator, for example (from the test):
> 
>    $1 = FE_NONE
> 
> gdb/ChangeLog:
> 
> 	* valprint.c (generic_val_print_enum_1): When printing a flag
> 	enum with value 0 and there is no enumerator with value 0, print
> 	just "0" instead of "(unknown: 0x0)".
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/printcmds.exp (test_print_enums): Update expected
> 	output.
> ---
>   gdb/testsuite/gdb.base/printcmds.exp |  2 +-
>   gdb/valprint.c                       | 31 +++++++++++++++++++++-------
>   2 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index d6f5c75650bf..bd2afc8696f0 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -743,7 +743,7 @@ proc test_print_enums {} {
>       gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"]
>   
>       # Print a flag enum with value 0, where no enumerator has value 0.
> -    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0x0)"]
> +    gdb_test "print flag_enum_without_zero" [string_to_regexp " = 0"]
>   
>       # Print a flag enum with unknown bits set.
>       gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 0xf0)"]
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index bd21be69e1bf..6e62d420c0f4 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -634,7 +634,6 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
>         /* We have a "flag" enum, so we try to decompose it into
>   	 pieces as appropriate.  A flag enum has disjoint
>   	 constants by definition.  */
> -      fputs_filtered ("(", stream);
>         for (i = 0; i < len; ++i)
>   	{
>   	  QUIT;
> @@ -646,24 +645,42 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
>   
>   	  if ((val & enumval) != 0)
>   	    {
> -	      if (!first)
> +	      if (first)
> +		{
> +		  fputs_filtered ("(", stream);
> +		  first = 0;
> +		}
> +	      else
>   		fputs_filtered (" | ", stream);
> -	      first = 0;
>   
>   	      val &= ~TYPE_FIELD_ENUMVAL (type, i);
>   	      fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
>   	    }
>   	}
>   
> -      if (first || val != 0)
> +      if (val != 0)
>   	{
> -	  if (!first)
> +	  /* There are leftover bits, print them.  */
> +	  if (first)
> +	    fputs_filtered ("(", stream);
> +	  else
>   	    fputs_filtered (" | ", stream);
> +
>   	  fputs_filtered ("unknown: 0x", stream);
>   	  print_longest (stream, 'x', 0, val);
> +	  fputs_filtered (")", stream);
> +	}
> +      else if (first)
> +	{
> +	  /* Nothing has been printed and the value is 0, the enum value must
> +	     have been 0.  */
> +	  fputs_filtered ("0", stream);
> +	}
> +      else
> +	{
> +	  /* Something has been printed, close the parenthesis.  */
> +	  fputs_filtered (")", stream);
>   	}
> -
> -      fputs_filtered (")", stream);
>       }
>     else
>       print_longest (stream, 'd', 0, val);
>
Otherwise LGTM.

Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators

[Simon Marchi]

On 2020-02-17 5:56 a.m., Luis Machado wrote:
>> @@ -15526,10 +15527,17 @@ update_enumeration_type_from_children (struct die_info *die,
>>         unsigned_enum = 0;
>>         flag_enum = 0;
>>       }
>> -      else if ((mask & value) != 0)
>> -    flag_enum = 0;
>>         else
>> -    mask |= value;
>> +    {
>> +      int nbits = count_one_bits_ll (value);
>> +
>> +      if (nbits != 0 && nbits && nbits != 1)
> 
> Isn't this the same as nbits >= 2? popcount shouldn't return a negative number, should it?

I think I wrote that because count_one_bits_ll returns a signed int, so I
indeed thought "what if it returns a negative number".  But if it did, there
would be some quite more serious problems, so we probably don't have to think
about it here.  I'll change it as "nbits >= 2".

Oh and there was a spurious "&& nbits" in there.

> 
>> +        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
>>        a flag type, no need to look at the rest of the enumerates.  */
>> diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
>> index 57e04e6c01f3..f0b4fa4b86b1 100644
>> --- a/gdb/testsuite/gdb.base/printcmds.c
>> +++ b/gdb/testsuite/gdb.base/printcmds.c
>> @@ -96,9 +96,35 @@ enum some_volatile_enum { enumvolval1, enumvolval2 };
>>      name.  See PR11827.  */
>>   volatile enum some_volatile_enum some_volatile_enum = enumvolval1;
>>   -enum flag_enum { ONE = 1, TWO = 2 };
>> +/* An enum considered as a "flag enum".  */
>> +enum flag_enum
>> +{
>> +  FE_NONE = 0x00,
>> +  FE_ONE  = 0x01,
>> +  FE_TWO  = 0x02,
>> +};
>> +
>> +enum flag_enum three = FE_ONE | FE_TWO;
>> +
>> +/* Another enum considered as a "flag enum", but with enumerator with value
>> +   0.  */
>> +enum flag_enum_without_zero
>> +{
>> +  FEWZ_ONE = 0x01,
>> +  FEWZ_TWO = 0x02,
>> +};
>> +
> 
> Typo maybe? There is no enum with value 0 in flag_enum_without_zero. Maybe you meant flag_enum to contain a 0 value with FE_NONE?
> 
>> +enum flag_enum_without_zero flag_enum_without_zero = 0;
>> +
> 
> Or maybe you were referring to the above?

Do you mean a typo in the comment, or the type name?  Because there indeed seems
to be a typo, it should read "but with no enumerator with value", not
"but with enumerator with value".

The type name "flag_enum_without_zero" means there is no enumerator that has value
zero, is that clear?

> Otherwise LGTM.

Thanks for your review, I'll likely send a new version.

Simon

Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators

[Luis Machado]

On 2/17/20 2:27 PM, Simon Marchi wrote:
> On 2020-02-17 5:56 a.m., Luis Machado wrote:
>>> @@ -15526,10 +15527,17 @@ update_enumeration_type_from_children (struct die_info *die,
>>>          unsigned_enum = 0;
>>>          flag_enum = 0;
>>>        }
>>> -      else if ((mask & value) != 0)
>>> -    flag_enum = 0;
>>>          else
>>> -    mask |= value;
>>> +    {
>>> +      int nbits = count_one_bits_ll (value);
>>> +
>>> +      if (nbits != 0 && nbits && nbits != 1)
>>
>> Isn't this the same as nbits >= 2? popcount shouldn't return a negative number, should it?
> 
> I think I wrote that because count_one_bits_ll returns a signed int, so I
> indeed thought "what if it returns a negative number".  But if it did, there
> would be some quite more serious problems, so we probably don't have to think
> about it here.  I'll change it as "nbits >= 2".

I did some research and did not see a clear reason why popcount returns 
an int. There was a mention of popcount returning negative for some 
obscure implementation if it was passed a negative number. GCC's 
documentation doesn't make it clear either.

> 
> Oh and there was a spurious "&& nbits" in there.
> 
>>
>>> +        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
>>>         a flag type, no need to look at the rest of the enumerates.  */
>>> diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
>>> index 57e04e6c01f3..f0b4fa4b86b1 100644
>>> --- a/gdb/testsuite/gdb.base/printcmds.c
>>> +++ b/gdb/testsuite/gdb.base/printcmds.c
>>> @@ -96,9 +96,35 @@ enum some_volatile_enum { enumvolval1, enumvolval2 };
>>>       name.  See PR11827.  */
>>>    volatile enum some_volatile_enum some_volatile_enum = enumvolval1;
>>>    -enum flag_enum { ONE = 1, TWO = 2 };
>>> +/* An enum considered as a "flag enum".  */
>>> +enum flag_enum
>>> +{
>>> +  FE_NONE = 0x00,
>>> +  FE_ONE  = 0x01,
>>> +  FE_TWO  = 0x02,
>>> +};
>>> +
>>> +enum flag_enum three = FE_ONE | FE_TWO;
>>> +
>>> +/* Another enum considered as a "flag enum", but with enumerator with value
>>> +   0.  */
>>> +enum flag_enum_without_zero
>>> +{
>>> +  FEWZ_ONE = 0x01,
>>> +  FEWZ_TWO = 0x02,
>>> +};
>>> +
>>
>> Typo maybe? There is no enum with value 0 in flag_enum_without_zero. Maybe you meant flag_enum to contain a 0 value with FE_NONE?
>>
>>> +enum flag_enum_without_zero flag_enum_without_zero = 0;
>>> +
>>
>> Or maybe you were referring to the above?
> 
> Do you mean a typo in the comment, or the type name?  Because there indeed seems
> to be a typo, it should read "but with no enumerator with value", not
> "but with enumerator with value".

Sorry, i should've been more clear. I meant the comment saying "but with 
enumerator with value". You guessed it right.

Re: [PATCH 4/5] gdb: print unknown part of flag enum in hex

[Simon Marchi]

On 2020-02-17 6:04 a.m., Luis Machado wrote:
> On 2/13/20 5:30 PM, Simon Marchi wrote:
>> When we print the "unknown" part of a flag enum, it is printed in
>> decimal.  I think it would be more useful if it was printed in hex, as
>> it helps to determine which bits are set more than a decimal value.
>>
> 
> Would it be better to mention the offending bit position explicitly? The hex value could be displayed along with it as well.
> 
> I mean, in both decimal and hex you'd need to do some calculations to figure out the bit position anyway. Might as well make it easier for developers by displaying the information.

It's much easier to convert between hex and bit positions than decimal and
bit positions, so I wouldn't put decimal and hex on the same level here.

Also, there could be many bits that are set and unknown, so if we mention them
explicitly, it could take a lot of space and become actually less readable.  I
think that the hex notation is the most convenient, it's compact and people are
used to converting between hex and binary on the spot.

Simon

Re: [PATCH 5/5] gdb: change print format of flag enums with value 0

[Simon Marchi]

On 2020-02-17 7:08 a.m., Luis Machado wrote:
> On 2/13/20 5:30 PM, Simon Marchi wrote:
>> If a flag enum has value 0 and the enumeration type does not have an
>> enumerator with value 0, we currently print:
>>
>>    $1 = (unknown: 0x0)
>>
>> I don't like the display of "unknown" here, since for flags, 0 is a
>> an expected value.  It just means that no flags are set.  This patch
>> makes it so that we print it as a simple 0 in this situation:
> 
> Should we print "no flags set" alongside the 0 for this case then?

I don't know, I think that 0 is the natural and standard thing to print
here.  If you had a flags variable in your code, you'd initialize it with

  my_flags = 0;

So if GDB prints that, I think it's clear.

Simon

Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators

[Simon Marchi]

On 2020-02-17 12:40 p.m., Luis Machado wrote:
>> Do you mean a typo in the comment, or the type name?  Because there indeed seems
>> to be a typo, it should read "but with no enumerator with value", not
>> "but with enumerator with value".
> 
> Sorry, i should've been more clear. I meant the comment saying "but with enumerator with value". You guessed it right.

Ok, thanks.  Here is the new version of this patch.


From 3db3fe30c045a020a07a48c8e0123e9e44274d77 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 13 Feb 2020 13:36:53 -0500
Subject: [PATCH] gdb: fix printing of flag enums with multi-bit enumerators

GDB has this feature where if an enum looks like it is meant to
represent binary flags, it will present the values of that type as a
bitwise OR of the flags that are set in the value.

The original motivation for this patch is to fix this behavior:

  enum hello { AAA = 0x1, BBB = 0xf0 };

  (gdb) p (enum hello) 0x11
  $1 = (AAA | BBB)

This is wrong because the bits set in BBB (0xf0) are not all set in the
value 0x11, but GDB presents it as if they all were.

I think that enumerations with enumerators that have more than one bit
set should simply not qualify as "flag enum", as far as this
heuristic is concerned.  I'm not sure what it means to have flags of
more than one bit.  So this is what this patch implements.

I have added an assert in generic_val_print_enum_1 to make sure the flag
enum types respect that, in case they are used by other debug info
readers, in the future.

I've enhanced the gdb.base/printcmds.exp test to cover this case.  I've
also added tests for printing flag enums with value 0, both when the
enumeration has and doesn't have an enumerator for value 0.

gdb/ChangeLog:

	* dwarf2/read.c: Include "count-one-bits.h".
	(update_enumeration_type_from_children): If an enumerator has
	multiple bits set, don't treat the enumeration as a "flag enum".
	* valprint.c (generic_val_print_enum_1): Assert that enumerators
	of flag enums have 0 or 1 bit set.

gdb/testsuite/ChangeLog:

	* gdb.base/printcmds.c (enum flag_enum): Prefix enumerators with
	FE_, add FE_NONE.
	(three): Update.
	(enum flag_enum_without_zero): New enum.
	(flag_enum_without_zero): New variable.
	(enum not_flag_enum): New enum.
	(three_not_flag): New variable.
	* gdb.base/printcmds.exp (test_artificial_arrays): Update.
	(test_print_enums): Add more tests for printing flag enums.
---
 gdb/dwarf2/read.c                    | 12 ++++++++---
 gdb/testsuite/gdb.base/printcmds.c   | 30 ++++++++++++++++++++++++++--
 gdb/testsuite/gdb.base/printcmds.exp | 20 ++++++++++++++++---
 gdb/valprint.c                       |  8 +++++++-
 4 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index e74383e01dcc..5a77b62b5a05 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -82,6 +82,7 @@
 #include "gdbsupport/selftest.h"
 #include "rust-lang.h"
 #include "gdbsupport/pathstuff.h"
+#include "count-one-bits.h"

 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
@@ -15526,10 +15527,15 @@ update_enumeration_type_from_children (struct die_info *die,
 	  unsigned_enum = 0;
 	  flag_enum = 0;
 	}
-      else if ((mask & value) != 0)
-	flag_enum = 0;
       else
-	mask |= value;
+	{
+	  if (count_one_bits_ll (value) >= 2)
+	    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
 	 a flag type, no need to look at the rest of the enumerates.  */
diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
index 57e04e6c01f3..acb3cb3ad25e 100644
--- a/gdb/testsuite/gdb.base/printcmds.c
+++ b/gdb/testsuite/gdb.base/printcmds.c
@@ -96,9 +96,35 @@ enum some_volatile_enum { enumvolval1, enumvolval2 };
    name.  See PR11827.  */
 volatile enum some_volatile_enum some_volatile_enum = enumvolval1;

-enum flag_enum { ONE = 1, TWO = 2 };
+/* An enum considered as a "flag enum".  */
+enum flag_enum
+{
+  FE_NONE = 0x00,
+  FE_ONE  = 0x01,
+  FE_TWO  = 0x02,
+};
+
+enum flag_enum three = FE_ONE | FE_TWO;
+
+/* Another enum considered as a "flag enum", but with no enumerator with value
+   0.  */
+enum flag_enum_without_zero
+{
+  FEWZ_ONE = 0x01,
+  FEWZ_TWO = 0x02,
+};
+
+enum flag_enum_without_zero flag_enum_without_zero = 0;
+
+/* Not a flag enum, an enumerator value has multiple bits sets.  */
+enum not_flag_enum
+{
+  NFE_ONE = 0x01,
+  NFE_TWO = 0x02,
+  NFE_F0  = 0xf0,
+};

-enum flag_enum three = ONE | TWO;
+enum not_flag_enum three_not_flag = NFE_ONE | NFE_TWO;

 /* A structure with an embedded array at an offset > 0.  The array has
    all elements with the same repeating value, which must not be the
diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index 6e98b7943ba3..6afb965af066 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -653,9 +653,9 @@ proc test_artificial_arrays {} {
     gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@2${ctrlv}@3" \
 	"({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \
 	{p int1dim[0]@2@3}
-    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO" " = {0, 1}" \
+    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO" " = {0, 1}" \
         {p int1dim[0]@TWO}
-    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@TWO${ctrlv}@three" \
+    gdb_test_escape_braces "p int1dim\[0\]${ctrlv}@FE_TWO${ctrlv}@three" \
 	"({{0, 1}, {2, 3}, {4, 5}}|\[Cc\]annot.*)" \
 	{p int1dim[0]@TWO@three}
     gdb_test_escape_braces {p/x (short [])0x12345678} \
@@ -736,7 +736,21 @@ proc test_print_enums {} {
     # Regression test for PR11827.
     gdb_test "print some_volatile_enum" "enumvolval1"

-    gdb_test "print three" " = \\\(ONE \\| TWO\\\)"
+    # Print a flag enum.
+    gdb_test "print three" [string_to_regexp " = (FE_ONE | FE_TWO)"]
+
+    # Print a flag enum with value 0, where an enumerator has value 0.
+    gdb_test "print (enum flag_enum) 0x0" [string_to_regexp " = FE_NONE"]
+
+    # Print a flag enum with value 0, where no enumerator has value 0.
+    gdb_test "print flag_enum_without_zero" [string_to_regexp " = (unknown: 0)"]
+
+    # Print a flag enum with unknown bits set.
+    gdb_test "print (enum flag_enum) 0xf1" [string_to_regexp " = (FE_ONE | unknown: 240)"]
+
+    # Test printing an enum not considered a "flag enum" (because one of its
+    # enumerators has multiple bits set).
+    gdb_test "print three_not_flag" [string_to_regexp " = 3"]
 }

 proc test_printf {} {
diff --git a/gdb/valprint.c b/gdb/valprint.c
index f26a87da3bd4..77b9a4993d79 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -39,6 +39,7 @@
 #include "cli/cli-option.h"
 #include "gdbarch.h"
 #include "cli/cli-style.h"
+#include "count-one-bits.h"

 /* Maximum number of wchars returned from wchar_iterate.  */
 #define MAX_WCHARS 4
@@ -638,7 +639,12 @@ generic_val_print_enum_1 (struct type *type, LONGEST val,
 	{
 	  QUIT;

-	  if ((val & TYPE_FIELD_ENUMVAL (type, i)) != 0)
+	  ULONGEST enumval = TYPE_FIELD_ENUMVAL (type, i);
+	  int nbits = count_one_bits_ll (enumval);
+
+	  gdb_assert (nbits == 0 || nbits == 1);
+
+	  if ((val & enumval) != 0)
 	    {
 	      if (!first)
 		fputs_filtered (" | ", stream);
-- 
2.25.0

Re: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums

[Tom Tromey]

>>>>> "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

Re: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums

[Tom Tromey]

>>>>> "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

Re: [PATCH 2/5] gdb: fix printing of flag enums with multi-bit enumerators

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon>   enum hello { AAA = 0x1, BBB = 0xf0 };
Simon>   (gdb) p (enum hello) 0x11
Simon>   $1 = (AAA | BBB)

Ouch.

Simon> gdb/ChangeLog:

Simon> 	* dwarf2/read.c: Include "count-one-bits.h".
Simon> 	(update_enumeration_type_from_children): If an enumerator has
Simon> 	multiple bits set, don't treat the enumeration as a "flag enum".
Simon> 	* valprint.c (generic_val_print_enum_1): Assert that enumerators
Simon> 	of flag enums have 0 or 1 bit set.

Thanks, this looks good to me.

Tom

Re: [PATCH 4/5] gdb: print unknown part of flag enum in hex

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> When we print the "unknown" part of a flag enum, it is printed in
Simon> decimal.  I think it would be more useful if it was printed in hex, as
Simon> it helps to determine which bits are set more than a decimal value.

This seems fine to me.

Tom

Re: [PATCH 5/5] gdb: change print format of flag enums with value 0

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> gdb/ChangeLog:

Simon> 	* valprint.c (generic_val_print_enum_1): When printing a flag
Simon> 	enum with value 0 and there is no enumerator with value 0, print
Simon> 	just "0" instead of "(unknown: 0x0)".

This looks reasonable.  Thanks.

Simon>        /* We have a "flag" enum, so we try to decompose it into
Simon>  	 pieces as appropriate.  A flag enum has disjoint
Simon>  	 constants by definition.  */

I guess this comment is stale now, because an earlier patch changed it
so that identical constants are allowed.

Tom

Re: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums

[Simon Marchi]

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

Re: [PATCH 5/5] gdb: change print format of flag enums with value 0

[Simon Marchi]

On 2020-02-18 3:45 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> gdb/ChangeLog:
> 
> Simon> 	* valprint.c (generic_val_print_enum_1): When printing a flag
> Simon> 	enum with value 0 and there is no enumerator with value 0, print
> Simon> 	just "0" instead of "(unknown: 0x0)".
> 
> This looks reasonable.  Thanks.
> 
> Simon>        /* We have a "flag" enum, so we try to decompose it into
> Simon>  	 pieces as appropriate.  A flag enum has disjoint
> Simon>  	 constants by definition.  */
> 
> I guess this comment is stale now, because an earlier patch changed it
> so that identical constants are allowed.

Right, I would then change this comment to this in patch 3/5:

 634       /* We have a "flag" enum, so we try to decompose it into pieces as
 635          appropriate.  The enum may have multiple enumerators representing
 636          the same bit, in which case we choose to only print the first one
 637          we find.  */

Simon

Re: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums

[Tom Tromey]

>>>>> "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

Re: [PATCH 3/5] gdb: allow duplicate enumerators in flag enums

[Simon Marchi]

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