[PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems

public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed

* [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems
@ 2020-08-21 14:45 Pedro Alves
  2020-08-21 14:45 ` [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502) Pedro Alves
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Pedro Alves @ 2020-08-21 14:45 UTC (permalink / raw)
  To: gdb-patches

This is an update of a version of this series that I posted back in
2016:

 [PATCH 0/3] enum_flags: Fix problems and add comprehensive unit tests
 https://sourceware.org/legacy-ml/gdb-patches/2016-11/msg00079.html

But I never followed thorough with it, leaving it collecting dust on
my github.  Over the following years, I polished it and tweaked it
further and extended the unit tests, but never managed to re-submit it
all upstream.

Since the subject of enum_flags came up again this week, I thought I'd
try it again.  This is the result.

Pedro Alves (3):
  Rewrite valid-expr.h's internals in terms of the detection idiom
    (C++17/N4502)
  Use type_instance_flags more throughout
  Rewrite enum_flags, add unit tests, fix problems

 gdb/Makefile.in                      |   1 +
 gdb/btrace.c                         |   4 +-
 gdb/compile/compile-c-types.c        |   3 +-
 gdb/compile/compile-cplus-symbols.c  |   4 +-
 gdb/compile/compile-cplus-types.c    |  10 +-
 gdb/dwarf2/read.c                    |   7 +-
 gdb/eval.c                           |   2 +-
 gdb/gdbarch.c                        |   6 +-
 gdb/gdbarch.h                        |  12 +-
 gdb/gdbarch.sh                       |   8 +-
 gdb/gdbtypes.c                       |  58 ++--
 gdb/gdbtypes.h                       |  15 +-
 gdb/go-exp.y                         |   2 +-
 gdb/record-btrace.c                  |  10 +-
 gdb/stabsread.c                      |   2 +-
 gdb/type-stack.c                     |   4 +-
 gdb/unittests/enum-flags-selftests.c | 586 +++++++++++++++++++++++++++++++++++
 gdbsupport/enum-flags.h              | 366 +++++++++++++++++-----
 gdbsupport/traits.h                  |  67 ++++
 gdbsupport/valid-expr.h              |  35 ++-
 20 files changed, 1035 insertions(+), 167 deletions(-)
 create mode 100644 gdb/unittests/enum-flags-selftests.c


base-commit: b70e516e89d95d06252cb04ded3397ec0a0a3212
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)
  2020-08-21 14:45 [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
@ 2020-08-21 14:45 ` Pedro Alves
  2020-09-28 18:59   ` Tom Tromey
  2020-08-21 14:45 ` [PATCH 2/3] Use type_instance_flags more throughout Pedro Alves
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2020-08-21 14:45 UTC (permalink / raw)
  To: gdb-patches

An earlier attempt at doing this had failed (wouldn't work in GCCs
around 4.8, IIRC), but now that I try again, it works.  I suspect that
my previous attempt did not use the pre C++14-safe void_t (in
traits.h).

I want to switch to this model because:

 - It's the standard detection idiom that folks will learn starting
   with C++17.

 - In the enum_flags unit tests, I have a static_assert that triggers
   a warning (resulting in build error), which GCC does not suppress
   because the warning is not being triggered in the SFINAE context.
   Switching to the detection idiom fixes that.  Alternatively,
   switching to the C++03-style expression-validity checking with a
   varargs overload would allow addressing that, but I think that
   would be going backwards idiomatically speaking.

 - While this patch shows a net increase of lines of code, the magic
   being added to traits.h can be removed in a few years when we start
   requiring C++17.

gdbsupport/ChangeLog:

	* traits.h (struct nonesuch, struct detector, detected_or)
	(detected_or_t, is_detected, detected_t, detected_or)
	(detected_or_t, is_detected_exact, is_detected_convertible): New.
	* valid-expr.h (CHECK_VALID_EXPR_INT): Use gdb::is_detected_exact.
---
 gdbsupport/traits.h     | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdbsupport/valid-expr.h | 20 +++------------
 2 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/gdbsupport/traits.h b/gdbsupport/traits.h
index 2a6f00654c..93b609ac10 100644
--- a/gdbsupport/traits.h
+++ b/gdbsupport/traits.h
@@ -52,6 +52,73 @@ struct make_void { typedef void type; };
 template<typename... Ts>
 using void_t = typename make_void<Ts...>::type;
 
+/* Implementation of the detection idiom:
+
+   - http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4502.pdf
+   - http://en.cppreference.com/w/cpp/experimental/is_detected
+
+*/
+
+struct nonesuch
+{
+  nonesuch () = delete;
+  ~nonesuch () = delete;
+  nonesuch (const nonesuch &) = delete;
+  void operator= (const nonesuch &) = delete;
+};
+
+namespace detection_detail {
+/* Implementation of the detection idiom (negative case).  */
+template<typename Default, typename AlwaysVoid,
+	 template<typename...> class Op, typename... Args>
+struct detector
+{
+  using value_t = std::false_type;
+  using type = Default;
+};
+
+/* Implementation of the detection idiom (positive case).  */
+template<typename Default, template<typename...> class Op, typename... Args>
+struct detector<Default, void_t<Op<Args...>>, Op, Args...>
+{
+  using value_t = std::true_type;
+  using type = Op<Args...>;
+};
+
+/* Detect whether Op<Args...> is a valid type, use Default if not.  */
+template<typename Default, template<typename...> class Op,
+	 typename... Args>
+using detected_or = detector<Default, void, Op, Args...>;
+
+/* Op<Args...> if that is a valid type, otherwise Default.  */
+template<typename Default, template<typename...> class Op,
+	 typename... Args>
+using detected_or_t
+  = typename detected_or<Default, Op, Args...>::type;
+
+} /* detection_detail */
+
+template<template<typename...> class Op, typename... Args>
+using is_detected
+  = typename detection_detail::detector<nonesuch, void, Op, Args...>::value_t;
+
+template<template<typename...> class Op, typename... Args>
+using detected_t
+  = typename detection_detail::detector<nonesuch, void, Op, Args...>::type;
+
+template<typename Default, template<typename...> class Op, typename... Args>
+using detected_or = detection_detail::detected_or<Default, Op, Args...>;
+
+template<typename Default, template<typename...> class Op, typename... Args>
+using detected_or_t = typename detected_or<Default, Op, Args...>::type;
+
+template<typename Expected, template<typename...> class Op, typename... Args>
+using is_detected_exact = std::is_same<Expected, detected_t<Op, Args...>>;
+
+template<typename To, template<typename...> class Op, typename... Args>
+using is_detected_convertible
+  = std::is_convertible<detected_t<Op, Args...>, To>;
+
 /* A few trait helpers, mainly stolen from libstdc++.  Uppercase
    because "and/or", etc. are reserved keywords.  */
 
diff --git a/gdbsupport/valid-expr.h b/gdbsupport/valid-expr.h
index b1c8446814..a22fa61134 100644
--- a/gdbsupport/valid-expr.h
+++ b/gdbsupport/valid-expr.h
@@ -58,26 +58,12 @@
 #define CHECK_VALID_EXPR_INT(TYPENAMES, TYPES, VALID, EXPR_TYPE, EXPR)	\
   namespace CONCAT (check_valid_expr, __LINE__) {			\
 									\
-  template<typename, typename, typename = void>				\
-  struct is_valid_expression						\
-    : std::false_type {};						\
-									\
   template <TYPENAMES>							\
-    struct is_valid_expression<TYPES, gdb::void_t<decltype (EXPR)>>	\
-    : std::true_type {};						\
+    using archetype = decltype (EXPR);					\
 									\
-  static_assert (is_valid_expression<TYPES>::value == VALID,		\
+  static_assert (gdb::is_detected_exact<EXPR_TYPE,			\
+		 archetype, TYPES>::value == VALID,			\
 		 "");							\
-									\
-  template<TYPENAMES, typename = void>					\
-  struct is_same_type							\
-    : std::is_same<EXPR_TYPE, void> {};					\
-									\
-  template <TYPENAMES>							\
-    struct is_same_type<TYPES, gdb::void_t<decltype (EXPR)>>		\
-    : std::is_same<EXPR_TYPE, decltype (EXPR)> {};			\
-									\
-  static_assert (is_same_type<TYPES>::value, "");			\
   } /* namespace */
 
 /* A few convenience macros that support expressions involving a
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)
  2020-08-21 14:45 ` [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502) Pedro Alves
@ 2020-09-28 18:59   ` Tom Tromey
  2020-09-28 19:58     ` Luis Machado
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2020-09-28 18:59 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> +  static_assert (gdb::is_detected_exact<EXPR_TYPE,			\
Pedro> +		 archetype, TYPES>::value == VALID,			\
Pedro>  		 "");							\


I'm not sure if it was exactly this patch, but after I merged this code
to our internal tree, we started having build problems on a machine that
uses GCC 6.4.  I've appended the error message.

I'm not sure it is worth doing anything about this, but I thought it
would be worth noting in case anybody else winds up in this situation.

Tom

../../src/gdb/unittests/offset-type-selftests.c: In substitution of 'template<class Expected, template<class ...> class Op, class ... Args> using is_detected_exact = std::is_same<Expected, typename gdb::detection_detail::detector<gdb::nonesuch, void, Op, Args ...>::type> [with Expected = selftests::offset_type::off_A&; Op = selftests::offset_type::check_valid_expr75::archetype; Args = {selftests::offset_type::off_A, selftests::offset_type::off_B}]':
../../src/gdb/unittests/offset-type-selftests.c:75:1:   required from here
../../src/gdb/../gdbsupport/valid-expr.h:65:20: error: type/value mismatch at argument 2 in template parameter list for 'template<class Expected, template<class ...> class Op, class ... Args> using is_detected_exact = std::is_same<Expected, typename gdb::detection_detail::detector<gdb::nonesuch, void, Op, Args ...>::type>'
    archetype, TYPES>::value == VALID,   \
                    ^
../../src/gdb/../gdbsupport/valid-expr.h:79:3: note: in expansion of macro 'CHECK_VALID_EXPR_INT'
   CHECK_VALID_EXPR_INT (ESC_PARENS(typename T1, typename T2),  \
   ^~~~~~~~~~~~~~~~~~~~
../../src/gdb/unittests/offset-type-selftests.c:42:3: note: in expansion of macro 'CHECK_VALID_EXPR_2'
   CHECK_VALID_EXPR_2 (off_A, off_B, VALID, EXPR_TYPE, EXPR)
   ^~~~~~~~~~~~~~~~~~
../../src/gdb/unittests/offset-type-selftests.c:75:1: note: in expansion of macro 'CHECK_VALID'
 CHECK_VALID (true,  off_A&, lval_a += undrl {});
 ^~~~~~~~~~~
../../src/gdb/../gdbsupport/valid-expr.h:65:20: note:   expected a class template, got 'selftests::offset_type::check_valid_expr75::archetype'
    archetype, TYPES>::value == VALID,   \
                    ^
../../src/gdb/../gdbsupport/valid-expr.h:79:3: note: in expansion of macro 'CHECK_VALID_EXPR_INT'
   CHECK_VALID_EXPR_INT (ESC_PARENS(typename T1, typename T2),  \
   ^~~~~~~~~~~~~~~~~~~~
../../src/gdb/unittests/offset-type-selftests.c:42:3: note: in expansion of macro 'CHECK_VALID_EXPR_2'
   CHECK_VALID_EXPR_2 (off_A, off_B, VALID, EXPR_TYPE, EXPR)
   ^~~~~~~~~~~~~~~~~~
../../src/gdb/unittests/offset-type-selftests.c:75:1: note: in expansion of macro 'CHECK_VALID'
 CHECK_VALID (true,  off_A&, lval_a += undrl {});
 ^~~~~~~~~~~

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)
  2020-09-28 18:59   ` Tom Tromey
@ 2020-09-28 19:58     ` Luis Machado
  2020-09-28 20:00       ` Tom Tromey
  0 siblings, 1 reply; 20+ messages in thread
From: Luis Machado @ 2020-09-28 19:58 UTC (permalink / raw)
  To: Tom Tromey, Pedro Alves; +Cc: gdb-patches

On 9/28/20 3:59 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> +  static_assert (gdb::is_detected_exact<EXPR_TYPE,			\
> Pedro> +		 archetype, TYPES>::value == VALID,			\
> Pedro>  		 "");							\
> 
> 
> I'm not sure if it was exactly this patch, but after I merged this code
> to our internal tree, we started having build problems on a machine that
> uses GCC 6.4.  I've appended the error message.
> 
> I'm not sure it is worth doing anything about this, but I thought it
> would be worth noting in case anybody else winds up in this situation.
> 
> Tom

It was reported before on this thread a week or so ago. Unless GCC 6.4 
isn't supported any longer, I think it should be fixed.

If GCC 6.4 isn't supported, then we should make that explicit on the 
documentation to spare folks the trouble of having to figure it out.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)
  2020-09-28 19:58     ` Luis Machado
@ 2020-09-28 20:00       ` Tom Tromey
  2020-09-29 13:04         ` Tom Tromey
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2020-09-28 20:00 UTC (permalink / raw)
  To: Luis Machado; +Cc: Tom Tromey, Pedro Alves, gdb-patches

>> I'm not sure if it was exactly this patch, but after I merged this code
>> to our internal tree, we started having build problems on a machine that
>> uses GCC 6.4.  I've appended the error message.
>> I'm not sure it is worth doing anything about this, but I thought it
>> would be worth noting in case anybody else winds up in this situation.
>> Tom

Luis> It was reported before on this thread a week or so ago. Unless GCC 6.4
Luis> isn't supported any longer, I think it should be fixed.

Ok, thanks.  I will find out tonight.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)
  2020-09-28 20:00       ` Tom Tromey
@ 2020-09-29 13:04         ` Tom Tromey
  2020-09-29 19:24           ` Pedro Alves
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2020-09-29 13:04 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Luis Machado, Pedro Alves, gdb-patches

>>> I'm not sure if it was exactly this patch, but after I merged this code
>>> to our internal tree, we started having build problems on a machine that
>>> uses GCC 6.4.  I've appended the error message.
>>> I'm not sure it is worth doing anything about this, but I thought it
>>> would be worth noting in case anybody else winds up in this situation.
>>> Tom

Luis> It was reported before on this thread a week or so ago. Unless GCC 6.4
Luis> isn't supported any longer, I think it should be fixed.

Tom> Ok, thanks.  I will find out tonight.

Oops, I really misunderstood your note, and re-reading it now I can't
really understand why.  I thought you were saying it was fixed, but
really you were saying it is still there and should be fixed if we want
to support GCC 6.4.

I am not sure if we should try to fix it or not.  Locally I plan to
disable unit tests on this particular machine.  It is the only one using
6.4.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)
  2020-09-29 13:04         ` Tom Tromey
@ 2020-09-29 19:24           ` Pedro Alves
  2020-09-29 22:50             ` [pushed] Tweak gdbsupport/valid-expr.h for GCC 6, fix build (Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)) Pedro Alves
  0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2020-09-29 19:24 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Luis Machado, gdb-patches

On 9/29/20 2:04 PM, Tom Tromey wrote:
>>>> I'm not sure if it was exactly this patch, but after I merged this code
>>>> to our internal tree, we started having build problems on a machine that
>>>> uses GCC 6.4.  I've appended the error message.
>>>> I'm not sure it is worth doing anything about this, but I thought it
>>>> would be worth noting in case anybody else winds up in this situation.
>>>> Tom
> 
> Luis> It was reported before on this thread a week or so ago. Unless GCC 6.4
> Luis> isn't supported any longer, I think it should be fixed.
> 
> Tom> Ok, thanks.  I will find out tonight.
> 
> Oops, I really misunderstood your note, and re-reading it now I can't
> really understand why.  I thought you were saying it was fixed, but
> really you were saying it is still there and should be fixed if we want
> to support GCC 6.4.
> 
> I am not sure if we should try to fix it or not.  Locally I plan to
> disable unit tests on this particular machine.  It is the only one using
> 6.4.

I built a GCC 6.4 here, and could reproduce it.  I'm testing a fix.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [pushed] Tweak gdbsupport/valid-expr.h for GCC 6, fix build (Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502))
  2020-09-29 19:24           ` Pedro Alves
@ 2020-09-29 22:50             ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2020-09-29 22:50 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Luis Machado, gdb-patches

On 9/29/20 8:24 PM, Pedro Alves wrote:
> On 9/29/20 2:04 PM, Tom Tromey wrote:
>>>>> I'm not sure if it was exactly this patch, but after I merged this code
>>>>> to our internal tree, we started having build problems on a machine that
>>>>> uses GCC 6.4.  I've appended the error message.
>>>>> I'm not sure it is worth doing anything about this, but I thought it
>>>>> would be worth noting in case anybody else winds up in this situation.
>>>>> Tom
>>
>> Luis> It was reported before on this thread a week or so ago. Unless GCC 6.4
>> Luis> isn't supported any longer, I think it should be fixed.
>>
>> Tom> Ok, thanks.  I will find out tonight.
>>
>> Oops, I really misunderstood your note, and re-reading it now I can't
>> really understand why.  I thought you were saying it was fixed, but
>> really you were saying it is still there and should be fixed if we want
>> to support GCC 6.4.
>>
>> I am not sure if we should try to fix it or not.  Locally I plan to
>> disable unit tests on this particular machine.  It is the only one using
>> 6.4.
> 
> I built a GCC 6.4 here, and could reproduce it.  I'm testing a fix.
> 

Here's what I merged.

From de38d64ad2502312afb0000ac806474c1e2c0fe5 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 29 Sep 2020 20:08:51 +0100
Subject: [PATCH] Tweak gdbsupport/valid-expr.h for GCC 6, fix build

With GCC 6.4 and 6.5 (at least), unit tests that use
gdbsupport/valid-expr.h's CHECK_VALID fail to compile, with:

 In file included from src/gdb/unittests/offset-type-selftests.c:24:0:
 src/gdb/unittests/offset-type-selftests.c: In substitution of 'template<class Expected, template<class ...> class Op, class ... Args> using is_detected_exact = std::is_same<Expected, typename gdb::detection_detail::detector<gdb::nonesuch, void, Op, Args ...>::type> [with Expected = selftests::offset_type::off_A&; Op = selftests::offset_type::check_valid_expr75::archetype; Args = {selftests::offset_type::off_A, selftests::offset_type::off_B}]':
 src/gdb/unittests/offset-type-selftests.c:75:1:   required from here
 src/gdb/../gdbsupport/valid-expr.h:65:20: error: type/value mismatch at argument 2 in template parameter list for 'template<class Expected, template<class ...> class Op, class ... Args> using is_detected_exact = std::is_same<Expected, typename gdb::detection_detail::detector<gdb::nonesuch, void, Op, Args ...>::type>'
     archetype, TYPES>::value == VALID,   \
		     ^

The important part is the "error: type/value mismatch" error.  Seems
like that GCC doesn't understand that archetype is an alias template,
and is being strict in requiring a template class.

The fix here is then to make archetype a template class, to pacify
GCC.  The resulting code looks like this:

  template <TYPENAMES, typename = decltype (EXPR)>
  struct archetype
  {
  };

  static_assert (gdb::is_detected_exact<archetype<TYPES, EXPR_TYPE>,
 		 archetype, TYPES>::value == VALID, "");

is_detected_exact<Expected, Op, Args> checks whether Op<Args> is type
Expected:

 - For Expected, we pass the explicit EXPR_TYPE, overriding the
   default parameter type of archetype.

 - For Args we don't pass the last template parameter, so archtype
   defaults to the EXPR's decltype.

So in essence, we're really checking whether EXPR_TYPE is the same as
decltype(EXPR).

We need to do the decltype in a template context in order to trigger
SFINAE instead of failing to compile.


The hunk in unittests/enum-flags-selftests.c becomes necessary,
because unlike with the current alias template version, this new
version makes GCC trigger -Wenum-compare warnings as well:

 src/gdb/unittests/enum-flags-selftests.c:328:33: error: comparison between 'enum selftests::enum_flags_tests::RE' and 'enum selftests::enum_flags_tests::RE2' [-Werror=enum-compare]
  CHECK_VALID (true,  bool, RE () != RE2 ())
				  ^
 src/gdb/../gdbsupport/valid-expr.h:61:45: note: in definition of macro 'CHECK_VALID_EXPR_INT'
    template <TYPENAMES, typename = decltype (EXPR)>   \
					      ^

Build-tested with:

 - GCC {4.8.5, 6.4, 6.5, 7.3.1, 9.3.0, 11.0.0-20200910}
 - Clang 10.0.0

gdbsupport/ChangeLog:

	* valid-expr.h (CHECK_VALID_EXPR_INT): Make archetype a template
	class instead of an alias template and adjust static_assert.

gdb/ChangeLog:

	* unittests/enum-flags-selftests.c: Check whether __GNUC__ is
	defined before using '#pragma GCC diagnostic' instead of checking
	__clang__.
---
 gdb/ChangeLog                        |  6 ++++++
 gdbsupport/ChangeLog                 |  5 +++++
 gdb/unittests/enum-flags-selftests.c | 24 +++++++++++++++++-------
 gdbsupport/valid-expr.h              |  8 +++++---
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fcfa751fd7f..28e34fba04f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2020-09-29  Pedro Alves  <pedro@palves.net>
+
+	* unittests/enum-flags-selftests.c: Check whether __GNUC__ is
+	defined before using '#pragma GCC diagnostic' instead of checking
+	__clang__.
+
 2020-09-28  Tom Tromey  <tom@tromey.com>
 
 	* infrun.c (displaced_step_fixup, thread_still_needs_step_over)
diff --git a/gdbsupport/ChangeLog b/gdbsupport/ChangeLog
index a2f563c81c0..91435ff1e8e 100644
--- a/gdbsupport/ChangeLog
+++ b/gdbsupport/ChangeLog
@@ -1,3 +1,8 @@
+2020-09-29  Pedro Alves  <pedro@palves.net>
+
+	* valid-expr.h (CHECK_VALID_EXPR_INT): Make archetype a template
+	class instead of an alias template and adjust static_assert.
+
 2020-09-24  Simon Marchi  <simon.marchi@efficios.com>
 
 	* event-loop.c (struct file_handler): Remove typedef, re-format.
diff --git a/gdb/unittests/enum-flags-selftests.c b/gdb/unittests/enum-flags-selftests.c
index af585f04ae5..5455120a259 100644
--- a/gdb/unittests/enum-flags-selftests.c
+++ b/gdb/unittests/enum-flags-selftests.c
@@ -315,18 +315,28 @@ CHECK_VALID (false, void, EF () != EF2 ())
 CHECK_VALID (false, void, EF () != RE2 ())
 CHECK_VALID (false, void, RE () != EF2 ())
 
-/* On clang, disable -Wenum-compare due to "error: comparison of two
-   values with different enumeration types [-Werror,-Wenum-compare]".
-   clang doesn't suppress -Wenum-compare in SFINAE contexts.  Not a
-   big deal since misuses like these in GDB will be caught by -Werror
-   anyway.  This check is here mainly for completeness.  */
-#if defined __clang__
+/* Disable -Wenum-compare due to:
+
+   Clang:
+
+    "error: comparison of two values with different enumeration types
+    [-Werror,-Wenum-compare]"
+
+   GCC:
+
+    "error: comparison between ‘enum selftests::enum_flags_tests::RE’
+     and ‘enum selftests::enum_flags_tests::RE2’
+     [-Werror=enum-compare]"
+
+   Not a big deal since misuses like these in GDB will be caught by
+   -Werror anyway.  This check is here mainly for completeness.  */
+#if defined __GNUC__
 # pragma GCC diagnostic push
 # pragma GCC diagnostic ignored "-Wenum-compare"
 #endif
 CHECK_VALID (true,  bool, RE () == RE2 ())
 CHECK_VALID (true,  bool, RE () != RE2 ())
-#if defined __clang__
+#if defined __GNUC__
 # pragma GCC diagnostic pop
 #endif
 
diff --git a/gdbsupport/valid-expr.h b/gdbsupport/valid-expr.h
index 459de179266..ff5b8f510a4 100644
--- a/gdbsupport/valid-expr.h
+++ b/gdbsupport/valid-expr.h
@@ -58,10 +58,12 @@
 #define CHECK_VALID_EXPR_INT(TYPENAMES, TYPES, VALID, EXPR_TYPE, EXPR)	\
   namespace CONCAT (check_valid_expr, __LINE__) {			\
 									\
-  template <TYPENAMES>							\
-    using archetype = decltype (EXPR);					\
+  template <TYPENAMES, typename = decltype (EXPR)>			\
+  struct archetype							\
+  {									\
+  };									\
 									\
-  static_assert (gdb::is_detected_exact<EXPR_TYPE,			\
+  static_assert (gdb::is_detected_exact<archetype<TYPES, EXPR_TYPE>,	\
 		 archetype, TYPES>::value == VALID,			\
 		 "");							\
   } /* namespace */

base-commit: aeaccbf4c55033bd6641709f98f3f6d65e695f85
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-21 14:45 [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
  2020-08-21 14:45 ` [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502) Pedro Alves
@ 2020-08-21 14:45 ` Pedro Alves
  2020-08-25 11:36   ` Luis Machado
                     ` (3 more replies)
  2020-08-21 14:45 ` [PATCH 3/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
  2020-08-21 15:51 ` [PATCH 0/3] " Andrew Burgess
  3 siblings, 4 replies; 20+ messages in thread
From: Pedro Alves @ 2020-08-21 14:45 UTC (permalink / raw)
  To: gdb-patches

The next patch in this series will rewrites enum_flags fixing some API
holes.  That would cause build failures around code using
type_instance_flags.  Or rather, that should be using it, but wasn't.

This patch fixes it by using type_instance_flags throughout instead of
plain integers.

Note that we can't make the seemingly obvious change to struct
type::instance_flags:

 -  unsigned instance_flags : 9;
 +  ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9;

Because G++ complains then that 9 bits isn't sufficient for holding
all values of type_instance_flag_value.

So the patch adds a cast to TYPE_INSTANCE_FLAGS, and adds a separate
SET_TYPE_INSTANCE_FLAGS macro.

gdb/ChangeLog:

	* dwarf2/read.c (read_tag_pointer_type): Use type_instance_flags.
	* eval.c (fake_method::fake_method): Use SET_TYPE_INSTANCE_FLAGS.
	* gdbarch.h, gdbarch.c: Regenerate.
	* gdbarch.sh (address_class_type_flags): Use type_instance_flags.
	(address_class_name_to_type_flags): Use type_instance_flags and
	bool.
	* gdbtypes.c (address_space_name_to_int)
	(address_space_int_to_name, make_qualified_type): Use
	type_instance_flags.
	(make_qualified_type): Use type_instance_flags and
	SET_TYPE_INSTANCE_FLAGS.
	(make_type_with_address_space, make_cv_type, make_vector_type)
	(check_typedef): Use type_instance_flags.
	(recursive_dump_type): Cast type_instance_flags to unsigned for
	printing.
	(copy_type_recursive): Use SET_TYPE_INSTANCE_FLAGS.
	* gdbtypes.h (TYPE_INSTANCE_FLAGS): Return a type_instance_flags.
	(SET_TYPE_INSTANCE_FLAGS): New.
	(address_space_name_to_int, address_space_int_to_name)
	(make_type_with_address_space): Pass flags using
	type_instance_flags instead of int.
	* stabsread.c (cleanup_undefined_types_noname): Use
	SET_TYPE_INSTANCE_FLAGS.
	* type-stack.c (type_stack::follow_types): Use type_instance_flags.
---
 gdb/dwarf2/read.c |  7 +++----
 gdb/eval.c        |  2 +-
 gdb/gdbarch.c     |  6 +++---
 gdb/gdbarch.h     | 12 ++++++------
 gdb/gdbarch.sh    |  8 ++++----
 gdb/gdbtypes.c    | 58 ++++++++++++++++++++++++++++++-------------------------
 gdb/gdbtypes.h    | 15 +++++++++-----
 gdb/stabsread.c   |  2 +-
 gdb/type-stack.c  |  4 ++--
 9 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 0ac8533263..4ced5ac02b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17292,10 +17292,9 @@ read_tag_pointer_type (struct die_info *die, struct dwarf2_cu *cu)
     {
       if (gdbarch_address_class_type_flags_p (gdbarch))
 	{
-	  int type_flags;
-
-	  type_flags = gdbarch_address_class_type_flags
-			 (gdbarch, byte_size, addr_class);
+	  type_instance_flags type_flags
+	    = gdbarch_address_class_type_flags (gdbarch, byte_size,
+						addr_class);
 	  gdb_assert ((type_flags & ~TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
 		      == 0);
 	  type = make_type_with_address_space (type, type_flags);
diff --git a/gdb/eval.c b/gdb/eval.c
index c62c35f318..cd300ddfef 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -659,7 +659,7 @@ fake_method::fake_method (type_instance_flags flags,
   TYPE_LENGTH (type) = 1;
   type->set_code (TYPE_CODE_METHOD);
   TYPE_CHAIN (type) = type;
-  TYPE_INSTANCE_FLAGS (type) = flags;
+  SET_TYPE_INSTANCE_FLAGS (type, flags);
   if (num_types > 0)
     {
       if (param_types[num_types - 1] == NULL)
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index f8fe03ca68..2be959ecfc 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -3501,7 +3501,7 @@ gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch)
   return gdbarch->address_class_type_flags != NULL;
 }
 
-int
+type_instance_flags
 gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class)
 {
   gdb_assert (gdbarch != NULL);
@@ -3566,8 +3566,8 @@ gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch)
   return gdbarch->address_class_name_to_type_flags != NULL;
 }
 
-int
-gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr)
+bool
+gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->address_class_name_to_type_flags != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 7a3060e628..8a4a384fda 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -848,8 +848,8 @@ extern void set_gdbarch_have_nonsteppable_watchpoint (struct gdbarch *gdbarch, i
 
 extern int gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch);
 
-typedef int (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
-extern int gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
+typedef type_instance_flags (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
+extern type_instance_flags gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
 extern void set_gdbarch_address_class_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_type_flags_ftype *address_class_type_flags);
 
 extern int gdbarch_address_class_type_flags_to_name_p (struct gdbarch *gdbarch);
@@ -866,13 +866,13 @@ extern bool gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_by
 extern void set_gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdbarch_execute_dwarf_cfa_vendor_op_ftype *execute_dwarf_cfa_vendor_op);
 
 /* Return the appropriate type_flags for the supplied address class.
-   This function should return 1 if the address class was recognized and
-   type_flags was set, zero otherwise. */
+   This function should return true if the address class was recognized and
+   type_flags was set, false otherwise. */
 
 extern int gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch);
 
-typedef int (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
-extern int gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
+typedef bool (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
+extern bool gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
 extern void set_gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_name_to_type_flags_ftype *address_class_name_to_type_flags);
 
 /* Is a register in a group */
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 6d3c5c889d..7e9204119b 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -689,16 +689,16 @@ v;int;cannot_step_breakpoint;;;0;0;;0
 # See comment in target.h about continuable, steppable and
 # non-steppable watchpoints.
 v;int;have_nonsteppable_watchpoint;;;0;0;;0
-F;int;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
+F;type_instance_flags;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
 M;const char *;address_class_type_flags_to_name;int type_flags;type_flags
 # Execute vendor-specific DWARF Call Frame Instruction.  OP is the instruction.
 # FS are passed from the generic execute_cfa_program function.
 m;bool;execute_dwarf_cfa_vendor_op;gdb_byte op, struct dwarf2_frame_state *fs;op, fs;;default_execute_dwarf_cfa_vendor_op;;0
 
 # Return the appropriate type_flags for the supplied address class.
-# This function should return 1 if the address class was recognized and
-# type_flags was set, zero otherwise.
-M;int;address_class_name_to_type_flags;const char *name, int *type_flags_ptr;name, type_flags_ptr
+# This function should return true if the address class was recognized and
+# type_flags was set, false otherwise.
+M;bool;address_class_name_to_type_flags;const char *name, type_instance_flags *type_flags_ptr;name, type_flags_ptr
 # Is a register in a group
 m;int;register_reggroup_p;int regnum, struct reggroup *reggroup;regnum, reggroup;;default_register_reggroup_p;;0
 # Fetch the pointer to the ith function argument.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index c1eb03d898..64e44bfe23 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -574,11 +574,11 @@ lookup_function_type_with_arguments (struct type *type,
 /* Identify address space identifier by name --
    return the integer flag defined in gdbtypes.h.  */
 
-int
+type_instance_flags
 address_space_name_to_int (struct gdbarch *gdbarch,
 			   const char *space_identifier)
 {
-  int type_flags;
+  type_instance_flags type_flags;
 
   /* Check for known address space delimiters.  */
   if (!strcmp (space_identifier, "code"))
@@ -598,7 +598,8 @@ address_space_name_to_int (struct gdbarch *gdbarch,
    gdbtypes.h -- return the string version of the adress space name.  */
 
 const char *
-address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
+address_space_int_to_name (struct gdbarch *gdbarch,
+			   type_instance_flags space_flag)
 {
   if (space_flag & TYPE_INSTANCE_FLAG_CODE_SPACE)
     return "code";
@@ -617,7 +618,7 @@ address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
    STORAGE must be in the same obstack as TYPE.  */
 
 static struct type *
-make_qualified_type (struct type *type, int new_flags,
+make_qualified_type (struct type *type, type_instance_flags new_flags,
 		     struct type *storage)
 {
   struct type *ntype;
@@ -657,7 +658,7 @@ make_qualified_type (struct type *type, int new_flags,
   TYPE_CHAIN (type) = ntype;
 
   /* Now set the instance flags and return the new type.  */
-  TYPE_INSTANCE_FLAGS (ntype) = new_flags;
+  SET_TYPE_INSTANCE_FLAGS (ntype, new_flags);
 
   /* Set length of new type to that of the original type.  */
   TYPE_LENGTH (ntype) = TYPE_LENGTH (type);
@@ -675,13 +676,14 @@ make_qualified_type (struct type *type, int new_flags,
    representations.  */
 
 struct type *
-make_type_with_address_space (struct type *type, int space_flag)
+make_type_with_address_space (struct type *type,
+			      type_instance_flags space_flag)
 {
-  int new_flags = ((TYPE_INSTANCE_FLAGS (type)
-		    & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
-			| TYPE_INSTANCE_FLAG_DATA_SPACE
-		        | TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
-		   | space_flag);
+  type_instance_flags new_flags = ((TYPE_INSTANCE_FLAGS (type)
+				    & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
+					| TYPE_INSTANCE_FLAG_DATA_SPACE
+					| TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
+				   | space_flag);
 
   return make_qualified_type (type, new_flags, NULL);
 }
@@ -705,9 +707,9 @@ make_cv_type (int cnst, int voltl,
 {
   struct type *ntype;	/* New type */
 
-  int new_flags = (TYPE_INSTANCE_FLAGS (type)
-		   & ~(TYPE_INSTANCE_FLAG_CONST 
-		       | TYPE_INSTANCE_FLAG_VOLATILE));
+  type_instance_flags new_flags = (TYPE_INSTANCE_FLAGS (type)
+				   & ~(TYPE_INSTANCE_FLAG_CONST
+				       | TYPE_INSTANCE_FLAG_VOLATILE));
 
   if (cnst)
     new_flags |= TYPE_INSTANCE_FLAG_CONST;
@@ -1410,7 +1412,6 @@ void
 make_vector_type (struct type *array_type)
 {
   struct type *inner_array, *elt_type;
-  int flags;
 
   /* Find the innermost array type, in case the array is
      multi-dimensional.  */
@@ -1421,7 +1422,8 @@ make_vector_type (struct type *array_type)
   elt_type = TYPE_TARGET_TYPE (inner_array);
   if (elt_type->code () == TYPE_CODE_INT)
     {
-      flags = TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
+      type_instance_flags flags
+	= TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
       elt_type = make_qualified_type (elt_type, flags, NULL);
       TYPE_TARGET_TYPE (inner_array) = elt_type;
     }
@@ -2732,12 +2734,13 @@ struct type *
 check_typedef (struct type *type)
 {
   struct type *orig_type = type;
-  /* While we're removing typedefs, we don't want to lose qualifiers.
-     E.g., const/volatile.  */
-  int instance_flags = TYPE_INSTANCE_FLAGS (type);
 
   gdb_assert (type);
 
+  /* While we're removing typedefs, we don't want to lose qualifiers.
+     E.g., const/volatile.  */
+  type_instance_flags instance_flags = TYPE_INSTANCE_FLAGS (type);
+
   while (type->code () == TYPE_CODE_TYPEDEF)
     {
       if (!TYPE_TARGET_TYPE (type))
@@ -2778,10 +2781,13 @@ check_typedef (struct type *type)
 	 outer cast in a chain of casting win), instead of assuming
 	 "it can't happen".  */
       {
-	const int ALL_SPACES = (TYPE_INSTANCE_FLAG_CODE_SPACE
-				| TYPE_INSTANCE_FLAG_DATA_SPACE);
-	const int ALL_CLASSES = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
-	int new_instance_flags = TYPE_INSTANCE_FLAGS (type);
+	const type_instance_flags ALL_SPACES
+	  = (TYPE_INSTANCE_FLAG_CODE_SPACE
+	     | TYPE_INSTANCE_FLAG_DATA_SPACE);
+	const type_instance_flags ALL_CLASSES
+	  = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
+	type_instance_flags new_instance_flags
+	  = TYPE_INSTANCE_FLAGS (type);
 
 	/* Treat code vs data spaces and address classes separately.  */
 	if ((instance_flags & ALL_SPACES) != 0)
@@ -5026,7 +5032,7 @@ recursive_dump_type (struct type *type, int spaces)
   gdb_print_host_address (TYPE_CHAIN (type), gdb_stdout);
   printf_filtered ("\n");
   printfi_filtered (spaces, "instance_flags 0x%x", 
-		    TYPE_INSTANCE_FLAGS (type));
+		    (unsigned) TYPE_INSTANCE_FLAGS (type));
   if (TYPE_CONST (type))
     {
       puts_filtered (" TYPE_CONST");
@@ -5300,7 +5306,7 @@ copy_type_recursive (struct objfile *objfile,
   if (type->name ())
     new_type->set_name (xstrdup (type->name ()));
 
-  TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
+  SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
   TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
 
   /* Copy the fields.  */
@@ -5427,7 +5433,7 @@ copy_type (const struct type *type)
   gdb_assert (TYPE_OBJFILE_OWNED (type));
 
   new_type = alloc_type_copy (type);
-  TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
+  SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
   TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
   memcpy (TYPE_MAIN_TYPE (new_type), TYPE_MAIN_TYPE (type),
 	  sizeof (struct main_type));
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 55a6dafb7e..b42cef6137 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1585,7 +1585,10 @@ extern void allocate_gnat_aux_type (struct type *);
      TYPE_ZALLOC (type,							       \
 		  sizeof (*TYPE_MAIN_TYPE (type)->type_specific.func_stuff)))
 
-#define TYPE_INSTANCE_FLAGS(thistype) (thistype)->instance_flags
+#define TYPE_INSTANCE_FLAGS(thistype) \
+  type_instance_flags ((enum type_instance_flag_value) (thistype)->instance_flags)
+#define SET_TYPE_INSTANCE_FLAGS(thistype, flags) \
+  (thistype)->instance_flags = flags
 #define TYPE_MAIN_TYPE(thistype) (thistype)->main_type
 #define TYPE_TARGET_TYPE(thistype) TYPE_MAIN_TYPE(thistype)->target_type
 #define TYPE_POINTER_TYPE(thistype) (thistype)->pointer_type
@@ -2117,12 +2120,14 @@ extern struct type *make_atomic_type (struct type *);
 
 extern void replace_type (struct type *, struct type *);
 
-extern int address_space_name_to_int (struct gdbarch *, const char *);
+extern type_instance_flags address_space_name_to_int (struct gdbarch *,
+						      const char *);
 
-extern const char *address_space_int_to_name (struct gdbarch *, int);
+extern const char *address_space_int_to_name (struct gdbarch *,
+					      type_instance_flags);
 
-extern struct type *make_type_with_address_space (struct type *type, 
-						  int space_identifier);
+extern struct type *make_type_with_address_space
+  (struct type *type, type_instance_flags space_identifier);
 
 extern struct type *lookup_memberptr_type (struct type *, struct type *);
 
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index d2ff54a47b..ed31dc0111 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -4397,7 +4397,7 @@ cleanup_undefined_types_noname (struct objfile *objfile)
              and needs to be copied over from the reference type.
              Since replace_type expects them to be identical, we need
              to set these flags manually before hand.  */
-          TYPE_INSTANCE_FLAGS (nat.type) = TYPE_INSTANCE_FLAGS (*type);
+          SET_TYPE_INSTANCE_FLAGS (nat.type, TYPE_INSTANCE_FLAGS (*type));
           replace_type (nat.type, *type);
         }
     }
diff --git a/gdb/type-stack.c b/gdb/type-stack.c
index f8661d7565..608142c849 100644
--- a/gdb/type-stack.c
+++ b/gdb/type-stack.c
@@ -109,7 +109,7 @@ type_stack::follow_types (struct type *follow_type)
   int done = 0;
   int make_const = 0;
   int make_volatile = 0;
-  int make_addr_space = 0;
+  type_instance_flags make_addr_space = 0;
   bool make_restrict = false;
   bool make_atomic = false;
   int array_size;
@@ -128,7 +128,7 @@ type_stack::follow_types (struct type *follow_type)
 	make_volatile = 1;
 	break;
       case tp_space_identifier:
-	make_addr_space = pop_int ();
+	make_addr_space = (enum type_instance_flag_value) pop_int ();
 	break;
       case tp_atomic:
 	make_atomic = true;
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-21 14:45 ` [PATCH 2/3] Use type_instance_flags more throughout Pedro Alves
@ 2020-08-25 11:36   ` Luis Machado
  2020-09-14 11:56     ` Pedro Alves
  2020-08-26 10:06   ` Andrew Burgess
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Luis Machado @ 2020-08-25 11:36 UTC (permalink / raw)
  To: Pedro Alves, gdb-patches

Hi,

On 8/21/20 11:45 AM, Pedro Alves wrote:
> The next patch in this series will rewrites enum_flags fixing some API
> holes.  That would cause build failures around code using
> type_instance_flags.  Or rather, that should be using it, but wasn't.
> 
> This patch fixes it by using type_instance_flags throughout instead of
> plain integers.
> 
> Note that we can't make the seemingly obvious change to struct
> type::instance_flags:
> 
>   -  unsigned instance_flags : 9;
>   +  ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9;
> 
> Because G++ complains then that 9 bits isn't sufficient for holding
> all values of type_instance_flag_value.

I ran into the problem above the last time I used instance flags 
(address classes), but mostly because of the (rather silly) 9 bit (or 
whatever current number is there) limitation.

Isn't this more an artifact of trying to save space where there isn't a 
need to anymore? Wouldn't a 64-bit integer suffice here? That would give 
us 64 flags worth of data and virtually no problems until we ran out of 
bits.

Honestly, I don't particularly like the hackery that is going on in 
traits.h, just to catch something that may be easier fixed with a longer 
integer type.

I understand the C++ standard is evolving, but GDB's code base is still 
full of old code. We don't necessarily need to convert GDB's code base 
to use the latest and greatest out there, right? We just need to make it 
good quality and well maintained?

Just an opinion from someone that has used these flags before and has 
been bitten by their storage-space-saving ugliness.

> 
> So the patch adds a cast to TYPE_INSTANCE_FLAGS, and adds a separate
> SET_TYPE_INSTANCE_FLAGS macro.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/read.c (read_tag_pointer_type): Use type_instance_flags.
> 	* eval.c (fake_method::fake_method): Use SET_TYPE_INSTANCE_FLAGS.
> 	* gdbarch.h, gdbarch.c: Regenerate.
> 	* gdbarch.sh (address_class_type_flags): Use type_instance_flags.
> 	(address_class_name_to_type_flags): Use type_instance_flags and
> 	bool.
> 	* gdbtypes.c (address_space_name_to_int)
> 	(address_space_int_to_name, make_qualified_type): Use
> 	type_instance_flags.
> 	(make_qualified_type): Use type_instance_flags and
> 	SET_TYPE_INSTANCE_FLAGS.
> 	(make_type_with_address_space, make_cv_type, make_vector_type)
> 	(check_typedef): Use type_instance_flags.
> 	(recursive_dump_type): Cast type_instance_flags to unsigned for
> 	printing.
> 	(copy_type_recursive): Use SET_TYPE_INSTANCE_FLAGS.
> 	* gdbtypes.h (TYPE_INSTANCE_FLAGS): Return a type_instance_flags.
> 	(SET_TYPE_INSTANCE_FLAGS): New.
> 	(address_space_name_to_int, address_space_int_to_name)
> 	(make_type_with_address_space): Pass flags using
> 	type_instance_flags instead of int.
> 	* stabsread.c (cleanup_undefined_types_noname): Use
> 	SET_TYPE_INSTANCE_FLAGS.
> 	* type-stack.c (type_stack::follow_types): Use type_instance_flags.
> ---
>   gdb/dwarf2/read.c |  7 +++----
>   gdb/eval.c        |  2 +-
>   gdb/gdbarch.c     |  6 +++---
>   gdb/gdbarch.h     | 12 ++++++------
>   gdb/gdbarch.sh    |  8 ++++----
>   gdb/gdbtypes.c    | 58 ++++++++++++++++++++++++++++++-------------------------
>   gdb/gdbtypes.h    | 15 +++++++++-----
>   gdb/stabsread.c   |  2 +-
>   gdb/type-stack.c  |  4 ++--
>   9 files changed, 62 insertions(+), 52 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 0ac8533263..4ced5ac02b 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -17292,10 +17292,9 @@ read_tag_pointer_type (struct die_info *die, struct dwarf2_cu *cu)
>       {
>         if (gdbarch_address_class_type_flags_p (gdbarch))
>   	{
> -	  int type_flags;
> -
> -	  type_flags = gdbarch_address_class_type_flags
> -			 (gdbarch, byte_size, addr_class);
> +	  type_instance_flags type_flags
> +	    = gdbarch_address_class_type_flags (gdbarch, byte_size,
> +						addr_class);
>   	  gdb_assert ((type_flags & ~TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
>   		      == 0);
>   	  type = make_type_with_address_space (type, type_flags);
> diff --git a/gdb/eval.c b/gdb/eval.c
> index c62c35f318..cd300ddfef 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -659,7 +659,7 @@ fake_method::fake_method (type_instance_flags flags,
>     TYPE_LENGTH (type) = 1;
>     type->set_code (TYPE_CODE_METHOD);
>     TYPE_CHAIN (type) = type;
> -  TYPE_INSTANCE_FLAGS (type) = flags;
> +  SET_TYPE_INSTANCE_FLAGS (type, flags);
>     if (num_types > 0)
>       {
>         if (param_types[num_types - 1] == NULL)
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index f8fe03ca68..2be959ecfc 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -3501,7 +3501,7 @@ gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch)
>     return gdbarch->address_class_type_flags != NULL;
>   }
>   
> -int
> +type_instance_flags
>   gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class)
>   {
>     gdb_assert (gdbarch != NULL);
> @@ -3566,8 +3566,8 @@ gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch)
>     return gdbarch->address_class_name_to_type_flags != NULL;
>   }
>   
> -int
> -gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr)
> +bool
> +gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr)
>   {
>     gdb_assert (gdbarch != NULL);
>     gdb_assert (gdbarch->address_class_name_to_type_flags != NULL);
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 7a3060e628..8a4a384fda 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -848,8 +848,8 @@ extern void set_gdbarch_have_nonsteppable_watchpoint (struct gdbarch *gdbarch, i
>   
>   extern int gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch);
>   
> -typedef int (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
> -extern int gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
> +typedef type_instance_flags (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
> +extern type_instance_flags gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
>   extern void set_gdbarch_address_class_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_type_flags_ftype *address_class_type_flags);
>   
>   extern int gdbarch_address_class_type_flags_to_name_p (struct gdbarch *gdbarch);
> @@ -866,13 +866,13 @@ extern bool gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_by
>   extern void set_gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdbarch_execute_dwarf_cfa_vendor_op_ftype *execute_dwarf_cfa_vendor_op);
>   
>   /* Return the appropriate type_flags for the supplied address class.
> -   This function should return 1 if the address class was recognized and
> -   type_flags was set, zero otherwise. */
> +   This function should return true if the address class was recognized and
> +   type_flags was set, false otherwise. */
>   
>   extern int gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch);
>   
> -typedef int (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
> -extern int gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
> +typedef bool (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
> +extern bool gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
>   extern void set_gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_name_to_type_flags_ftype *address_class_name_to_type_flags);
>   
>   /* Is a register in a group */
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 6d3c5c889d..7e9204119b 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -689,16 +689,16 @@ v;int;cannot_step_breakpoint;;;0;0;;0
>   # See comment in target.h about continuable, steppable and
>   # non-steppable watchpoints.
>   v;int;have_nonsteppable_watchpoint;;;0;0;;0
> -F;int;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
> +F;type_instance_flags;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
>   M;const char *;address_class_type_flags_to_name;int type_flags;type_flags
>   # Execute vendor-specific DWARF Call Frame Instruction.  OP is the instruction.
>   # FS are passed from the generic execute_cfa_program function.
>   m;bool;execute_dwarf_cfa_vendor_op;gdb_byte op, struct dwarf2_frame_state *fs;op, fs;;default_execute_dwarf_cfa_vendor_op;;0
>   
>   # Return the appropriate type_flags for the supplied address class.
> -# This function should return 1 if the address class was recognized and
> -# type_flags was set, zero otherwise.
> -M;int;address_class_name_to_type_flags;const char *name, int *type_flags_ptr;name, type_flags_ptr
> +# This function should return true if the address class was recognized and
> +# type_flags was set, false otherwise.
> +M;bool;address_class_name_to_type_flags;const char *name, type_instance_flags *type_flags_ptr;name, type_flags_ptr
>   # Is a register in a group
>   m;int;register_reggroup_p;int regnum, struct reggroup *reggroup;regnum, reggroup;;default_register_reggroup_p;;0
>   # Fetch the pointer to the ith function argument.
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index c1eb03d898..64e44bfe23 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -574,11 +574,11 @@ lookup_function_type_with_arguments (struct type *type,
>   /* Identify address space identifier by name --
>      return the integer flag defined in gdbtypes.h.  */
>   
> -int
> +type_instance_flags
>   address_space_name_to_int (struct gdbarch *gdbarch,
>   			   const char *space_identifier)
>   {
> -  int type_flags;
> +  type_instance_flags type_flags;
>   
>     /* Check for known address space delimiters.  */
>     if (!strcmp (space_identifier, "code"))
> @@ -598,7 +598,8 @@ address_space_name_to_int (struct gdbarch *gdbarch,
>      gdbtypes.h -- return the string version of the adress space name.  */
>   
>   const char *
> -address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
> +address_space_int_to_name (struct gdbarch *gdbarch,
> +			   type_instance_flags space_flag)
>   {
>     if (space_flag & TYPE_INSTANCE_FLAG_CODE_SPACE)
>       return "code";
> @@ -617,7 +618,7 @@ address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
>      STORAGE must be in the same obstack as TYPE.  */
>   
>   static struct type *
> -make_qualified_type (struct type *type, int new_flags,
> +make_qualified_type (struct type *type, type_instance_flags new_flags,
>   		     struct type *storage)
>   {
>     struct type *ntype;
> @@ -657,7 +658,7 @@ make_qualified_type (struct type *type, int new_flags,
>     TYPE_CHAIN (type) = ntype;
>   
>     /* Now set the instance flags and return the new type.  */
> -  TYPE_INSTANCE_FLAGS (ntype) = new_flags;
> +  SET_TYPE_INSTANCE_FLAGS (ntype, new_flags);
>   
>     /* Set length of new type to that of the original type.  */
>     TYPE_LENGTH (ntype) = TYPE_LENGTH (type);
> @@ -675,13 +676,14 @@ make_qualified_type (struct type *type, int new_flags,
>      representations.  */
>   
>   struct type *
> -make_type_with_address_space (struct type *type, int space_flag)
> +make_type_with_address_space (struct type *type,
> +			      type_instance_flags space_flag)
>   {
> -  int new_flags = ((TYPE_INSTANCE_FLAGS (type)
> -		    & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
> -			| TYPE_INSTANCE_FLAG_DATA_SPACE
> -		        | TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
> -		   | space_flag);
> +  type_instance_flags new_flags = ((TYPE_INSTANCE_FLAGS (type)
> +				    & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
> +					| TYPE_INSTANCE_FLAG_DATA_SPACE
> +					| TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
> +				   | space_flag);
>   
>     return make_qualified_type (type, new_flags, NULL);
>   }
> @@ -705,9 +707,9 @@ make_cv_type (int cnst, int voltl,
>   {
>     struct type *ntype;	/* New type */
>   
> -  int new_flags = (TYPE_INSTANCE_FLAGS (type)
> -		   & ~(TYPE_INSTANCE_FLAG_CONST
> -		       | TYPE_INSTANCE_FLAG_VOLATILE));
> +  type_instance_flags new_flags = (TYPE_INSTANCE_FLAGS (type)
> +				   & ~(TYPE_INSTANCE_FLAG_CONST
> +				       | TYPE_INSTANCE_FLAG_VOLATILE));
>   
>     if (cnst)
>       new_flags |= TYPE_INSTANCE_FLAG_CONST;
> @@ -1410,7 +1412,6 @@ void
>   make_vector_type (struct type *array_type)
>   {
>     struct type *inner_array, *elt_type;
> -  int flags;
>   
>     /* Find the innermost array type, in case the array is
>        multi-dimensional.  */
> @@ -1421,7 +1422,8 @@ make_vector_type (struct type *array_type)
>     elt_type = TYPE_TARGET_TYPE (inner_array);
>     if (elt_type->code () == TYPE_CODE_INT)
>       {
> -      flags = TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
> +      type_instance_flags flags
> +	= TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
>         elt_type = make_qualified_type (elt_type, flags, NULL);
>         TYPE_TARGET_TYPE (inner_array) = elt_type;
>       }
> @@ -2732,12 +2734,13 @@ struct type *
>   check_typedef (struct type *type)
>   {
>     struct type *orig_type = type;
> -  /* While we're removing typedefs, we don't want to lose qualifiers.
> -     E.g., const/volatile.  */
> -  int instance_flags = TYPE_INSTANCE_FLAGS (type);
>   
>     gdb_assert (type);
>   
> +  /* While we're removing typedefs, we don't want to lose qualifiers.
> +     E.g., const/volatile.  */
> +  type_instance_flags instance_flags = TYPE_INSTANCE_FLAGS (type);
> +
>     while (type->code () == TYPE_CODE_TYPEDEF)
>       {
>         if (!TYPE_TARGET_TYPE (type))
> @@ -2778,10 +2781,13 @@ check_typedef (struct type *type)
>   	 outer cast in a chain of casting win), instead of assuming
>   	 "it can't happen".  */
>         {
> -	const int ALL_SPACES = (TYPE_INSTANCE_FLAG_CODE_SPACE
> -				| TYPE_INSTANCE_FLAG_DATA_SPACE);
> -	const int ALL_CLASSES = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
> -	int new_instance_flags = TYPE_INSTANCE_FLAGS (type);
> +	const type_instance_flags ALL_SPACES
> +	  = (TYPE_INSTANCE_FLAG_CODE_SPACE
> +	     | TYPE_INSTANCE_FLAG_DATA_SPACE);
> +	const type_instance_flags ALL_CLASSES
> +	  = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
> +	type_instance_flags new_instance_flags
> +	  = TYPE_INSTANCE_FLAGS (type);
>   
>   	/* Treat code vs data spaces and address classes separately.  */
>   	if ((instance_flags & ALL_SPACES) != 0)
> @@ -5026,7 +5032,7 @@ recursive_dump_type (struct type *type, int spaces)
>     gdb_print_host_address (TYPE_CHAIN (type), gdb_stdout);
>     printf_filtered ("\n");
>     printfi_filtered (spaces, "instance_flags 0x%x",
> -		    TYPE_INSTANCE_FLAGS (type));
> +		    (unsigned) TYPE_INSTANCE_FLAGS (type));
>     if (TYPE_CONST (type))
>       {
>         puts_filtered (" TYPE_CONST");
> @@ -5300,7 +5306,7 @@ copy_type_recursive (struct objfile *objfile,
>     if (type->name ())
>       new_type->set_name (xstrdup (type->name ()));
>   
> -  TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
> +  SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
>     TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
>   
>     /* Copy the fields.  */
> @@ -5427,7 +5433,7 @@ copy_type (const struct type *type)
>     gdb_assert (TYPE_OBJFILE_OWNED (type));
>   
>     new_type = alloc_type_copy (type);
> -  TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
> +  SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
>     TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
>     memcpy (TYPE_MAIN_TYPE (new_type), TYPE_MAIN_TYPE (type),
>   	  sizeof (struct main_type));
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 55a6dafb7e..b42cef6137 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1585,7 +1585,10 @@ extern void allocate_gnat_aux_type (struct type *);
>        TYPE_ZALLOC (type,							       \
>   		  sizeof (*TYPE_MAIN_TYPE (type)->type_specific.func_stuff)))
>   
> -#define TYPE_INSTANCE_FLAGS(thistype) (thistype)->instance_flags
> +#define TYPE_INSTANCE_FLAGS(thistype) \
> +  type_instance_flags ((enum type_instance_flag_value) (thistype)->instance_flags)
> +#define SET_TYPE_INSTANCE_FLAGS(thistype, flags) \
> +  (thistype)->instance_flags = flags
>   #define TYPE_MAIN_TYPE(thistype) (thistype)->main_type
>   #define TYPE_TARGET_TYPE(thistype) TYPE_MAIN_TYPE(thistype)->target_type
>   #define TYPE_POINTER_TYPE(thistype) (thistype)->pointer_type
> @@ -2117,12 +2120,14 @@ extern struct type *make_atomic_type (struct type *);
>   
>   extern void replace_type (struct type *, struct type *);
>   
> -extern int address_space_name_to_int (struct gdbarch *, const char *);
> +extern type_instance_flags address_space_name_to_int (struct gdbarch *,
> +						      const char *);
>   
> -extern const char *address_space_int_to_name (struct gdbarch *, int);
> +extern const char *address_space_int_to_name (struct gdbarch *,
> +					      type_instance_flags);
>   
> -extern struct type *make_type_with_address_space (struct type *type,
> -						  int space_identifier);
> +extern struct type *make_type_with_address_space
> +  (struct type *type, type_instance_flags space_identifier);
>   
>   extern struct type *lookup_memberptr_type (struct type *, struct type *);
>   
> diff --git a/gdb/stabsread.c b/gdb/stabsread.c
> index d2ff54a47b..ed31dc0111 100644
> --- a/gdb/stabsread.c
> +++ b/gdb/stabsread.c
> @@ -4397,7 +4397,7 @@ cleanup_undefined_types_noname (struct objfile *objfile)
>                and needs to be copied over from the reference type.
>                Since replace_type expects them to be identical, we need
>                to set these flags manually before hand.  */
> -          TYPE_INSTANCE_FLAGS (nat.type) = TYPE_INSTANCE_FLAGS (*type);
> +          SET_TYPE_INSTANCE_FLAGS (nat.type, TYPE_INSTANCE_FLAGS (*type));
>             replace_type (nat.type, *type);
>           }
>       }
> diff --git a/gdb/type-stack.c b/gdb/type-stack.c
> index f8661d7565..608142c849 100644
> --- a/gdb/type-stack.c
> +++ b/gdb/type-stack.c
> @@ -109,7 +109,7 @@ type_stack::follow_types (struct type *follow_type)
>     int done = 0;
>     int make_const = 0;
>     int make_volatile = 0;
> -  int make_addr_space = 0;
> +  type_instance_flags make_addr_space = 0;
>     bool make_restrict = false;
>     bool make_atomic = false;
>     int array_size;
> @@ -128,7 +128,7 @@ type_stack::follow_types (struct type *follow_type)
>   	make_volatile = 1;
>   	break;
>         case tp_space_identifier:
> -	make_addr_space = pop_int ();
> +	make_addr_space = (enum type_instance_flag_value) pop_int ();
>   	break;
>         case tp_atomic:
>   	make_atomic = true;
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-25 11:36   ` Luis Machado
@ 2020-09-14 11:56     ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2020-09-14 11:56 UTC (permalink / raw)
  To: Luis Machado, gdb-patches

Hi Luis,

I should have replied to this promptly.  Sorry about that.

On 8/25/20 12:36 PM, Luis Machado wrote:
> Hi,
> 
> On 8/21/20 11:45 AM, Pedro Alves wrote:
>> The next patch in this series will rewrites enum_flags fixing some API
>> holes.  That would cause build failures around code using
>> type_instance_flags.  Or rather, that should be using it, but wasn't.
>>
>> This patch fixes it by using type_instance_flags throughout instead of
>> plain integers.
>>
>> Note that we can't make the seemingly obvious change to struct
>> type::instance_flags:
>>
>>   -  unsigned instance_flags : 9;
>>   +  ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9;
>>
>> Because G++ complains then that 9 bits isn't sufficient for holding
>> all values of type_instance_flag_value.
> 
> I ran into the problem above the last time I used instance flags (address classes), but mostly because of the (rather silly) 9 bit (or whatever current number is there) limitation.
> 
> Isn't this more an artifact of trying to save space where there isn't a need to anymore? Wouldn't a 64-bit integer suffice here? That would give us 64 flags worth of data and virtually no problems until we ran out of bits.
> 

Well, I'm not exactly sure about struct type, but symbol related
structures are space sensitive, a few bytes here and there can make
a significant difference when you have thousands or millions of 
instances.  Once in a while someone tries to shrink these structures,
see if we can pack fields better by reordering fields, etc.

Here's what we have today, on x86-64:

 (top-gdb) ptype /o type
 /* offset    |  size */  type = struct type {
 /*    0      |     8 */    type *pointer_type;
 /*    8      |     8 */    type *reference_type;
 /*   16      |     8 */    type *rvalue_reference_type;
 /*   24      |     8 */    type *chain;
 /*   32: 0   |     4 */    unsigned int align_log2 : 8;
 /*   33: 0   |     4 */    unsigned int instance_flags : 9;
 /* XXX  7-bit hole   */
 /* XXX  5-byte hole  */
 /*   40      |     8 */    ULONGEST length;
 /*   48      |     8 */    main_type *main_type;
 
                            /* total size (bytes):   56 */
                          }

And if we blindly made instance_flags 64-bit:

 (top-gdb) ptype /o type
 /* offset    |  size */  type = struct type {
 /*    0      |     8 */    type *pointer_type;
 /*    8      |     8 */    type *reference_type;
 /*   16      |     8 */    type *rvalue_reference_type;
 /*   24      |     8 */    type *chain;
 /*   32: 0   |     4 */    unsigned int align_log2 : 8;
 /* XXX  7-byte hole  */
 /*   40      |     8 */    ULONGEST instance_flags;
 /*   48      |     8 */    ULONGEST length;
 /*   56      |     8 */    main_type *main_type;
 
                            /* total size (bytes):   64 */
                          }

Maybe we will need 64 bits for flags in the future anyway, I don't
know.

> Honestly, I don't particularly like the hackery that is going on in traits.h, just to catch something that may be easier fixed with a longer integer type.
> 
> I understand the C++ standard is evolving, but GDB's code base is still full of old code. We don't necessarily need to convert GDB's code base to use the latest and greatest out there, right? We just need to make it good quality and well maintained?

The "hackery" is basically switching to the detection idiom that's now standard in C++17.  We
can remove our implementation in traits.h once we start requiring C++17.  But the "hackery" is
used in the enum flags unit tests -- even if instance_flags was made a plain 64-bits enum, I'd
still want the "hackery".  It's orthogonal to instance_flags.  This patch could be the first
in the series, before the traits.h change.  In fact, I'll do just that.

> 
> Just an opinion from someone that has used these flags before and has been bitten by their storage-space-saving ugliness.
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-21 14:45 ` [PATCH 2/3] Use type_instance_flags more throughout Pedro Alves
  2020-08-25 11:36   ` Luis Machado
@ 2020-08-26 10:06   ` Andrew Burgess
  2020-09-14 12:54     ` Pedro Alves
  2020-08-26 15:21   ` Andrew Burgess
  2020-09-11 20:21   ` Tom Tromey
  3 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2020-08-26 10:06 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

* Pedro Alves <pedro@palves.net> [2020-08-21 15:45:22 +0100]:

> The next patch in this series will rewrites enum_flags fixing some API
> holes.  That would cause build failures around code using
> type_instance_flags.  Or rather, that should be using it, but wasn't.
> 
> This patch fixes it by using type_instance_flags throughout instead of
> plain integers.
> 
> Note that we can't make the seemingly obvious change to struct
> type::instance_flags:
> 
>  -  unsigned instance_flags : 9;
>  +  ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9;
> 
> Because G++ complains then that 9 bits isn't sufficient for holding
> all values of type_instance_flag_value.
> 
> So the patch adds a cast to TYPE_INSTANCE_FLAGS, and adds a separate
> SET_TYPE_INSTANCE_FLAGS macro.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/read.c (read_tag_pointer_type): Use type_instance_flags.
> 	* eval.c (fake_method::fake_method): Use SET_TYPE_INSTANCE_FLAGS.
> 	* gdbarch.h, gdbarch.c: Regenerate.
> 	* gdbarch.sh (address_class_type_flags): Use type_instance_flags.
> 	(address_class_name_to_type_flags): Use type_instance_flags and
> 	bool.
> 	* gdbtypes.c (address_space_name_to_int)
> 	(address_space_int_to_name, make_qualified_type): Use
> 	type_instance_flags.
> 	(make_qualified_type): Use type_instance_flags and
> 	SET_TYPE_INSTANCE_FLAGS.
> 	(make_type_with_address_space, make_cv_type, make_vector_type)
> 	(check_typedef): Use type_instance_flags.
> 	(recursive_dump_type): Cast type_instance_flags to unsigned for
> 	printing.
> 	(copy_type_recursive): Use SET_TYPE_INSTANCE_FLAGS.
> 	* gdbtypes.h (TYPE_INSTANCE_FLAGS): Return a type_instance_flags.
> 	(SET_TYPE_INSTANCE_FLAGS): New.
> 	(address_space_name_to_int, address_space_int_to_name)
> 	(make_type_with_address_space): Pass flags using
> 	type_instance_flags instead of int.
> 	* stabsread.c (cleanup_undefined_types_noname): Use
> 	SET_TYPE_INSTANCE_FLAGS.
> 	* type-stack.c (type_stack::follow_types): Use type_instance_flags.

Pedro,

While testing this I found a few issues building with
--enable-targets=all.  The patch below fixes all of the issues I ran
into.  Feel free to merge this with your work of keep it as a separate
patch, whatever works for you.

Thanks,
Andrew

---

commit b6c8d1ca8077fa7ea2d6f6336b2115b3dcf9dfbb
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed Aug 26 11:02:22 2020 +0100

    gdb: additional changes to make use of type_instance_flags more
    
    Further updates to make use of type_instance_flags.
    
    gdb/ChangeLog:
    
            * avr-tdep.c (avr_address_class_type_flags): Update return type.
            (avr_address_class_type_flags_to_name): Update argument types.
            (avr_address_class_name_to_type_flags): Update return type and
            argument types.
            * ft32-tdep.c (ft32_address_class_type_flags): Update return type.
            (ft32_address_class_type_flags_to_name): Update argument types.
            (ft32_address_class_name_to_type_flags): Update return type and
            argument types.
            * gdbarch.c: Regenerate.
            * gdbarch.h: Regenerate.
            * gdbarch.sh (address_class_type_flags_to_name): Update argument
            types.
            * s390-tdep.c (s390_address_class_type_flags): Update return type.
            (s390_address_class_type_flags_to_name): Update argument types.
            (s390_address_class_name_to_type_flags): Update return type and
            argument types.

diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index 74ab531711e..0148f4e4db2 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -1372,7 +1372,7 @@ avr_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
    This method maps DW_AT_address_class attributes to a
    type_instance_flag_value.  */
 
-static int
+static type_instance_flags
 avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
 {
   /* The value 1 of the DW_AT_address_class attribute corresponds to the
@@ -1389,7 +1389,8 @@ avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
    Convert a type_instance_flag_value to an address space qualifier.  */
 
 static const char*
-avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
+avr_address_class_type_flags_to_name (struct gdbarch *gdbarch,
+				      type_instance_flags type_flags)
 {
   if (type_flags & AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH)
     return "flash";
@@ -1401,18 +1402,18 @@ avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
 
    Convert an address space qualifier to a type_instance_flag_value.  */
 
-static int
+static bool
 avr_address_class_name_to_type_flags (struct gdbarch *gdbarch,
                                       const char* name,
-                                      int *type_flags_ptr)
+                                      type_instance_flags *type_flags_ptr)
 {
   if (strcmp (name, "flash") == 0)
     {
       *type_flags_ptr = AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
-      return 1;
+      return true;
     }
   else
-    return 0;
+    return false;
 }
 
 /* Initialize the gdbarch structure for the AVR's.  */
diff --git a/gdb/ft32-tdep.c b/gdb/ft32-tdep.c
index 99ef69de770..8ce16c06505 100644
--- a/gdb/ft32-tdep.c
+++ b/gdb/ft32-tdep.c
@@ -341,7 +341,7 @@ ft32_pointer_to_address (struct gdbarch *gdbarch,
    This method maps DW_AT_address_class attributes to a
    type_instance_flag_value.  */
 
-static int
+static type_instance_flags
 ft32_address_class_type_flags (int byte_size, int dwarf2_addr_class)
 {
   /* The value 1 of the DW_AT_address_class attribute corresponds to the
@@ -357,7 +357,8 @@ ft32_address_class_type_flags (int byte_size, int dwarf2_addr_class)
    Convert a type_instance_flag_value to an address space qualifier.  */
 
 static const char*
-ft32_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
+ft32_address_class_type_flags_to_name (struct gdbarch *gdbarch,
+				       type_instance_flags type_flags)
 {
   if (type_flags & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1)
     return "flash";
@@ -369,18 +370,18 @@ ft32_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
 
    Convert an address space qualifier to a type_instance_flag_value.  */
 
-static int
+static bool
 ft32_address_class_name_to_type_flags (struct gdbarch *gdbarch,
 				       const char* name,
-				       int *type_flags_ptr)
+				       type_instance_flags *type_flags_ptr)
 {
   if (strcmp (name, "flash") == 0)
     {
       *type_flags_ptr = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1;
-      return 1;
+      return true;
     }
   else
-    return 0;
+    return false;
 }
 
 /* Given a return value in `regbuf' with a type `valtype',
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 2be959ecfc9..a3a67020078 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -3526,7 +3526,7 @@ gdbarch_address_class_type_flags_to_name_p (struct gdbarch *gdbarch)
 }
 
 const char *
-gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
+gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, type_instance_flags type_flags)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->address_class_type_flags_to_name != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 8a4a384fda9..5ac4f5495c4 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -854,8 +854,8 @@ extern void set_gdbarch_address_class_type_flags (struct gdbarch *gdbarch, gdbar
 
 extern int gdbarch_address_class_type_flags_to_name_p (struct gdbarch *gdbarch);
 
-typedef const char * (gdbarch_address_class_type_flags_to_name_ftype) (struct gdbarch *gdbarch, int type_flags);
-extern const char * gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags);
+typedef const char * (gdbarch_address_class_type_flags_to_name_ftype) (struct gdbarch *gdbarch, type_instance_flags type_flags);
+extern const char * gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, type_instance_flags type_flags);
 extern void set_gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, gdbarch_address_class_type_flags_to_name_ftype *address_class_type_flags_to_name);
 
 /* Execute vendor-specific DWARF Call Frame Instruction.  OP is the instruction.
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 7e9204119bd..a64afb5a3d2 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -690,7 +690,7 @@ v;int;cannot_step_breakpoint;;;0;0;;0
 # non-steppable watchpoints.
 v;int;have_nonsteppable_watchpoint;;;0;0;;0
 F;type_instance_flags;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
-M;const char *;address_class_type_flags_to_name;int type_flags;type_flags
+M;const char *;address_class_type_flags_to_name;type_instance_flags type_flags;type_flags
 # Execute vendor-specific DWARF Call Frame Instruction.  OP is the instruction.
 # FS are passed from the generic execute_cfa_program function.
 m;bool;execute_dwarf_cfa_vendor_op;gdb_byte op, struct dwarf2_frame_state *fs;op, fs;;default_execute_dwarf_cfa_vendor_op;;0
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 65cb23705d2..49a507f7c2d 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -1583,7 +1583,7 @@ s390_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR addr)
 /* Implement addr_class_type_flags gdbarch method.
    Only used for ABI_LINUX_ZSERIES.  */
 
-static int
+static type_instance_flags
 s390_address_class_type_flags (int byte_size, int dwarf2_addr_class)
 {
   if (byte_size == 4)
@@ -1596,7 +1596,8 @@ s390_address_class_type_flags (int byte_size, int dwarf2_addr_class)
    Only used for ABI_LINUX_ZSERIES.  */
 
 static const char *
-s390_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
+s390_address_class_type_flags_to_name (struct gdbarch *gdbarch,
+				       type_instance_flags type_flags)
 {
   if (type_flags & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1)
     return "mode32";
@@ -1607,18 +1608,18 @@ s390_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
 /* Implement addr_class_name_to_type_flags gdbarch method.
    Only used for ABI_LINUX_ZSERIES.  */
 
-static int
+static bool
 s390_address_class_name_to_type_flags (struct gdbarch *gdbarch,
 				       const char *name,
-				       int *type_flags_ptr)
+				       type_instance_flags *type_flags_ptr)
 {
   if (strcmp (name, "mode32") == 0)
     {
       *type_flags_ptr = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1;
-      return 1;
+      return true;
     }
   else
-    return 0;
+    return false;
 }
 
 /* Inferior function calls.  */

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-26 10:06   ` Andrew Burgess
@ 2020-09-14 12:54     ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2020-09-14 12:54 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 8/26/20 11:06 AM, Andrew Burgess wrote:

> While testing this I found a few issues building with
> --enable-targets=all.  The patch below fixes all of the issues I ran
> into.  Feel free to merge this with your work of keep it as a separate
> patch, whatever works for you.

Thanks Andrew.  Sorry I forgot to --enable-targets=all in the first place.
I saw those places needed adjustment when I was grepping around, but then
forgot.

I'll merge this with my patch and add you to the ChangeLog entry.

Pedro

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-21 14:45 ` [PATCH 2/3] Use type_instance_flags more throughout Pedro Alves
  2020-08-25 11:36   ` Luis Machado
  2020-08-26 10:06   ` Andrew Burgess
@ 2020-08-26 15:21   ` Andrew Burgess
  2020-09-14 13:53     ` Pedro Alves
  2020-09-11 20:21   ` Tom Tromey
  3 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2020-08-26 15:21 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

* Pedro Alves <pedro@palves.net> [2020-08-21 15:45:22 +0100]:

> The next patch in this series will rewrites enum_flags fixing some API
> holes.  That would cause build failures around code using
> type_instance_flags.  Or rather, that should be using it, but wasn't.
> 
> This patch fixes it by using type_instance_flags throughout instead of
> plain integers.
> 
> Note that we can't make the seemingly obvious change to struct
> type::instance_flags:
> 
>  -  unsigned instance_flags : 9;
>  +  ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9;
> 
> Because G++ complains then that 9 bits isn't sufficient for holding
> all values of type_instance_flag_value.
> 
> So the patch adds a cast to TYPE_INSTANCE_FLAGS, and adds a separate
> SET_TYPE_INSTANCE_FLAGS macro.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/read.c (read_tag_pointer_type): Use type_instance_flags.
> 	* eval.c (fake_method::fake_method): Use SET_TYPE_INSTANCE_FLAGS.
> 	* gdbarch.h, gdbarch.c: Regenerate.
> 	* gdbarch.sh (address_class_type_flags): Use type_instance_flags.
> 	(address_class_name_to_type_flags): Use type_instance_flags and
> 	bool.
> 	* gdbtypes.c (address_space_name_to_int)
> 	(address_space_int_to_name, make_qualified_type): Use
> 	type_instance_flags.
> 	(make_qualified_type): Use type_instance_flags and
> 	SET_TYPE_INSTANCE_FLAGS.
> 	(make_type_with_address_space, make_cv_type, make_vector_type)
> 	(check_typedef): Use type_instance_flags.
> 	(recursive_dump_type): Cast type_instance_flags to unsigned for
> 	printing.
> 	(copy_type_recursive): Use SET_TYPE_INSTANCE_FLAGS.
> 	* gdbtypes.h (TYPE_INSTANCE_FLAGS): Return a type_instance_flags.
> 	(SET_TYPE_INSTANCE_FLAGS): New.
> 	(address_space_name_to_int, address_space_int_to_name)
> 	(make_type_with_address_space): Pass flags using
> 	type_instance_flags instead of int.
> 	* stabsread.c (cleanup_undefined_types_noname): Use
> 	SET_TYPE_INSTANCE_FLAGS.
> 	* type-stack.c (type_stack::follow_types): Use type_instance_flags.
> ---
>  gdb/dwarf2/read.c |  7 +++----
>  gdb/eval.c        |  2 +-
>  gdb/gdbarch.c     |  6 +++---
>  gdb/gdbarch.h     | 12 ++++++------
>  gdb/gdbarch.sh    |  8 ++++----
>  gdb/gdbtypes.c    | 58 ++++++++++++++++++++++++++++++-------------------------
>  gdb/gdbtypes.h    | 15 +++++++++-----
>  gdb/stabsread.c   |  2 +-
>  gdb/type-stack.c  |  4 ++--
>  9 files changed, 62 insertions(+), 52 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 0ac8533263..4ced5ac02b 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -17292,10 +17292,9 @@ read_tag_pointer_type (struct die_info *die, struct dwarf2_cu *cu)
>      {
>        if (gdbarch_address_class_type_flags_p (gdbarch))
>  	{
> -	  int type_flags;
> -
> -	  type_flags = gdbarch_address_class_type_flags
> -			 (gdbarch, byte_size, addr_class);
> +	  type_instance_flags type_flags
> +	    = gdbarch_address_class_type_flags (gdbarch, byte_size,
> +						addr_class);
>  	  gdb_assert ((type_flags & ~TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
>  		      == 0);
>  	  type = make_type_with_address_space (type, type_flags);
> diff --git a/gdb/eval.c b/gdb/eval.c
> index c62c35f318..cd300ddfef 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -659,7 +659,7 @@ fake_method::fake_method (type_instance_flags flags,
>    TYPE_LENGTH (type) = 1;
>    type->set_code (TYPE_CODE_METHOD);
>    TYPE_CHAIN (type) = type;
> -  TYPE_INSTANCE_FLAGS (type) = flags;
> +  SET_TYPE_INSTANCE_FLAGS (type, flags);
>    if (num_types > 0)
>      {
>        if (param_types[num_types - 1] == NULL)
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index f8fe03ca68..2be959ecfc 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -3501,7 +3501,7 @@ gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch)
>    return gdbarch->address_class_type_flags != NULL;
>  }
>  
> -int
> +type_instance_flags
>  gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class)
>  {
>    gdb_assert (gdbarch != NULL);
> @@ -3566,8 +3566,8 @@ gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch)
>    return gdbarch->address_class_name_to_type_flags != NULL;
>  }
>  
> -int
> -gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr)
> +bool
> +gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->address_class_name_to_type_flags != NULL);
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 7a3060e628..8a4a384fda 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -848,8 +848,8 @@ extern void set_gdbarch_have_nonsteppable_watchpoint (struct gdbarch *gdbarch, i
>  
>  extern int gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch);
>  
> -typedef int (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
> -extern int gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
> +typedef type_instance_flags (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
> +extern type_instance_flags gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
>  extern void set_gdbarch_address_class_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_type_flags_ftype *address_class_type_flags);
>  
>  extern int gdbarch_address_class_type_flags_to_name_p (struct gdbarch *gdbarch);
> @@ -866,13 +866,13 @@ extern bool gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_by
>  extern void set_gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdbarch_execute_dwarf_cfa_vendor_op_ftype *execute_dwarf_cfa_vendor_op);
>  
>  /* Return the appropriate type_flags for the supplied address class.
> -   This function should return 1 if the address class was recognized and
> -   type_flags was set, zero otherwise. */
> +   This function should return true if the address class was recognized and
> +   type_flags was set, false otherwise. */
>  
>  extern int gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch);
>  
> -typedef int (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
> -extern int gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
> +typedef bool (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
> +extern bool gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
>  extern void set_gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_name_to_type_flags_ftype *address_class_name_to_type_flags);
>  
>  /* Is a register in a group */
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 6d3c5c889d..7e9204119b 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -689,16 +689,16 @@ v;int;cannot_step_breakpoint;;;0;0;;0
>  # See comment in target.h about continuable, steppable and
>  # non-steppable watchpoints.
>  v;int;have_nonsteppable_watchpoint;;;0;0;;0
> -F;int;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
> +F;type_instance_flags;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
>  M;const char *;address_class_type_flags_to_name;int type_flags;type_flags
>  # Execute vendor-specific DWARF Call Frame Instruction.  OP is the instruction.
>  # FS are passed from the generic execute_cfa_program function.
>  m;bool;execute_dwarf_cfa_vendor_op;gdb_byte op, struct dwarf2_frame_state *fs;op, fs;;default_execute_dwarf_cfa_vendor_op;;0
>  
>  # Return the appropriate type_flags for the supplied address class.
> -# This function should return 1 if the address class was recognized and
> -# type_flags was set, zero otherwise.
> -M;int;address_class_name_to_type_flags;const char *name, int *type_flags_ptr;name, type_flags_ptr
> +# This function should return true if the address class was recognized and
> +# type_flags was set, false otherwise.
> +M;bool;address_class_name_to_type_flags;const char *name, type_instance_flags *type_flags_ptr;name, type_flags_ptr
>  # Is a register in a group
>  m;int;register_reggroup_p;int regnum, struct reggroup *reggroup;regnum, reggroup;;default_register_reggroup_p;;0
>  # Fetch the pointer to the ith function argument.
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index c1eb03d898..64e44bfe23 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -574,11 +574,11 @@ lookup_function_type_with_arguments (struct type *type,
>  /* Identify address space identifier by name --
>     return the integer flag defined in gdbtypes.h.  */
>  
> -int
> +type_instance_flags
>  address_space_name_to_int (struct gdbarch *gdbarch,
>  			   const char *space_identifier)
>  {
> -  int type_flags;
> +  type_instance_flags type_flags;
>  
>    /* Check for known address space delimiters.  */
>    if (!strcmp (space_identifier, "code"))
> @@ -598,7 +598,8 @@ address_space_name_to_int (struct gdbarch *gdbarch,
>     gdbtypes.h -- return the string version of the adress space name.  */
>  
>  const char *
> -address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
> +address_space_int_to_name (struct gdbarch *gdbarch,
> +			   type_instance_flags space_flag)
>  {
>    if (space_flag & TYPE_INSTANCE_FLAG_CODE_SPACE)
>      return "code";
> @@ -617,7 +618,7 @@ address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
>     STORAGE must be in the same obstack as TYPE.  */
>  
>  static struct type *
> -make_qualified_type (struct type *type, int new_flags,
> +make_qualified_type (struct type *type, type_instance_flags new_flags,
>  		     struct type *storage)
>  {
>    struct type *ntype;
> @@ -657,7 +658,7 @@ make_qualified_type (struct type *type, int new_flags,
>    TYPE_CHAIN (type) = ntype;
>  
>    /* Now set the instance flags and return the new type.  */
> -  TYPE_INSTANCE_FLAGS (ntype) = new_flags;
> +  SET_TYPE_INSTANCE_FLAGS (ntype, new_flags);
>  
>    /* Set length of new type to that of the original type.  */
>    TYPE_LENGTH (ntype) = TYPE_LENGTH (type);
> @@ -675,13 +676,14 @@ make_qualified_type (struct type *type, int new_flags,
>     representations.  */
>  
>  struct type *
> -make_type_with_address_space (struct type *type, int space_flag)
> +make_type_with_address_space (struct type *type,
> +			      type_instance_flags space_flag)
>  {
> -  int new_flags = ((TYPE_INSTANCE_FLAGS (type)
> -		    & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
> -			| TYPE_INSTANCE_FLAG_DATA_SPACE
> -		        | TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
> -		   | space_flag);
> +  type_instance_flags new_flags = ((TYPE_INSTANCE_FLAGS (type)
> +				    & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
> +					| TYPE_INSTANCE_FLAG_DATA_SPACE
> +					| TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
> +				   | space_flag);
>  
>    return make_qualified_type (type, new_flags, NULL);
>  }
> @@ -705,9 +707,9 @@ make_cv_type (int cnst, int voltl,
>  {
>    struct type *ntype;	/* New type */
>  
> -  int new_flags = (TYPE_INSTANCE_FLAGS (type)
> -		   & ~(TYPE_INSTANCE_FLAG_CONST 
> -		       | TYPE_INSTANCE_FLAG_VOLATILE));
> +  type_instance_flags new_flags = (TYPE_INSTANCE_FLAGS (type)
> +				   & ~(TYPE_INSTANCE_FLAG_CONST
> +				       | TYPE_INSTANCE_FLAG_VOLATILE));
>  
>    if (cnst)
>      new_flags |= TYPE_INSTANCE_FLAG_CONST;
> @@ -1410,7 +1412,6 @@ void
>  make_vector_type (struct type *array_type)
>  {
>    struct type *inner_array, *elt_type;
> -  int flags;
>  
>    /* Find the innermost array type, in case the array is
>       multi-dimensional.  */
> @@ -1421,7 +1422,8 @@ make_vector_type (struct type *array_type)
>    elt_type = TYPE_TARGET_TYPE (inner_array);
>    if (elt_type->code () == TYPE_CODE_INT)
>      {
> -      flags = TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
> +      type_instance_flags flags
> +	= TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
>        elt_type = make_qualified_type (elt_type, flags, NULL);
>        TYPE_TARGET_TYPE (inner_array) = elt_type;
>      }
> @@ -2732,12 +2734,13 @@ struct type *
>  check_typedef (struct type *type)
>  {
>    struct type *orig_type = type;
> -  /* While we're removing typedefs, we don't want to lose qualifiers.
> -     E.g., const/volatile.  */
> -  int instance_flags = TYPE_INSTANCE_FLAGS (type);
>  
>    gdb_assert (type);
>  
> +  /* While we're removing typedefs, we don't want to lose qualifiers.
> +     E.g., const/volatile.  */
> +  type_instance_flags instance_flags = TYPE_INSTANCE_FLAGS (type);
> +
>    while (type->code () == TYPE_CODE_TYPEDEF)
>      {
>        if (!TYPE_TARGET_TYPE (type))
> @@ -2778,10 +2781,13 @@ check_typedef (struct type *type)
>  	 outer cast in a chain of casting win), instead of assuming
>  	 "it can't happen".  */
>        {
> -	const int ALL_SPACES = (TYPE_INSTANCE_FLAG_CODE_SPACE
> -				| TYPE_INSTANCE_FLAG_DATA_SPACE);
> -	const int ALL_CLASSES = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
> -	int new_instance_flags = TYPE_INSTANCE_FLAGS (type);
> +	const type_instance_flags ALL_SPACES
> +	  = (TYPE_INSTANCE_FLAG_CODE_SPACE
> +	     | TYPE_INSTANCE_FLAG_DATA_SPACE);
> +	const type_instance_flags ALL_CLASSES
> +	  = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
> +	type_instance_flags new_instance_flags
> +	  = TYPE_INSTANCE_FLAGS (type);
>  
>  	/* Treat code vs data spaces and address classes separately.  */
>  	if ((instance_flags & ALL_SPACES) != 0)
> @@ -5026,7 +5032,7 @@ recursive_dump_type (struct type *type, int spaces)
>    gdb_print_host_address (TYPE_CHAIN (type), gdb_stdout);
>    printf_filtered ("\n");
>    printfi_filtered (spaces, "instance_flags 0x%x", 
> -		    TYPE_INSTANCE_FLAGS (type));
> +		    (unsigned) TYPE_INSTANCE_FLAGS (type));
>    if (TYPE_CONST (type))
>      {
>        puts_filtered (" TYPE_CONST");
> @@ -5300,7 +5306,7 @@ copy_type_recursive (struct objfile *objfile,
>    if (type->name ())
>      new_type->set_name (xstrdup (type->name ()));
>  
> -  TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
> +  SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
>    TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
>  
>    /* Copy the fields.  */
> @@ -5427,7 +5433,7 @@ copy_type (const struct type *type)
>    gdb_assert (TYPE_OBJFILE_OWNED (type));
>  
>    new_type = alloc_type_copy (type);
> -  TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
> +  SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
>    TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
>    memcpy (TYPE_MAIN_TYPE (new_type), TYPE_MAIN_TYPE (type),
>  	  sizeof (struct main_type));
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 55a6dafb7e..b42cef6137 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1585,7 +1585,10 @@ extern void allocate_gnat_aux_type (struct type *);
>       TYPE_ZALLOC (type,							       \
>  		  sizeof (*TYPE_MAIN_TYPE (type)->type_specific.func_stuff)))
>  
> -#define TYPE_INSTANCE_FLAGS(thistype) (thistype)->instance_flags
> +#define TYPE_INSTANCE_FLAGS(thistype) \
> +  type_instance_flags ((enum type_instance_flag_value) (thistype)->instance_flags)
> +#define SET_TYPE_INSTANCE_FLAGS(thistype, flags) \
> +  (thistype)->instance_flags = flags

There are some places where you've not updated a use of
TYPE_INSTANCE_FLAGS.  The patch below fixes these.  In order to find
these I changed TYPE_INSTANCE_FLAGS to make use of a member function
returning a 'const type_instance_flags'.

Feel free to take any parts of this patch you think are useful.

Thanks,
Andrew

---

commit 77f72bf3ec4f67354bb42b99b5115001f08bd492
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed Aug 26 16:19:22 2020 +0100

    gdb: use SET_TYPE_INSTANCE_FLAGS more
    
    A few places where SET_TYPE_INSTANCE_FLAGS should be used.
    
    gdb/ChangeLog:
    
            * d-lang.c (build_d_types): Use SET_TYPE_INSTANCE_FLAGS.
            * ft32-tdep.c (ft32_gdbarch_init): Likewise.
            * gdbtypes.c (gdbtypes_post_init): Likewise.
            * gdbtypes.h (struct type) <get_instance_flags>: New member
            function.
            <set_instance_flags>: Likewise.
            (TYPE_INSTANCE_FLAGS): Update.
            (SET_TYPE_INSTANCE_FLAGS): Update.

diff --git a/gdb/d-lang.c b/gdb/d-lang.c
index 4ebb011ee9b..6b1f68b827e 100644
--- a/gdb/d-lang.c
+++ b/gdb/d-lang.c
@@ -321,10 +321,12 @@ build_d_types (struct gdbarch *gdbarch)
     = arch_float_type (gdbarch, gdbarch_long_double_bit (gdbarch),
 		       "real", gdbarch_long_double_format (gdbarch));
 
-  TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_byte)
-    |= TYPE_INSTANCE_FLAG_NOTTEXT;
-  TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_ubyte)
-    |= TYPE_INSTANCE_FLAG_NOTTEXT;
+  SET_TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_byte,
+			   (TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_byte)
+			    | TYPE_INSTANCE_FLAG_NOTTEXT));
+  SET_TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_ubyte,
+			   (TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_ubyte)
+			    | TYPE_INSTANCE_FLAG_NOTTEXT));
 
   /* Imaginary and complex types.  */
   builtin_d_type->builtin_ifloat
diff --git a/gdb/ft32-tdep.c b/gdb/ft32-tdep.c
index 8ce16c06505..c3a83149bec 100644
--- a/gdb/ft32-tdep.c
+++ b/gdb/ft32-tdep.c
@@ -577,7 +577,9 @@ ft32_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   func_void_type = make_function_type (void_type, NULL);
   tdep->pc_type = arch_pointer_type (gdbarch, 4 * TARGET_CHAR_BIT, NULL,
 				     func_void_type);
-  TYPE_INSTANCE_FLAGS (tdep->pc_type) |= TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1;
+  SET_TYPE_INSTANCE_FLAGS (tdep->pc_type,
+			   (TYPE_INSTANCE_FLAGS (tdep->pc_type)
+			    | TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1));
 
   set_gdbarch_num_regs (gdbarch, FT32_NUM_REGS);
   set_gdbarch_sp_regnum (gdbarch, FT32_SP_REGNUM);
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 64e44bfe23d..394abaae106 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5823,10 +5823,12 @@ gdbtypes_post_init (struct gdbarch *gdbarch)
     = arch_integer_type (gdbarch, 128, 0, "int128_t");
   builtin_type->builtin_uint128
     = arch_integer_type (gdbarch, 128, 1, "uint128_t");
-  TYPE_INSTANCE_FLAGS (builtin_type->builtin_int8) |=
-    TYPE_INSTANCE_FLAG_NOTTEXT;
-  TYPE_INSTANCE_FLAGS (builtin_type->builtin_uint8) |=
-    TYPE_INSTANCE_FLAG_NOTTEXT;
+  SET_TYPE_INSTANCE_FLAGS (builtin_type->builtin_int8,
+			   (TYPE_INSTANCE_FLAGS (builtin_type->builtin_int8)
+			    | TYPE_INSTANCE_FLAG_NOTTEXT));
+  SET_TYPE_INSTANCE_FLAGS (builtin_type->builtin_uint8,
+			   (TYPE_INSTANCE_FLAGS (builtin_type->builtin_uint8)
+			    | TYPE_INSTANCE_FLAG_NOTTEXT));
 
   /* Wide character types.  */
   builtin_type->builtin_char16
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index b42cef61371..9c0289b18b2 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1037,6 +1037,18 @@ struct type
     this->field (0).set_type (index_type);
   }
 
+  /* Return the instance flags converted to the correct type.  */
+  const type_instance_flags get_instance_flags () const
+  {
+    return type_instance_flags ((enum type_instance_flag_value) this->instance_flags);
+  }
+
+  /* Set the instance flags.  */
+  void set_instance_flags (type_instance_flags flags)
+  {
+    this->instance_flags = flags;
+  }
+
   /* Get the bounds bounds of this type.  The type must be a range type.  */
   range_bounds *bounds () const
   {
@@ -1585,10 +1597,9 @@ extern void allocate_gnat_aux_type (struct type *);
      TYPE_ZALLOC (type,							       \
 		  sizeof (*TYPE_MAIN_TYPE (type)->type_specific.func_stuff)))
 
-#define TYPE_INSTANCE_FLAGS(thistype) \
-  type_instance_flags ((enum type_instance_flag_value) (thistype)->instance_flags)
-#define SET_TYPE_INSTANCE_FLAGS(thistype, flags) \
-  (thistype)->instance_flags = flags
+#define TYPE_INSTANCE_FLAGS(thistype) ((thistype)->get_instance_flags ())
+#define SET_TYPE_INSTANCE_FLAGS(thistype, flags)	\
+  ((thistype)->set_instance_flags (flags))
 #define TYPE_MAIN_TYPE(thistype) (thistype)->main_type
 #define TYPE_TARGET_TYPE(thistype) TYPE_MAIN_TYPE(thistype)->target_type
 #define TYPE_POINTER_TYPE(thistype) (thistype)->pointer_type

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-26 15:21   ` Andrew Burgess
@ 2020-09-14 13:53     ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2020-09-14 13:53 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 8/26/20 4:21 PM, Andrew Burgess wrote:

>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
>> index 55a6dafb7e..b42cef6137 100644
>> --- a/gdb/gdbtypes.h
>> +++ b/gdb/gdbtypes.h
>> @@ -1585,7 +1585,10 @@ extern void allocate_gnat_aux_type (struct type *);
>>       TYPE_ZALLOC (type,							       \
>>  		  sizeof (*TYPE_MAIN_TYPE (type)->type_specific.func_stuff)))
>>  
>> -#define TYPE_INSTANCE_FLAGS(thistype) (thistype)->instance_flags
>> +#define TYPE_INSTANCE_FLAGS(thistype) \
>> +  type_instance_flags ((enum type_instance_flag_value) (thistype)->instance_flags)
>> +#define SET_TYPE_INSTANCE_FLAGS(thistype, flags) \
>> +  (thistype)->instance_flags = flags
> 
> There are some places where you've not updated a use of
> TYPE_INSTANCE_FLAGS.  The patch below fixes these.  In order to find
> these I changed TYPE_INSTANCE_FLAGS to make use of a member function
> returning a 'const type_instance_flags'.
> 
> Feel free to take any parts of this patch you think are useful.

Wow, these shouldn't even compile.  That is showing a hole in enum_flags.

>  
> -  TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_byte)
> -    |= TYPE_INSTANCE_FLAG_NOTTEXT;

TYPE_INSTANCE_FLAGS does a cast, so this is the equivalent of:

  int flags = 0;
  (int) flags |= 1;

which of course doesn't compile:

 test.c:1:18: error: lvalue required as left operand of assignment
   1 |   (int) flags |= 1;
     |                  ^

I'm merging the patch below into the main enum flags patch, to
disable the rvalue versions of compound assignment.  It catches the
spots you caught as well:

 src/gdb/d-lang.c: In function ‘void* build_d_types(gdbarch*)’:
 src/gdb/d-lang.c:325:8: error: use of deleted function ‘void enum_flags<E>::operator|=(enum_flags<E>) && [with E = type_instance_flag_value]’
   325 |     |= TYPE_INSTANCE_FLAG_NOTTEXT;
       |        ^~~~~~~~~~~~~~~~~~~~~~~~~~

 src/gdb/ft32-tdep.c: In function ‘gdbarch* ft32_gdbarch_init(gdbarch_info, gdbarch_list*)’:
 src/gdb/ft32-tdep.c:580:42: error: use of deleted function ‘void enum_flags<E>::operator|=(enum_flags<E>) && [with E = type_instance_flag_value]’
   580 |   TYPE_INSTANCE_FLAGS (tdep->pc_type) |= TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1;
       |                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'll merge your changes into the type_instance_flags patch.

From aadb29540a43d96dc234d0d8126e5081926eec02 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 14 Sep 2020 14:48:34 +0100
Subject: [PATCH] delete rval

---
 gdb/unittests/enum-flags-selftests.c | 18 +++++++++---------
 gdbsupport/enum-flags.h              | 18 +++++++++++-------
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/gdb/unittests/enum-flags-selftests.c b/gdb/unittests/enum-flags-selftests.c
index 17ab5c9b09..af585f04ae 100644
--- a/gdb/unittests/enum-flags-selftests.c
+++ b/gdb/unittests/enum-flags-selftests.c
@@ -174,9 +174,9 @@ CHECK_VALID (true,  EF,   EF () ^ RE ())
 CHECK_VALID (false, void, RE () |= RE2 ())
 CHECK_VALID (false, void, RE () &= RE2 ())
 CHECK_VALID (false, void, RE () ^= RE2 ())
-CHECK_VALID (true,  RE&,  RE () |= RE ())
-CHECK_VALID (true,  RE&,  RE () &= RE ())
-CHECK_VALID (true,  RE&,  RE () ^= RE ())
+CHECK_VALID (false, void, RE () |= RE ())
+CHECK_VALID (false, void, RE () &= RE ())
+CHECK_VALID (false, void, RE () ^= RE ())
 
 /* operator OP= (raw_enum, raw_enum), lvalue ref on the lhs. */
 
@@ -192,9 +192,9 @@ CHECK_VALID (true,  RE&,  re ^= RE ())
 CHECK_VALID (false, void, EF () |= RE2 ())
 CHECK_VALID (false, void, EF () &= RE2 ())
 CHECK_VALID (false, void, EF () ^= RE2 ())
-CHECK_VALID (true,  EF&,  EF () |= RE ())
-CHECK_VALID (true,  EF&,  EF () &= RE ())
-CHECK_VALID (true,  EF&,  EF () ^= RE ())
+CHECK_VALID (false, void, EF () |= RE ())
+CHECK_VALID (false, void, EF () &= RE ())
+CHECK_VALID (false, void, EF () ^= RE ())
 
 /* operator OP= (enum_flags, raw_enum), lvalue ref on the lhs.  */
 
@@ -210,9 +210,9 @@ CHECK_VALID (true,  EF&,  ef ^= EF ())
 CHECK_VALID (false, void, EF () |= EF2 ())
 CHECK_VALID (false, void, EF () &= EF2 ())
 CHECK_VALID (false, void, EF () ^= EF2 ())
-CHECK_VALID (true,  EF&,  EF () |= EF ())
-CHECK_VALID (true,  EF&,  EF () &= EF ())
-CHECK_VALID (true,  EF&,  EF () ^= EF ())
+CHECK_VALID (false, void, EF () |= EF ())
+CHECK_VALID (false, void, EF () &= EF ())
+CHECK_VALID (false, void, EF () ^= EF ())
 
 /* operator OP= (enum_flags, enum_flags), lvalue ref on the lhs.  */
 
diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index b3e317ecb9..d30d90b68d 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -147,22 +147,27 @@ class enum_flags
     : m_enum_value ((enum_type) 0)
   {}
 
-  enum_flags &operator&= (enum_flags e)
+  enum_flags &operator&= (enum_flags e) &
   {
     m_enum_value = (enum_type) (m_enum_value & e.m_enum_value);
     return *this;
   }
-  enum_flags &operator|= (enum_flags e)
+  enum_flags &operator|= (enum_flags e) &
   {
     m_enum_value = (enum_type) (m_enum_value | e.m_enum_value);
     return *this;
   }
-  enum_flags &operator^= (enum_flags e)
+  enum_flags &operator^= (enum_flags e) &
   {
     m_enum_value = (enum_type) (m_enum_value ^ e.m_enum_value);
     return *this;
   }
 
+  /* Delete rval versions.  */
+  void operator&= (enum_flags e) && = delete;
+  void operator|= (enum_flags e) && = delete;
+  void operator^= (enum_flags e) && = delete;
+
   /* Like raw enums, allow conversion to the underlying type.  */
   constexpr operator underlying_type () const
   {
@@ -278,9 +283,8 @@ using is_enum_flags_enum_type_t
   /* rval reference version.  */					\
   template <typename enum_type,						\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
-  constexpr enum_type &							\
-  OPERATOR_OP (enum_type &&e1, enum_type e2)				\
-  { return e1 = e1 OP e2; }						\
+  void									\
+  OPERATOR_OP (enum_type &&e1, enum_type e2) = delete;			\
 									\
   /* Delete compound assignment from unrelated types.  */		\
 									\
@@ -291,7 +295,7 @@ using is_enum_flags_enum_type_t
 									\
   template <typename enum_type, typename other_enum_type,		\
 	    typename = is_enum_flags_enum_type_t<enum_type>>		\
-  constexpr enum_type &							\
+  void									\
   OPERATOR_OP (enum_type &&e1, other_enum_type e2) = delete;
 
 ENUM_FLAGS_GEN_BINOP (operator|, |)

-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-08-21 14:45 ` [PATCH 2/3] Use type_instance_flags more throughout Pedro Alves
                     ` (2 preceding siblings ...)
  2020-08-26 15:21   ` Andrew Burgess
@ 2020-09-11 20:21   ` Tom Tromey
  2020-09-14 19:26     ` Pedro Alves
  3 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2020-09-11 20:21 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> -int
Pedro> +type_instance_flags
Pedro>  address_space_name_to_int (struct gdbarch *gdbarch,

Maybe this is misnamed after the change.

Pedro>  const char *
Pedro> -address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
Pedro> +address_space_int_to_name (struct gdbarch *gdbarch,

Likewise.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] Use type_instance_flags more throughout
  2020-09-11 20:21   ` Tom Tromey
@ 2020-09-14 19:26     ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2020-09-14 19:26 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 9/11/20 9:21 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> -int
> Pedro> +type_instance_flags
> Pedro>  address_space_name_to_int (struct gdbarch *gdbarch,
> 
> Maybe this is misnamed after the change.
> 
> Pedro>  const char *
> Pedro> -address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
> Pedro> +address_space_int_to_name (struct gdbarch *gdbarch,
> 
> Likewise.

Yeah, I wasn't sure it was worth it.

I'm adding this patch to the series to address this.

From 924d1538efdfbc2241cbab0c2ad91abbb0921fde Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 14 Sep 2020 19:53:18 +0100
Subject: [PATCH] Rename address_space_int_to_name/address_space_name_to_int

These methods now take/return a type_instance_flags instead of a raw
integer, so rename them accordingly.

gdb/ChangeLog:

	* c-typeprint.c (c_type_print_modifier): Adjust to rename.
	* gdbtypes.c (address_space_name_to_int): Rename to ...
	(address_space_name_to_type_instance_flags): ... this.
	(address_space_int_to_name): Rename to ...
	(address_space_type_instance_flags_to_name): ... this.
	* gdbtypes.h (address_space_name_to_int): Rename to ...
	(address_space_name_to_type_instance_flags): ... this.
	(address_space_int_to_name): Rename to ...
	(address_space_type_instance_flags_to_name): ... this.
	* type-stack.c (type_stack::insert): Adjust to rename.
	* type-stack.h (type_stack::insert): Likewise.
---
 gdb/c-typeprint.c |  5 +++--
 gdb/gdbtypes.c    | 16 ++++++++--------
 gdb/gdbtypes.h    |  8 ++++----
 gdb/type-stack.c  |  5 +++--
 gdb/type-stack.h  | 10 +++++-----
 5 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index b642c88178..a07b29a95d 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -526,8 +526,9 @@ c_type_print_modifier (struct type *type, struct ui_file *stream,
       did_print_modifier = 1;
     }
 
-  address_space_id = address_space_int_to_name (get_type_arch (type),
-						TYPE_INSTANCE_FLAGS (type));
+  address_space_id
+    = address_space_type_instance_flags_to_name (get_type_arch (type),
+						 TYPE_INSTANCE_FLAGS (type));
   if (address_space_id)
     {
       if (did_print_modifier || need_pre_space)
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b57353dec3..7ade2ccb53 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -575,12 +575,12 @@ lookup_function_type_with_arguments (struct type *type,
   return fn;
 }
 
-/* Identify address space identifier by name --
-   return the integer flag defined in gdbtypes.h.  */
+/* Identify address space identifier by name -- return a
+   type_instance_flags.  */
 
 type_instance_flags
-address_space_name_to_int (struct gdbarch *gdbarch,
-			   const char *space_identifier)
+address_space_name_to_type_instance_flags (struct gdbarch *gdbarch,
+					   const char *space_identifier)
 {
   type_instance_flags type_flags;
 
@@ -598,12 +598,12 @@ address_space_name_to_int (struct gdbarch *gdbarch,
     error (_("Unknown address space specifier: \"%s\""), space_identifier);
 }
 
-/* Identify address space identifier by integer flag as defined in 
-   gdbtypes.h -- return the string version of the adress space name.  */
+/* Identify address space identifier by type_instance_flags and return
+   the string version of the adress space name.  */
 
 const char *
-address_space_int_to_name (struct gdbarch *gdbarch,
-			   type_instance_flags space_flag)
+address_space_type_instance_flags_to_name (struct gdbarch *gdbarch,
+					   type_instance_flags space_flag)
 {
   if (space_flag & TYPE_INSTANCE_FLAG_CODE_SPACE)
     return "code";
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index cdd136ef52..6b87de307b 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2217,11 +2217,11 @@ extern struct type *make_atomic_type (struct type *);
 
 extern void replace_type (struct type *, struct type *);
 
-extern type_instance_flags address_space_name_to_int (struct gdbarch *,
-						      const char *);
+extern type_instance_flags address_space_name_to_type_instance_flags
+  (struct gdbarch *, const char *);
 
-extern const char *address_space_int_to_name (struct gdbarch *,
-					      type_instance_flags);
+extern const char *address_space_type_instance_flags_to_name
+  (struct gdbarch *, type_instance_flags);
 
 extern struct type *make_type_with_address_space
   (struct type *type, type_instance_flags space_identifier);
diff --git a/gdb/type-stack.c b/gdb/type-stack.c
index 608142c849..94ff9ba8c9 100644
--- a/gdb/type-stack.c
+++ b/gdb/type-stack.c
@@ -67,8 +67,9 @@ type_stack::insert (struct expr_builder *pstate, const char *string)
 
   element.piece = tp_space_identifier;
   insert_into (slot, element);
-  element.int_val = address_space_name_to_int (pstate->gdbarch (),
-					       string);
+  element.int_val
+    = address_space_name_to_type_instance_flags (pstate->gdbarch (),
+						 string);
   insert_into (slot, element);
 }
 
diff --git a/gdb/type-stack.h b/gdb/type-stack.h
index 8060f2fea7..265178fc37 100644
--- a/gdb/type-stack.h
+++ b/gdb/type-stack.h
@@ -157,11 +157,11 @@ struct type_stack
 
   /* Insert a tp_space_identifier and the corresponding address space
      value into the stack.  STRING is the name of an address space, as
-     recognized by address_space_name_to_int.  If the stack is empty,
-     the new elements are simply pushed.  If the stack is not empty,
-     this function assumes that the first item on the stack is a
-     tp_pointer, and the new values are inserted above the first
-     item.  */
+     recognized by address_space_name_to_type_instance_flags.  If the
+     stack is empty, the new elements are simply pushed.  If the stack
+     is not empty, this function assumes that the first item on the
+     stack is a tp_pointer, and the new values are inserted above the
+     first item.  */
 
   void insert (struct expr_builder *pstate, const char *string);
 

-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 3/3] Rewrite enum_flags, add unit tests, fix problems
  2020-08-21 14:45 [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
  2020-08-21 14:45 ` [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502) Pedro Alves
  2020-08-21 14:45 ` [PATCH 2/3] Use type_instance_flags more throughout Pedro Alves
@ 2020-08-21 14:45 ` Pedro Alves
  2020-08-21 15:51 ` [PATCH 0/3] " Andrew Burgess
  3 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2020-08-21 14:45 UTC (permalink / raw)
  To: gdb-patches

This patch started by adding comprehensive unit tests for enum_flags.

For the testing part, it adds:

 - tests of normal expected uses of the API.

 - checks that _invalid_ uses of the API would fail to compile.  I.e.,
   it validates that enum_flags really is a strong type, and that
   incorrect mixing of enum types would be caught at compile time.  It
   pulls that off making use of SFINEA and C++11's decltype/constexpr.

This revealed many holes in the enum_flags API.  For example, the f1
assignment below currently incorrectly fails to compile:

 enum_flags<flags> f1 = FLAG1;
 enum_flags<flags> f2 = FLAG2 | f1;

The unit tests also revealed that this useful use case doesn't work:

    enum flag { FLAG1 = 1, FLAG2 = 2 };
    enum_flags<flag> src = FLAG1;
    enum_flags<flag> f1 = condition ? src : FLAG2;

It fails to compile because enum_flags<flag> and flag are convertible
to each other.

Turns out that making enum_flags be implicitly convertible to the
backing raw enum type was not a good idea.

If we make it convertible to the underlying type instead, we fix that
ternary operator use case, and, we find cases throughout the codebase
that should be using the enum_flags but were using the raw backing
enum instead.  So it's a good change overall.

Also, several operators were missing.

These holes and more are plugged by this patch, by reworking how the
enum_flags operators are implemented, and making use of C++11's
feature of being able to delete methods/functions.

There are cases in gdb/compile/ where we need to call a function in a
C plugin API that expects the raw enum.  To address cases like that,
this adds a "raw()" method to enum_flags.  This way we can keep using
the safer enum_flags to construct the value, and then be explicit when
we need to get at the raw enum.

This makes most of the enum_flags operators constexpr.  Beyond
enabling more compiler optimizations and enabling the new unit tests,
this has other advantages, like making it possible to use operator|
with enum_flags values in switch cases, where only compile-time
constants are allowed:

    enum_flags<flags> f = FLAG1 | FLAG2;
    switch (f)
      {
      case FLAG1 | FLAG2:
	break;
      }

Currently that fails to compile.

It also switches to a different mechanism of enabling the global
operators.  The current mechanism isn't namespace friendly, the new
one is.

It also switches to C++11-style SFINAE -- instead of wrapping the
return type in a SFINAE-friently structure, we use an unnamed template
parameter.  I.e., this:

  template <typename enum_type,
	    typename = is_enum_flags_enum_type_t<enum_type>>
  enum_type
  operator& (enum_type e1, enum_type e2)

instead of:

  template <typename enum_type>
  typename enum_flags_type<enum_type>::type
  operator& (enum_type e1, enum_type e2)

Note that the static_assert inside operator~() was converted to a
couple overloads (signed vs unsigned), because static_assert is too
late for SFINAE-based tests, which is important for the CHECK_VALID
unit tests.

Tested with gcc {4.8, 7.1, 9.3} and clang {5.0.2, 10.0.0}.

gdb/ChangeLog:

	* Makefile.in (SELFTESTS_SRCS): Add
	unittests/enum-flags-selftests.c.
	* btrace.c (ftrace_update_caller, ftrace_fixup_calle): Use
	btrace_function_flags instead of enum btrace_function_flag.
	* compile/compile-c-types.c (convert_qualified): Use
	enum_flags::raw.
	* compile/compile-cplus-symbols.c (convert_one_symbol)
	(convert_symbol_bmsym):
	* compile/compile-cplus-types.c (compile_cplus_convert_method)
	(compile_cplus_convert_struct_or_union_methods)
	(compile_cplus_instance::convert_qualified_base):
	* go-exp.y (parse_string_or_char): Add cast to int.
	* unittests/enum-flags-selftests.c: New file.
	* record-btrace.c (btrace_thread_flag_to_str): Change parameter's
	type to btrace_thread_flags from btrace_thread_flag.
	(record_btrace_cancel_resume, record_btrace_step_thread): Change
	local's type to btrace_thread_flags from btrace_thread_flag.  Add
	cast in DEBUG call.

gdbsupport/ChangeLog:

	* common/enum-flags.h: Include "traits.h".
	(DEF_ENUM_FLAGS_TYPE): Declare a function instead of defining a
	structure.
	(enum_underlying_type): Update comment.
	(namespace enum_flags_detail): New.  Move struct zero_type here.
	(EnumIsUnsigned, EnumIsSigned): New.
	(class enum_flags): Make most methods constexpr.
	(operator&=, operator|=, operator^=): Take an enum_flags instead
	of an enum_type.
	(operator enum_type()): Delete.
	(operator&, operator|, operator^, operator~): Delete, moved out of
	class.
	(raw()): New method.
	(is_enum_flags_enum_type_t): Declare.
	(ENUM_FLAGS_GEN_BINOP, ENUM_FLAGS_GEN_COMPOUND_ASSIGN)
	(ENUM_FLAGS_GEN_COMP): New.  Use them to reimplement global
	operators.
	(operator~): Now constexpr and reimplemented.
	(operator<<, operator>>): New deleted functions.
	* valid-expr.h (CHECK_VALID_EXPR_5, CHECK_VALID_EXPR_6): New.
---
 gdb/Makefile.in                      |   1 +
 gdb/btrace.c                         |   4 +-
 gdb/compile/compile-c-types.c        |   3 +-
 gdb/compile/compile-cplus-symbols.c  |   4 +-
 gdb/compile/compile-cplus-types.c    |  10 +-
 gdb/go-exp.y                         |   2 +-
 gdb/record-btrace.c                  |  10 +-
 gdb/unittests/enum-flags-selftests.c | 586 +++++++++++++++++++++++++++++++++++
 gdbsupport/enum-flags.h              | 366 +++++++++++++++++-----
 gdbsupport/valid-expr.h              |  15 +
 10 files changed, 903 insertions(+), 98 deletions(-)
 create mode 100644 gdb/unittests/enum-flags-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 66abac4b15..a683d232da 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -435,6 +435,7 @@ SELFTESTS_SRCS = \
 	unittests/command-def-selftests.c \
 	unittests/common-utils-selftests.c \
 	unittests/copy_bitwise-selftests.c \
+	unittests/enum-flags-selftests.c \
 	unittests/environ-selftests.c \
 	unittests/filtered_iterator-selftests.c \
 	unittests/format_pieces-selftests.c \
diff --git a/gdb/btrace.c b/gdb/btrace.c
index 2a0c61de76..9022aedd1c 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -265,7 +265,7 @@ ftrace_new_function (struct btrace_thread_info *btinfo,
 static void
 ftrace_update_caller (struct btrace_function *bfun,
 		      struct btrace_function *caller,
-		      enum btrace_function_flag flags)
+		      btrace_function_flags flags)
 {
   if (bfun->up != 0)
     ftrace_debug (bfun, "updating caller");
@@ -283,7 +283,7 @@ static void
 ftrace_fixup_caller (struct btrace_thread_info *btinfo,
 		     struct btrace_function *bfun,
 		     struct btrace_function *caller,
-		     enum btrace_function_flag flags)
+		     btrace_function_flags flags)
 {
   unsigned int prev, next;
 
diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
index 2b25783bb0..0234db59ea 100644
--- a/gdb/compile/compile-c-types.c
+++ b/gdb/compile/compile-c-types.c
@@ -254,7 +254,8 @@ convert_qualified (compile_c_instance *context, struct type *type)
   if (TYPE_RESTRICT (type))
     quals |= GCC_QUALIFIER_RESTRICT;
 
-  return context->plugin ().build_qualified_type (unqual_converted, quals);
+  return context->plugin ().build_qualified_type (unqual_converted,
+						  quals.raw ());
 }
 
 /* Convert a complex type to its gcc representation.  */
diff --git a/gdb/compile/compile-cplus-symbols.c b/gdb/compile/compile-cplus-symbols.c
index 11a2d32345..9840485039 100644
--- a/gdb/compile/compile-cplus-symbols.c
+++ b/gdb/compile/compile-cplus-symbols.c
@@ -208,7 +208,7 @@ convert_one_symbol (compile_cplus_instance *instance,
 
 	  /* Define the decl.  */
 	  instance->plugin ().build_decl
-	    ("variable", name.c_str (), kind, sym_type,
+	    ("variable", name.c_str (), kind.raw (), sym_type,
 	     symbol_name.get (), addr, filename, line);
 
 	  /* Pop scope for non-local symbols.  */
@@ -323,7 +323,7 @@ convert_symbol_bmsym (compile_cplus_instance *instance,
   sym_type = instance->convert_type (type);
   instance->plugin ().push_namespace ("");
   instance->plugin ().build_decl
-    ("minsym", msym->natural_name (), kind, sym_type, nullptr, addr,
+    ("minsym", msym->natural_name (), kind.raw (), sym_type, nullptr, addr,
      nullptr, 0);
   instance->plugin ().pop_binding_level ("");
 }
diff --git a/gdb/compile/compile-cplus-types.c b/gdb/compile/compile-cplus-types.c
index 02df7ab90e..022cc88979 100644
--- a/gdb/compile/compile-cplus-types.c
+++ b/gdb/compile/compile-cplus-types.c
@@ -668,7 +668,7 @@ compile_cplus_convert_method (compile_cplus_instance *instance,
      type and corresponding qualifier flags.  */
   gcc_type func_type = compile_cplus_convert_func (instance, method_type, true);
   gcc_type class_type = instance->convert_type (parent_type);
-  gcc_cp_qualifiers_flags quals = (enum gcc_cp_qualifiers) 0;
+  gcc_cp_qualifiers_flags quals = 0;
 
   if (TYPE_CONST (method_type))
     quals |= GCC_CP_QUALIFIER_CONST;
@@ -681,7 +681,7 @@ compile_cplus_convert_method (compile_cplus_instance *instance,
   gcc_cp_ref_qualifiers_flags rquals = GCC_CP_REF_QUAL_NONE;
 
   return instance->plugin ().build_method_type
-    (class_type, func_type, quals, rquals);
+    (class_type, func_type, quals.raw (), rquals.raw ());
 }
 
 /* Convert a member or method pointer represented by TYPE.  */
@@ -745,7 +745,7 @@ compile_cplus_convert_struct_or_union_methods (compile_cplus_instance *instance,
 		     (sym_kind
 		      | get_method_access_flag (type, i, j)
 		      | GCC_CP_FLAG_VIRTUAL_FUNCTION
-		      | GCC_CP_FLAG_PURE_VIRTUAL_FUNCTION),
+		      | GCC_CP_FLAG_PURE_VIRTUAL_FUNCTION).raw (),
 		     method_type, nullptr, 0, nullptr, 0);
 		  continue;
 		}
@@ -787,7 +787,7 @@ compile_cplus_convert_struct_or_union_methods (compile_cplus_instance *instance,
 
 	  instance->plugin ().build_decl
 	    (kind, overloaded_name.get (),
-	     sym_kind | get_method_access_flag (type, i, j),
+	     (sym_kind | get_method_access_flag (type, i, j)).raw (),
 	     method_type, nullptr, address, filename, line);
 	}
     }
@@ -1060,7 +1060,7 @@ compile_cplus_instance::convert_qualified_base (gcc_type base,
   gcc_type result = base;
 
   if (quals != 0)
-    result = plugin ().build_qualified_type (base, quals);
+    result = plugin ().build_qualified_type (base, quals.raw ());
 
   return result;
 }
diff --git a/gdb/go-exp.y b/gdb/go-exp.y
index 17c76ac02a..ee1db2b587 100644
--- a/gdb/go-exp.y
+++ b/gdb/go-exp.y
@@ -924,7 +924,7 @@ parse_string_or_char (const char *tokptr, const char **outptr,
     }
   ++tokptr;
 
-  value->type = C_STRING | (quote == '\'' ? C_CHAR : 0); /*FIXME*/
+  value->type = (int) C_STRING | (quote == '\'' ? C_CHAR : 0); /*FIXME*/
   value->ptr = (char *) obstack_base (&tempbuf);
   value->length = obstack_object_size (&tempbuf);
 
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index a1a3efc3d6..fd0d13fb25 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1928,7 +1928,7 @@ record_btrace_target::get_tailcall_unwinder ()
 /* Return a human-readable string for FLAG.  */
 
 static const char *
-btrace_thread_flag_to_str (enum btrace_thread_flag flag)
+btrace_thread_flag_to_str (btrace_thread_flags flag)
 {
   switch (flag)
     {
@@ -2221,7 +2221,7 @@ record_btrace_target::commit_resume ()
 static void
 record_btrace_cancel_resume (struct thread_info *tp)
 {
-  enum btrace_thread_flag flags;
+  btrace_thread_flags flags;
 
   flags = tp->btrace.flags & (BTHR_MOVE | BTHR_STOP);
   if (flags == 0)
@@ -2229,7 +2229,7 @@ record_btrace_cancel_resume (struct thread_info *tp)
 
   DEBUG ("cancel resume thread %s (%s): %x (%s)",
 	 print_thread_id (tp),
-	 target_pid_to_str (tp->ptid).c_str (), flags,
+	 target_pid_to_str (tp->ptid).c_str (), flags.raw (),
 	 btrace_thread_flag_to_str (flags));
 
   tp->btrace.flags &= ~(BTHR_MOVE | BTHR_STOP);
@@ -2449,7 +2449,7 @@ record_btrace_step_thread (struct thread_info *tp)
 {
   struct btrace_thread_info *btinfo;
   struct target_waitstatus status;
-  enum btrace_thread_flag flags;
+  btrace_thread_flags flags;
 
   btinfo = &tp->btrace;
 
@@ -2457,7 +2457,7 @@ record_btrace_step_thread (struct thread_info *tp)
   btinfo->flags &= ~(BTHR_MOVE | BTHR_STOP);
 
   DEBUG ("stepping thread %s (%s): %x (%s)", print_thread_id (tp),
-	 target_pid_to_str (tp->ptid).c_str (), flags,
+	 target_pid_to_str (tp->ptid).c_str (), flags.raw (),
 	 btrace_thread_flag_to_str (flags));
 
   /* We can't step without an execution history.  */
diff --git a/gdb/unittests/enum-flags-selftests.c b/gdb/unittests/enum-flags-selftests.c
new file mode 100644
index 0000000000..17ab5c9b09
--- /dev/null
+++ b/gdb/unittests/enum-flags-selftests.c
@@ -0,0 +1,586 @@
+/* Self tests for enum-flags for GDB, the GNU debugger.
+
+   Copyright (C) 2016-2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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 <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "gdbsupport/enum-flags.h"
+#include "gdbsupport/valid-expr.h"
+#include "gdbsupport/selftest.h"
+
+namespace selftests {
+namespace enum_flags_tests {
+
+/* The (real) enum types used in CHECK_VALID.  Their names match the
+   template parameter names of the templates defined by CHECK_VALID to
+   make it simpler to use.  They could be named differently.  */
+
+/* A "real enum".  */
+enum RE
+  {
+    RE_FLAG1 = 1 << 1,
+    RE_FLAG2 = 1 << 2,
+  };
+
+/* Another "real enum".  */
+enum RE2
+  {
+    RE2_FLAG1 = 1 << 1,
+    RE2_FLAG2 = 1 << 2,
+  };
+
+/* An unsigned "real enum".  */
+enum URE : unsigned
+  {
+    URE_FLAG1 = 1 << 1,
+    URE_FLAG2 = 1 << 2,
+    URE_FLAG3 = 0xffffffff,
+  };
+
+/* A non-flags enum.  */
+enum NF
+  {
+    NF_FLAG1 = 1 << 1,
+    NF_FLAG2 = 1 << 2,
+  };
+
+/* The corresponding "enum flags" types.  */
+DEF_ENUM_FLAGS_TYPE (RE, EF);
+DEF_ENUM_FLAGS_TYPE (RE2, EF2);
+DEF_ENUM_FLAGS_TYPE (URE, UEF);
+
+#if HAVE_IS_TRIVIALLY_COPYABLE
+
+/* So that std::vectors of types that have enum_flags fields can
+   reallocate efficiently memcpy.  */
+gdb_static_assert (std::is_trivially_copyable<EF>::value);
+
+#endif
+
+/* A couple globals used as lvalues in the CHECK_VALID expressions
+   below.  Their names (and types) match the uppercase type names
+   exposed by CHECK_VALID just to make the expressions easier to
+   follow.  */
+static RE re ATTRIBUTE_UNUSED;
+static EF ef ATTRIBUTE_UNUSED;
+
+/* First, compile-time tests that:
+
+   - make sure that incorrect operations with mismatching enum types
+     are caught at compile time.
+
+   - make sure that the same operations but involving the right enum
+     types do compile and that they return the correct type.
+*/
+
+#define CHECK_VALID(VALID, EXPR_TYPE, EXPR)		\
+  CHECK_VALID_EXPR_6 (EF, RE, EF2, RE2, UEF, URE, VALID, EXPR_TYPE, EXPR)
+
+typedef std::underlying_type<RE>::type und;
+
+/* Test construction / conversion from/to different types.  */
+
+/* RE/EF -> underlying (explicit) */
+CHECK_VALID (true,  und,  und (RE ()))
+CHECK_VALID (true,  und,  und (EF ()))
+
+/* RE/EF -> int (explicit) */
+CHECK_VALID (true,  int,  int (RE ()))
+CHECK_VALID (true,  int,  int (EF ()))
+
+/* other -> RE */
+
+/* You can construct a raw enum value from an int explicitly to punch
+   a hole in the type system if need to.  */
+CHECK_VALID (true,  RE,   RE (1))
+CHECK_VALID (true,  RE,   RE (RE2 ()))
+CHECK_VALID (false, void, RE (EF2 ()))
+CHECK_VALID (true,  RE,   RE (RE ()))
+CHECK_VALID (false, void, RE (EF ()))
+
+/* other -> EF.  */
+
+/* As expected, enum-flags is a stronger type than the backing raw
+   enum.  Unlike with raw enums, you can't construct an enum flags
+   from an integer nor from an unrelated enum type explicitly.  Add an
+   intermediate conversion via the raw enum if you really need it.  */
+CHECK_VALID (false, void, EF (1))
+CHECK_VALID (false, void, EF (1u))
+CHECK_VALID (false, void, EF (RE2 ()))
+CHECK_VALID (false, void, EF (EF2 ()))
+CHECK_VALID (true,  EF,   EF (RE ()))
+CHECK_VALID (true,  EF,   EF (EF ()))
+
+/* Test operators.  */
+
+/* operator OP (raw_enum, int) */
+
+CHECK_VALID (false, void, RE () | 1)
+CHECK_VALID (false, void, RE () & 1)
+CHECK_VALID (false, void, RE () ^ 1)
+
+/* operator OP (int, raw_enum) */
+
+CHECK_VALID (false, void, 1 | RE ())
+CHECK_VALID (false, void, 1 & RE ())
+CHECK_VALID (false, void, 1 ^ RE ())
+
+/* operator OP (enum_flags, int) */
+
+CHECK_VALID (false, void, EF () | 1)
+CHECK_VALID (false, void, EF () & 1)
+CHECK_VALID (false, void, EF () ^ 1)
+
+/* operator OP (int, enum_flags) */
+
+CHECK_VALID (false, void, 1 | EF ())
+CHECK_VALID (false, void, 1 & EF ())
+CHECK_VALID (false, void, 1 ^ EF ())
+
+/* operator OP (raw_enum, raw_enum) */
+
+CHECK_VALID (false, void, RE () | RE2 ())
+CHECK_VALID (false, void, RE () & RE2 ())
+CHECK_VALID (false, void, RE () ^ RE2 ())
+CHECK_VALID (true,  RE,   RE () | RE ())
+CHECK_VALID (true,  RE,   RE () & RE ())
+CHECK_VALID (true,  RE,   RE () ^ RE ())
+
+/* operator OP (enum_flags, raw_enum) */
+
+CHECK_VALID (false, void, EF () | RE2 ())
+CHECK_VALID (false, void, EF () & RE2 ())
+CHECK_VALID (false, void, EF () ^ RE2 ())
+CHECK_VALID (true,  EF,   EF () | RE ())
+CHECK_VALID (true,  EF,   EF () & RE ())
+CHECK_VALID (true,  EF,   EF () ^ RE ())
+
+/* operator OP= (raw_enum, raw_enum), rvalue ref on the lhs. */
+
+CHECK_VALID (false, void, RE () |= RE2 ())
+CHECK_VALID (false, void, RE () &= RE2 ())
+CHECK_VALID (false, void, RE () ^= RE2 ())
+CHECK_VALID (true,  RE&,  RE () |= RE ())
+CHECK_VALID (true,  RE&,  RE () &= RE ())
+CHECK_VALID (true,  RE&,  RE () ^= RE ())
+
+/* operator OP= (raw_enum, raw_enum), lvalue ref on the lhs. */
+
+CHECK_VALID (false, void, re |= RE2 ())
+CHECK_VALID (false, void, re &= RE2 ())
+CHECK_VALID (false, void, re ^= RE2 ())
+CHECK_VALID (true,  RE&,  re |= RE ())
+CHECK_VALID (true,  RE&,  re &= RE ())
+CHECK_VALID (true,  RE&,  re ^= RE ())
+
+/* operator OP= (enum_flags, raw_enum), rvalue ref on the lhs.  */
+
+CHECK_VALID (false, void, EF () |= RE2 ())
+CHECK_VALID (false, void, EF () &= RE2 ())
+CHECK_VALID (false, void, EF () ^= RE2 ())
+CHECK_VALID (true,  EF&,  EF () |= RE ())
+CHECK_VALID (true,  EF&,  EF () &= RE ())
+CHECK_VALID (true,  EF&,  EF () ^= RE ())
+
+/* operator OP= (enum_flags, raw_enum), lvalue ref on the lhs.  */
+
+CHECK_VALID (false, void, ef |= RE2 ())
+CHECK_VALID (false, void, ef &= RE2 ())
+CHECK_VALID (false, void, ef ^= RE2 ())
+CHECK_VALID (true,  EF&,  ef |= EF ())
+CHECK_VALID (true,  EF&,  ef &= EF ())
+CHECK_VALID (true,  EF&,  ef ^= EF ())
+
+/* operator OP= (enum_flags, enum_flags), rvalue ref on the lhs.  */
+
+CHECK_VALID (false, void, EF () |= EF2 ())
+CHECK_VALID (false, void, EF () &= EF2 ())
+CHECK_VALID (false, void, EF () ^= EF2 ())
+CHECK_VALID (true,  EF&,  EF () |= EF ())
+CHECK_VALID (true,  EF&,  EF () &= EF ())
+CHECK_VALID (true,  EF&,  EF () ^= EF ())
+
+/* operator OP= (enum_flags, enum_flags), lvalue ref on the lhs.  */
+
+CHECK_VALID (false, void, ef |= EF2 ())
+CHECK_VALID (false, void, ef &= EF2 ())
+CHECK_VALID (false, void, ef ^= EF2 ())
+CHECK_VALID (true,  EF&,  ef |= EF ())
+CHECK_VALID (true,  EF&,  ef &= EF ())
+CHECK_VALID (true,  EF&,  ef ^= EF ())
+
+/* operator~ (raw_enum) */
+
+CHECK_VALID (false,  void,   ~RE ())
+CHECK_VALID (true,   URE,    ~URE ())
+
+/* operator~ (enum_flags) */
+
+CHECK_VALID (false,  void,   ~EF ())
+CHECK_VALID (true,   UEF,    ~UEF ())
+
+/* Check ternary operator.  This exercises implicit conversions.  */
+
+CHECK_VALID (true,  EF,   true ? EF () : RE ())
+CHECK_VALID (true,  EF,   true ? RE () : EF ())
+
+/* These are valid, but it's not a big deal since you won't be able to
+   assign the resulting integer to an enum or an enum_flags without a
+   cast.
+
+   The latter two tests are disabled on older GCCs because they
+   incorrectly fail with gcc 4.8 and 4.9 at least.  Running the test
+   outside a SFINAE context shows:
+
+    invalid user-defined conversion from ‘EF’ to ‘RE2’
+
+   They've been confirmed to compile/pass with gcc 5.3, gcc 7.1 and
+   clang 3.7.  */
+
+CHECK_VALID (true,  int,  true ? EF () : EF2 ())
+CHECK_VALID (true,  int,  true ? EF2 () : EF ())
+#if GCC_VERSION >= 5003 || defined __clang__
+CHECK_VALID (true,  int,  true ? EF () : RE2 ())
+CHECK_VALID (true,  int,  true ? RE2 () : EF ())
+#endif
+
+/* Same, but with an unsigned enum.  */
+
+typedef unsigned int uns;
+
+CHECK_VALID (true,  uns,  true ? EF () : UEF ())
+CHECK_VALID (true,  uns,  true ? UEF () : EF ())
+#if GCC_VERSION >= 5003 || defined __clang__
+CHECK_VALID (true,  uns,  true ? EF () : URE ())
+CHECK_VALID (true,  uns,  true ? URE () : EF ())
+#endif
+
+/* Unfortunately this can't work due to the way C++ computes the
+   return type of the ternary conditional operator.  int isn't
+   implicitly convertible to the raw enum type, so the type of the
+   expression is int.  And then int is not implicitly convertible to
+   enum_flags.
+
+   GCC 4.8 fails to compile this test with:
+     error: operands to ?: have different types ‘enum_flags<RE>’ and ‘int’
+   Confirmed to work with gcc 4.9, 5.3 and clang 3.7.
+*/
+#if GCC_VERSION >= 4009 || defined __clang__
+CHECK_VALID (false, void, true ? EF () : 0)
+CHECK_VALID (false, void, true ? 0 : EF ())
+#endif
+
+/* Check that the ++/--/<</>>/<<=/>>= operators are deleted.  */
+
+CHECK_VALID (false, void, RE ()++)
+CHECK_VALID (false, void, ++RE ())
+CHECK_VALID (false, void, --RE ())
+CHECK_VALID (false, void, RE ()--)
+
+CHECK_VALID (false, void, RE () << 1)
+CHECK_VALID (false, void, RE () >> 1)
+CHECK_VALID (false, void, EF () << 1)
+CHECK_VALID (false, void, EF () >> 1)
+
+CHECK_VALID (false, void, RE () <<= 1)
+CHECK_VALID (false, void, RE () >>= 1)
+CHECK_VALID (false, void, EF () <<= 1)
+CHECK_VALID (false, void, EF () >>= 1)
+
+/* Test comparison operators.  */
+
+CHECK_VALID (false, void, EF () == EF2 ())
+CHECK_VALID (false, void, EF () == RE2 ())
+CHECK_VALID (false, void, RE () == EF2 ())
+
+CHECK_VALID (true,  bool, EF (RE (1)) == EF (RE (1)))
+CHECK_VALID (true,  bool, EF (RE (1)) == RE (1))
+CHECK_VALID (true,  bool, RE (1)      == EF (RE (1)))
+
+CHECK_VALID (false, void, EF () != EF2 ())
+CHECK_VALID (false, void, EF () != RE2 ())
+CHECK_VALID (false, void, RE () != EF2 ())
+
+/* On clang, disable -Wenum-compare due to "error: comparison of two
+   values with different enumeration types [-Werror,-Wenum-compare]".
+   clang doesn't suppress -Wenum-compare in SFINAE contexts.  Not a
+   big deal since misuses like these in GDB will be caught by -Werror
+   anyway.  This check is here mainly for completeness.  */
+#if defined __clang__
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wenum-compare"
+#endif
+CHECK_VALID (true,  bool, RE () == RE2 ())
+CHECK_VALID (true,  bool, RE () != RE2 ())
+#if defined __clang__
+# pragma GCC diagnostic pop
+#endif
+
+CHECK_VALID (true,  bool, EF (RE (1)) != EF (RE (2)))
+CHECK_VALID (true,  bool, EF (RE (1)) != RE (2))
+CHECK_VALID (true,  bool, RE (1)      != EF (RE (2)))
+
+CHECK_VALID (true,  bool, EF () == 0)
+
+/* Check we didn't disable/delete comparison between non-flags enums
+   and unrelated types by mistake.  */
+CHECK_VALID (true,  bool, NF (1) == NF (1))
+CHECK_VALID (true,  bool, NF (1) == int (1))
+CHECK_VALID (true,  bool, NF (1) == char (1))
+
+/* -------------------------------------------------------------------- */
+
+/* Follows misc tests that exercise the API.  Some are compile time,
+   when possible, others are run time.  */
+
+enum test_flag
+  {
+    FLAG1 = 1 << 1,
+    FLAG2 = 1 << 2,
+    FLAG3 = 1 << 3,
+  };
+
+enum test_uflag : unsigned
+  {
+    UFLAG1 = 1 << 1,
+    UFLAG2 = 1 << 2,
+    UFLAG3 = 1 << 3,
+  };
+
+DEF_ENUM_FLAGS_TYPE (test_flag, test_flags);
+DEF_ENUM_FLAGS_TYPE (test_uflag, test_uflags);
+
+static void
+self_test ()
+{
+  /* Check that default construction works.  */
+  {
+    constexpr test_flags f;
+
+    gdb_static_assert (f == 0);
+  }
+
+  /* Check that assignment from zero works.  */
+  {
+    test_flags f (FLAG1);
+
+    SELF_CHECK (f == FLAG1);
+
+    f = 0;
+
+    SELF_CHECK (f == 0);
+  }
+
+  /* Check that construction from zero works.  */
+  {
+    constexpr test_flags zero1 = 0;
+    constexpr test_flags zero2 (0);
+    constexpr test_flags zero3 {0};
+    constexpr test_flags zero4 = {0};
+
+    gdb_static_assert (zero1 == 0);
+    gdb_static_assert (zero2 == 0);
+    gdb_static_assert (zero3 == 0);
+    gdb_static_assert (zero4 == 0);
+  }
+
+  /* Check construction from enum value.  */
+  {
+    gdb_static_assert (test_flags (FLAG1) == FLAG1);
+    gdb_static_assert (test_flags (FLAG2) != FLAG1);
+  }
+
+  /* Check copy/assignment.  */
+  {
+    constexpr test_flags src = FLAG1;
+
+    constexpr test_flags f1 = src;
+    constexpr test_flags f2 (src);
+    constexpr test_flags f3 {src};
+    constexpr test_flags f4 = {src};
+
+    gdb_static_assert (f1 == FLAG1);
+    gdb_static_assert (f2 == FLAG1);
+    gdb_static_assert (f3 == FLAG1);
+    gdb_static_assert (f4 == FLAG1);
+  }
+
+  /* Check moving.  */
+  {
+    test_flags src = FLAG1;
+    test_flags dst = 0;
+
+    dst = std::move (src);
+    SELF_CHECK (dst == FLAG1);
+  }
+
+  /* Check construction from an 'or' of multiple bits.  For this to
+     work, operator| must be overridden to return an enum type.  The
+     builtin version would return int instead and then the conversion
+     to test_flags would fail.  */
+  {
+    constexpr test_flags f = FLAG1 | FLAG2;
+    gdb_static_assert (f == (FLAG1 | FLAG2));
+  }
+
+  /* Similarly, check that "FLAG1 | FLAG2" on the rhs of an assignment
+     operator works.  */
+  {
+    test_flags f = 0;
+    f |= FLAG1 | FLAG2;
+    SELF_CHECK (f == (FLAG1 | FLAG2));
+
+    f &= FLAG1 | FLAG2;
+    SELF_CHECK (f == (FLAG1 | FLAG2));
+
+    f ^= FLAG1 | FLAG2;
+    SELF_CHECK (f == 0);
+  }
+
+  /* Check explicit conversion to int works.  */
+  {
+    constexpr int some_bits (FLAG1 | FLAG2);
+
+    /* And comparison with int works too.  */
+    gdb_static_assert (some_bits == (FLAG1 | FLAG2));
+    gdb_static_assert (some_bits == test_flags (FLAG1 | FLAG2));
+  }
+
+  /* Check operator| and operator|=.  Particularly interesting is
+     making sure that putting the enum value on the lhs side of the
+     expression works (FLAG | f).  */
+  {
+    test_flags f = FLAG1;
+    f |= FLAG2;
+    SELF_CHECK (f == (FLAG1 | FLAG2));
+  }
+  {
+    test_flags f = FLAG1;
+    f = f | FLAG2;
+    SELF_CHECK (f == (FLAG1 | FLAG2));
+  }
+  {
+    test_flags f = FLAG1;
+    f = FLAG2 | f;
+    SELF_CHECK (f == (FLAG1 | FLAG2));
+  }
+
+  /* Check the &/&= operators.  */
+  {
+    test_flags f = FLAG1 & FLAG2;
+    SELF_CHECK (f == 0);
+
+    f = FLAG1 | FLAG2;
+    f &= FLAG2;
+    SELF_CHECK (f == FLAG2);
+
+    f = FLAG1 | FLAG2;
+    f = f & FLAG2;
+    SELF_CHECK (f == FLAG2);
+
+    f = FLAG1 | FLAG2;
+    f = FLAG2 & f;
+    SELF_CHECK (f == FLAG2);
+  }
+
+  /* Check the ^/^= operators.  */
+  {
+    constexpr test_flags f = FLAG1 ^ FLAG2;
+    gdb_static_assert (f == (FLAG1 ^ FLAG2));
+  }
+
+  {
+    test_flags f = FLAG1 ^ FLAG2;
+    f ^= FLAG3;
+    SELF_CHECK (f == (FLAG1 | FLAG2 | FLAG3));
+    f = f ^ FLAG3;
+    SELF_CHECK (f == (FLAG1 | FLAG2));
+    f = FLAG3 ^ f;
+    SELF_CHECK (f == (FLAG1 | FLAG2 | FLAG3));
+  }
+
+  /* Check operator~.  Note this only compiles with unsigned
+     flags.  */
+  {
+    constexpr test_uflags f1 = ~UFLAG1;
+    constexpr test_uflags f2 = ~f1;
+    gdb_static_assert (f2 == UFLAG1);
+  }
+
+  /* Check the ternary operator.  */
+
+  {
+    /* raw enum, raw enum */
+    constexpr test_flags f1 = true ? FLAG1 : FLAG2;
+    gdb_static_assert (f1 == FLAG1);
+    constexpr test_flags f2 = false ? FLAG1 : FLAG2;
+    gdb_static_assert (f2 == FLAG2);
+  }
+
+  {
+    /* enum flags, raw enum */
+    constexpr test_flags src = FLAG1;
+    constexpr test_flags f1 = true ? src : FLAG2;
+    gdb_static_assert (f1 == FLAG1);
+    constexpr test_flags f2 = false ? src : FLAG2;
+    gdb_static_assert (f2 == FLAG2);
+  }
+
+  {
+    /* enum flags, enum flags */
+    constexpr test_flags src1 = FLAG1;
+    constexpr test_flags src2 = FLAG2;
+    constexpr test_flags f1 = true ? src1 : src2;
+    gdb_static_assert (f1 == src1);
+    constexpr test_flags f2 = false ? src1 : src2;
+    gdb_static_assert (f2 == src2);
+  }
+
+  /* Check that we can use flags in switch expressions (requires
+     unambiguous conversion to integer).  Also check that we can use
+     operator| in switch cases, where only constants are allowed.
+     This should work because operator| is constexpr.  */
+  {
+    test_flags f = FLAG1 | FLAG2;
+    bool ok = false;
+
+    switch (f)
+      {
+      case FLAG1:
+	break;
+      case FLAG2:
+	break;
+      case FLAG1 | FLAG2:
+	ok = true;
+	break;
+      }
+
+    SELF_CHECK (ok);
+  }
+}
+
+} /* namespace enum_flags_tests */
+} /* namespace selftests */
+
+void _initialize_enum_flags_selftests ();
+
+void
+_initialize_enum_flags_selftests ()
+{
+  selftests::register_test ("enum-flags",
+			    selftests::enum_flags_tests::self_test);
+}
diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index 825ff4faf2..b3e317ecb9 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -18,6 +18,8 @@
 #ifndef COMMON_ENUM_FLAGS_H
 #define COMMON_ENUM_FLAGS_H
 
+#include "traits.h"
+
 /* Type-safe wrapper for enum flags.  enum flags are enums where the
    values are bits that are meant to be ORed together.
 
@@ -51,23 +53,31 @@
 
 #ifdef __cplusplus
 
-/* Traits type used to prevent the global operator overloads from
-   instantiating for non-flag enums.  */
-template<typename T> struct enum_flags_type {};
-
-/* Use this to mark an enum as flags enum.  It defines FLAGS as
+/* Use this to mark an enum as flags enum.  It defines FLAGS_TYPE as
    enum_flags wrapper class for ENUM, and enables the global operator
    overloads for ENUM.  */
 #define DEF_ENUM_FLAGS_TYPE(enum_type, flags_type)	\
   typedef enum_flags<enum_type> flags_type;		\
-  template<>						\
-  struct enum_flags_type<enum_type>			\
-  {							\
-    typedef enum_flags<enum_type> type;			\
-  }
+  void is_enum_flags_enum_type (enum_type *)
+
+/* To enable the global enum_flags operators for enum, declare an
+   "is_enum_flags_enum_type" overload that has exactly one parameter,
+   of type a pointer to that enum class.  E.g.,:
+
+     void is_enum_flags_enum_type (enum some_flag *);
+
+   The function does not need to be defined, only declared.
+   DEF_ENUM_FLAGS_TYPE declares this.
+
+   A function declaration is preferred over a traits type, because the
+   former allows calling the DEF_ENUM_FLAGS_TYPE macro inside a
+   namespace to define the corresponding enum flags type in that
+   namespace.  The compiler finds the corresponding
+   is_enum_flags_enum_type function via ADL.  */
 
-/* Until we can rely on std::underlying type being universally
-   available (C++11), roll our own for enums.  */
+/* Note that std::underlying_type<enum_type> is not what we want here,
+   since that returns unsigned int even when the enum decays to signed
+   int.  */
 template<int size, bool sign> class integer_for_size { typedef void type; };
 template<> struct integer_for_size<1, 0> { typedef uint8_t type; };
 template<> struct integer_for_size<2, 0> { typedef uint16_t type; };
@@ -86,128 +96,320 @@ struct enum_underlying_type
     type;
 };
 
-template <typename E>
-class enum_flags
+namespace enum_flags_detail
 {
-public:
-  typedef E enum_type;
-  typedef typename enum_underlying_type<enum_type>::type underlying_type;
 
-private:
-  /* Private type used to support initializing flag types with zero:
+/* Private type used to support initializing flag types with zero:
 
-       foo_flags f = 0;
+   foo_flags f = 0;
 
-     but not other integers:
+   but not other integers:
 
-       foo_flags f = 1;
+   foo_flags f = 1;
 
-     The way this works is that we define an implicit constructor that
-     takes a pointer to this private type.  Since nothing can
-     instantiate an object of this type, the only possible pointer to
-     pass to the constructor is the NULL pointer, or, zero.  */
-  struct zero_type;
+   The way this works is that we define an implicit constructor that
+   takes a pointer to this private type.  Since nothing can
+   instantiate an object of this type, the only possible pointer to
+   pass to the constructor is the NULL pointer, or, zero.  */
+struct zero_type;
 
-  underlying_type
-  underlying_value () const
-  {
-    return m_enum_value;
-  }
+/* gdb::Requires trait helpers.  */
+template <typename enum_type>
+using EnumIsUnsigned
+  = std::is_unsigned<typename enum_underlying_type<enum_type>::type>;
+template <typename enum_type>
+using EnumIsSigned
+  = std::is_signed<typename enum_underlying_type<enum_type>::type>;
+
+}
+
+template <typename E>
+class enum_flags
+{
+public:
+  typedef E enum_type;
+  typedef typename enum_underlying_type<enum_type>::type underlying_type;
 
 public:
   /* Allow default construction.  */
-  enum_flags ()
+  constexpr enum_flags ()
     : m_enum_value ((enum_type) 0)
   {}
 
+  /* The default move/copy ctor/assignment do the right thing.  */
+
   /* If you get an error saying these two overloads are ambiguous,
      then you tried to mix values of different enum types.  */
-  enum_flags (enum_type e)
+  constexpr enum_flags (enum_type e)
     : m_enum_value (e)
   {}
-  enum_flags (struct enum_flags::zero_type *zero)
+  constexpr enum_flags (enum_flags_detail::zero_type *zero)
     : m_enum_value ((enum_type) 0)
   {}
 
-  enum_flags &operator&= (enum_type e)
+  enum_flags &operator&= (enum_flags e)
   {
-    m_enum_value = (enum_type) (underlying_value () & e);
+    m_enum_value = (enum_type) (m_enum_value & e.m_enum_value);
     return *this;
   }
-  enum_flags &operator|= (enum_type e)
+  enum_flags &operator|= (enum_flags e)
   {
-    m_enum_value = (enum_type) (underlying_value () | e);
+    m_enum_value = (enum_type) (m_enum_value | e.m_enum_value);
     return *this;
   }
-  enum_flags &operator^= (enum_type e)
+  enum_flags &operator^= (enum_flags e)
   {
-    m_enum_value = (enum_type) (underlying_value () ^ e);
+    m_enum_value = (enum_type) (m_enum_value ^ e.m_enum_value);
     return *this;
   }
 
-  operator enum_type () const
+  /* Like raw enums, allow conversion to the underlying type.  */
+  constexpr operator underlying_type () const
   {
     return m_enum_value;
   }
 
-  enum_flags operator& (enum_type e) const
-  {
-    return (enum_type) (underlying_value () & e);
-  }
-  enum_flags operator| (enum_type e) const
-  {
-    return (enum_type) (underlying_value () | e);
-  }
-  enum_flags operator^ (enum_type e) const
-  {
-    return (enum_type) (underlying_value () ^ e);
-  }
-  enum_flags operator~ () const
+  /* Get the underlying value as a raw enum.  */
+  constexpr enum_type raw () const
   {
-    // We only the underlying type to be unsigned when actually using
-    // operator~ -- if it were not unsigned, undefined behavior could
-    // result.  However, asserting this in the class itself would
-    // require too many unnecessary changes to otherwise ok enum
-    // types.
-    gdb_static_assert (std::is_unsigned<underlying_type>::value);
-    return (enum_type) ~underlying_value ();
+    return m_enum_value;
   }
 
+  /* Binary operations involving some unrelated type (which would be a
+     bug) are implemented as non-members, and deleted.  */
+
 private:
   /* Stored as enum_type because GDB knows to print the bit flags
      neatly if the enum values look like bit flags.  */
   enum_type m_enum_value;
 };
 
+template <typename E>
+using is_enum_flags_enum_type_t
+  = decltype (is_enum_flags_enum_type (std::declval<E *> ()));
+
 /* Global operator overloads.  */
 
-template <typename enum_type>
-typename enum_flags_type<enum_type>::type
-operator& (enum_type e1, enum_type e2)
+/* Generate binary operators.  */
+
+#define ENUM_FLAGS_GEN_BINOP(OPERATOR_OP, OP)				\
+									\
+  /* Raw enum on both LHS/RHS.  Returns raw enum type.  */		\
+  template <typename enum_type,						\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_type							\
+  OPERATOR_OP (enum_type e1, enum_type e2)				\
+  {									\
+    using underlying = typename enum_flags<enum_type>::underlying_type;	\
+    return (enum_type) (underlying (e1) OP underlying (e2));		\
+  }									\
+									\
+  /* enum_flags on the LHS.  */						\
+  template <typename enum_type,						\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_flags<enum_type>					\
+  OPERATOR_OP (enum_flags<enum_type> e1, enum_type e2)			\
+  { return e1.raw () OP e2; }						\
+									\
+  /* enum_flags on the RHS.  */						\
+  template <typename enum_type,						\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_flags<enum_type>					\
+  OPERATOR_OP (enum_type e1, enum_flags<enum_type> e2)			\
+  { return e1 OP e2.raw (); }						\
+									\
+  /* enum_flags on both LHS/RHS.  */					\
+  template <typename enum_type,						\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_flags<enum_type>					\
+  OPERATOR_OP (enum_flags<enum_type> e1, enum_flags<enum_type> e2)	\
+  { return e1.raw () OP e2.raw (); }					\
+									\
+  /* Delete cases involving unrelated types.  */			\
+									\
+  template <typename enum_type, typename unrelated_type,		\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_flags<enum_type>					\
+  OPERATOR_OP (enum_type e1, unrelated_type e2) = delete;		\
+									\
+  template <typename enum_type, typename unrelated_type,		\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_flags<enum_type>					\
+  OPERATOR_OP (unrelated_type e1, enum_type e2) = delete;		\
+									\
+  template <typename enum_type, typename unrelated_type,		\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_flags<enum_type>					\
+  OPERATOR_OP (enum_flags<enum_type> e1, unrelated_type e2) = delete;	\
+									\
+  template <typename enum_type, typename unrelated_type,		\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_flags<enum_type>					\
+  OPERATOR_OP (unrelated_type e1, enum_flags<enum_type> e2) = delete;
+
+/* Generate non-member compound assignment operators.  Only the raw
+   enum versions are defined here.  The enum_flags versions are
+   defined as member functions, simply because it's less code that
+   way.
+
+   Note we delete operators that would allow e.g.,
+
+     "enum_type | 1" or "enum_type1 | enum_type2"
+
+   because that would allow a mistake like :
+     enum flags1 { F1_FLAGS1 = 1 };
+     enum flags2 { F2_FLAGS2 = 2 };
+     enum flags1 val;
+     switch (val) {
+       case F1_FLAGS1 | F2_FLAGS2:
+     ...
+
+   If you really need to 'or' enumerators of different flag types,
+   cast to integer first.
+*/
+#define ENUM_FLAGS_GEN_COMPOUND_ASSIGN(OPERATOR_OP, OP)			\
+  /* lval reference version.  */					\
+  template <typename enum_type,						\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_type &							\
+  OPERATOR_OP (enum_type &e1, enum_type e2)				\
+  { return e1 = e1 OP e2; }						\
+									\
+  /* rval reference version.  */					\
+  template <typename enum_type,						\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_type &							\
+  OPERATOR_OP (enum_type &&e1, enum_type e2)				\
+  { return e1 = e1 OP e2; }						\
+									\
+  /* Delete compound assignment from unrelated types.  */		\
+									\
+  template <typename enum_type, typename other_enum_type,		\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_type &							\
+  OPERATOR_OP (enum_type &e1, other_enum_type e2) = delete;		\
+									\
+  template <typename enum_type, typename other_enum_type,		\
+	    typename = is_enum_flags_enum_type_t<enum_type>>		\
+  constexpr enum_type &							\
+  OPERATOR_OP (enum_type &&e1, other_enum_type e2) = delete;
+
+ENUM_FLAGS_GEN_BINOP (operator|, |)
+ENUM_FLAGS_GEN_BINOP (operator&, &)
+ENUM_FLAGS_GEN_BINOP (operator^, ^)
+
+ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator|=, |)
+ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator&=, &)
+ENUM_FLAGS_GEN_COMPOUND_ASSIGN (operator^=, ^)
+
+/* Allow comparison with enum_flags, raw enum, and integers, only.
+   The latter case allows "== 0".  As side effect, it allows comparing
+   with integer variables too, but that's not a common mistake to
+   make.  It's important to disable comparison with unrelated types to
+   prevent accidentally comparing with unrelated enum values, which
+   are convertible to integer, and thus coupled with enum_flags
+   convertion to underlying type too, would trigger the built-in 'bool
+   operator==(unsigned, int)' operator.  */
+
+#define ENUM_FLAGS_GEN_COMP(OPERATOR_OP, OP)				\
+									\
+  /* enum_flags OP enum_flags */					\
+									\
+  template <typename enum_type>						\
+  constexpr bool							\
+  OPERATOR_OP (enum_flags<enum_type> lhs, enum_flags<enum_type> rhs)	\
+  { return lhs.raw () OP rhs.raw (); }					\
+									\
+  /* enum_flags OP other */						\
+									\
+  template <typename enum_type>						\
+  constexpr bool							\
+  OPERATOR_OP (enum_flags<enum_type> lhs, enum_type rhs)		\
+  { return lhs.raw () OP rhs; }						\
+									\
+  template <typename enum_type>						\
+  constexpr bool							\
+  OPERATOR_OP (enum_flags<enum_type> lhs, int rhs)			\
+  { return lhs.raw () OP rhs; }						\
+									\
+  template <typename enum_type, typename U>				\
+  constexpr bool							\
+  OPERATOR_OP (enum_flags<enum_type> lhs, U rhs) = delete;		\
+									\
+  /* other OP enum_flags */						\
+									\
+  template <typename enum_type>						\
+  constexpr bool							\
+  OPERATOR_OP (enum_type lhs, enum_flags<enum_type> rhs)		\
+  { return lhs OP rhs.raw (); }						\
+									\
+  template <typename enum_type>						\
+  constexpr bool							\
+  OPERATOR_OP (int lhs, enum_flags<enum_type> rhs)			\
+  { return lhs OP rhs.raw (); }						\
+									\
+  template <typename enum_type, typename U>				\
+  constexpr bool							\
+  OPERATOR_OP (U lhs, enum_flags<enum_type> rhs) = delete;
+
+ENUM_FLAGS_GEN_COMP (operator==, ==)
+ENUM_FLAGS_GEN_COMP (operator!=, !=)
+
+/* Unary operators for the raw flags enum.  */
+
+/* We require underlying type to be unsigned when using operator~ --
+   if it were not unsigned, undefined behavior could result.  However,
+   asserting this in the class itself would require too many
+   unnecessary changes to usages of otherwise OK enum types.  */
+template <typename enum_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>,
+	  typename
+	    = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>>
+constexpr enum_type
+operator~ (enum_type e)
 {
-  return enum_flags<enum_type> (e1) & e2;
+  using underlying = typename enum_flags<enum_type>::underlying_type;
+  return (enum_type) ~underlying (e);
 }
 
-template <typename enum_type>
-typename enum_flags_type<enum_type>::type
-operator| (enum_type e1, enum_type e2)
+template <typename enum_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>,
+	  typename = gdb::Requires<enum_flags_detail::EnumIsSigned<enum_type>>>
+constexpr void operator~ (enum_type e) = delete;
+
+template <typename enum_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>,
+	  typename
+	    = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>>
+constexpr enum_flags<enum_type>
+operator~ (enum_flags<enum_type> e)
 {
-  return enum_flags<enum_type> (e1) | e2;
+  using underlying = typename enum_flags<enum_type>::underlying_type;
+  return (enum_type) ~underlying (e);
 }
 
-template <typename enum_type>
-typename enum_flags_type<enum_type>::type
-operator^ (enum_type e1, enum_type e2)
-{
-  return enum_flags<enum_type> (e1) ^ e2;
-}
+template <typename enum_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>,
+	  typename = gdb::Requires<enum_flags_detail::EnumIsSigned<enum_type>>>
+constexpr void operator~ (enum_flags<enum_type> e) = delete;
 
-template <typename enum_type>
-typename enum_flags_type<enum_type>::type
-operator~ (enum_type e)
-{
-  return ~enum_flags<enum_type> (e);
-}
+/* Delete operator<< and operator>>.  */
+
+template <typename enum_type, typename any_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>>
+void operator<< (const enum_type &, const any_type &) = delete;
+
+template <typename enum_type, typename any_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>>
+void operator<< (const enum_flags<enum_type> &, const any_type &) = delete;
+
+template <typename enum_type, typename any_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>>
+void operator>> (const enum_type &, const any_type &) = delete;
+
+template <typename enum_type, typename any_type,
+	  typename = is_enum_flags_enum_type_t<enum_type>>
+void operator>> (const enum_flags<enum_type> &, const any_type &) = delete;
 
 #else /* __cplusplus */
 
diff --git a/gdbsupport/valid-expr.h b/gdbsupport/valid-expr.h
index a22fa61134..459de17926 100644
--- a/gdbsupport/valid-expr.h
+++ b/gdbsupport/valid-expr.h
@@ -91,4 +91,19 @@
 			ESC_PARENS (T1, T2, T3, T4),			\
 			VALID, EXPR_TYPE, EXPR)
 
+#define CHECK_VALID_EXPR_5(T1, T2, T3, T4, T5, VALID, EXPR_TYPE, EXPR)	\
+  CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2,		\
+				    typename T3, typename T4,		\
+				    typename T5),			\
+			ESC_PARENS (T1, T2, T3, T4, T5),		\
+			VALID, EXPR_TYPE, EXPR)
+
+#define CHECK_VALID_EXPR_6(T1, T2, T3, T4, T5, T6,			\
+			   VALID, EXPR_TYPE, EXPR)			\
+  CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2,		\
+				    typename T3, typename T4,		\
+				    typename T5, typename T6),		\
+			ESC_PARENS (T1, T2, T3, T4, T5, T6),		\
+			VALID, EXPR_TYPE, EXPR)
+
 #endif /* COMMON_VALID_EXPR_H */
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems
  2020-08-21 14:45 [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
                   ` (2 preceding siblings ...)
  2020-08-21 14:45 ` [PATCH 3/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
@ 2020-08-21 15:51 ` Andrew Burgess
  2020-09-11 20:26   ` Tom Tromey
  3 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2020-08-21 15:51 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

* Pedro Alves <pedro@palves.net> [2020-08-21 15:45:20 +0100]:

> This is an update of a version of this series that I posted back in
> 2016:
> 
>  [PATCH 0/3] enum_flags: Fix problems and add comprehensive unit tests
>  https://sourceware.org/legacy-ml/gdb-patches/2016-11/msg00079.html
> 
> But I never followed thorough with it, leaving it collecting dust on
> my github.  Over the following years, I polished it and tweaked it
> further and extended the unit tests, but never managed to re-submit it
> all upstream.
> 
> Since the subject of enum_flags came up again this week, I thought I'd
> try it again.  This is the result.

Thanks for doing this work.

I've only skimmed through, but it looks good.

Thanks,
Andrew


> 
> Pedro Alves (3):
>   Rewrite valid-expr.h's internals in terms of the detection idiom
>     (C++17/N4502)
>   Use type_instance_flags more throughout
>   Rewrite enum_flags, add unit tests, fix problems
> 
>  gdb/Makefile.in                      |   1 +
>  gdb/btrace.c                         |   4 +-
>  gdb/compile/compile-c-types.c        |   3 +-
>  gdb/compile/compile-cplus-symbols.c  |   4 +-
>  gdb/compile/compile-cplus-types.c    |  10 +-
>  gdb/dwarf2/read.c                    |   7 +-
>  gdb/eval.c                           |   2 +-
>  gdb/gdbarch.c                        |   6 +-
>  gdb/gdbarch.h                        |  12 +-
>  gdb/gdbarch.sh                       |   8 +-
>  gdb/gdbtypes.c                       |  58 ++--
>  gdb/gdbtypes.h                       |  15 +-
>  gdb/go-exp.y                         |   2 +-
>  gdb/record-btrace.c                  |  10 +-
>  gdb/stabsread.c                      |   2 +-
>  gdb/type-stack.c                     |   4 +-
>  gdb/unittests/enum-flags-selftests.c | 586 +++++++++++++++++++++++++++++++++++
>  gdbsupport/enum-flags.h              | 366 +++++++++++++++++-----
>  gdbsupport/traits.h                  |  67 ++++
>  gdbsupport/valid-expr.h              |  35 ++-
>  20 files changed, 1035 insertions(+), 167 deletions(-)
>  create mode 100644 gdb/unittests/enum-flags-selftests.c
> 
> 
> base-commit: b70e516e89d95d06252cb04ded3397ec0a0a3212
> -- 
> 2.14.5
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems
  2020-08-21 15:51 ` [PATCH 0/3] " Andrew Burgess
@ 2020-09-11 20:26   ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2020-09-11 20:26 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: Pedro Alves, gdb-patches

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

>> Since the subject of enum_flags came up again this week, I thought I'd
>> try it again.  This is the result.

Andrew> Thanks for doing this work.

Andrew> I've only skimmed through, but it looks good.

I read through it and I agree.  Thanks for doing this.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-09-29 22:50 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 14:45 [PATCH 0/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
2020-08-21 14:45 ` [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502) Pedro Alves
2020-09-28 18:59   ` Tom Tromey
2020-09-28 19:58     ` Luis Machado
2020-09-28 20:00       ` Tom Tromey
2020-09-29 13:04         ` Tom Tromey
2020-09-29 19:24           ` Pedro Alves
2020-09-29 22:50             ` [pushed] Tweak gdbsupport/valid-expr.h for GCC 6, fix build (Re: [PATCH 1/3] Rewrite valid-expr.h's internals in terms of the detection idiom (C++17/N4502)) Pedro Alves
2020-08-21 14:45 ` [PATCH 2/3] Use type_instance_flags more throughout Pedro Alves
2020-08-25 11:36   ` Luis Machado
2020-09-14 11:56     ` Pedro Alves
2020-08-26 10:06   ` Andrew Burgess
2020-09-14 12:54     ` Pedro Alves
2020-08-26 15:21   ` Andrew Burgess
2020-09-14 13:53     ` Pedro Alves
2020-09-11 20:21   ` Tom Tromey
2020-09-14 19:26     ` Pedro Alves
2020-08-21 14:45 ` [PATCH 3/3] Rewrite enum_flags, add unit tests, fix problems Pedro Alves
2020-08-21 15:51 ` [PATCH 0/3] " Andrew Burgess
2020-09-11 20:26   ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).