[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 <pedro@palves.net>]

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