[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-09-29T22:50:11Z

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