diff mbox series

gcc: testsuite: Fix builtin-speculation-overloads[14].C testism

Message ID 20250212162344.211836-1-mmalcomson@nvidia.com
State New
Headers
Series gcc: testsuite: Fix builtin-speculation-overloads[14].C testism |

Checks

Context Check Description
rivoscibot/toolchain-ci-rivos-lint success Lint passed
rivoscibot/toolchain-ci-rivos-apply-patch success Patch applied
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gc_zba_zbb_zbc_zbs-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-test success Testing passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-aarch64-bootstrap success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Matthew Malcomson Feb. 12, 2025, 4:23 p.m. UTC 
  From: Matthew Malcomson <mmalcomson@nvidia.com>

I've posted the patch on the relevant Bugzilla, but also sending to
mailing list.  If should have only done one please do mention.

----------------- 8< ----------- >8 ----------------
When making warnings trigger a failure in template substitution I
could not find any way to trigger the warning about builtin speculation
not being available on the given target.

Turns out I misread the code -- this warning happens when the
speculation_barrier pattern is not defined.

Here we add an effective target to represent
"__builtin_speculation_safe_value is available on this target" and use
that to adjust our test on SFINAE behaviour accordingly.
N.b. this means that we get extra testing -- not just that things work
on targets which support __builtin_speculation_safe_value, but also that
the behaviour works on targets which don't support it.

Tested with AArch64 native, AArch64 cross compiler, and RISC-V cross
compiler (just running the tests that I've changed).

Points of interest for any reviewer:

In the new `check_known_compiler_messages_nocache` procedure I use some
procedures from `prune.exp`.  This works for the use I need in
the g++ testsuite since g++.exp imports prune.exp and g++-dg.exp
includes gcc-dg.exp which does the initialisation of prune_notes
(needed for this procedure).
- Would it be preferred to include a `load_lib prune.exp` statement at
  the top of `target-supports.exp` in order to use this procedure?
- What about the handling of `initialize_prune_notes` which must have
  been called before calling `prune_gcc_output`?
- I believe it's sensible to not use `gcc-dg-prune` which wraps
  `prune_gcc_output` since I don't believe the wrapping functionality
  useful here -- wanted to highlight that decision for review.

Ok for trunk?

gcc/testsuite/ChangeLog:

	PR/117991
	* g++.dg/template/builtin-speculation-overloads.def: SUCCESS
	argument in SPECULATION_ASSERTS now uses a macro `true_def`
	instead of the literal `true` for arguments which should work
	with `__builtin_speculation_safe_value`.
	* g++.dg/template/builtin-speculation-overloads1.C: Define
	`true_def` macro on command line to compiler according to the
	effective target representing that
	`__builtin_speculation_safe_value` does something on this
	target.
	* g++.dg/template/builtin-speculation-overloads4.C: Likewise.
	* lib/target-supports.exp
	(check_known_compiler_messages_nocache): New.
	(check_effective_target_speculation_barrier_defined): New.

Signed-off-by: Matthew Malcomson <mmalcomson@nvidia.com>
---
 .../builtin-speculation-overloads.def         |  9 ++-
 .../template/builtin-speculation-overloads1.C |  2 +
 .../template/builtin-speculation-overloads4.C |  2 +
 gcc/testsuite/lib/target-supports.exp         | 62 +++++++++++++++++++
 4 files changed, 72 insertions(+), 3 deletions(-)

Comments

Jason Merrill Feb. 12, 2025, 11:30 p.m. UTC | #1 
On 2/12/25 5:23 PM, mmalcomson@nvidia.com wrote:
> From: Matthew Malcomson <mmalcomson@nvidia.com>
> 
> I've posted the patch on the relevant Bugzilla, but also sending to
> mailing list.  If should have only done one please do mention.

Having it in both places is fine, or send to the mailing list and put a 
link to the list in bugzilla.

> ----------------- 8< ----------- >8 ----------------
> When making warnings trigger a failure in template substitution I
> could not find any way to trigger the warning about builtin speculation
> not being available on the given target.
> 
> Turns out I misread the code -- this warning happens when the
> speculation_barrier pattern is not defined.
> 
> Here we add an effective target to represent
> "__builtin_speculation_safe_value is available on this target" and use
> that to adjust our test on SFINAE behaviour accordingly.
> N.b. this means that we get extra testing -- not just that things work
> on targets which support __builtin_speculation_safe_value, but also that
> the behaviour works on targets which don't support it.
> 
> Tested with AArch64 native, AArch64 cross compiler, and RISC-V cross
> compiler (just running the tests that I've changed).
> 
> Points of interest for any reviewer:
> 
> In the new `check_known_compiler_messages_nocache` procedure I use some

Why is it not enough to look for the message with "[regexp" like 
check_alias_available does?

Jason
Matthew Malcomson Feb. 13, 2025, 10:38 a.m. UTC | #2 
On 2/12/25 23:30, Jason Merrill wrote:
> External email: Use caution opening links or attachments
>>
>> In the new `check_known_compiler_messages_nocache` procedure I use some
> 
> Why is it not enough to look for the message with "[regexp" like
> check_alias_available does?
> 
> Jason
> 

The goal was that I wanted to be able to query "the warnings/errors are 
*only* about this thing", rather than "warnings mention this thing".

That said, since my use-case here is to give a boolean, the hypothetical 
case of "extra" messages has to be categorised in one or the other bucket.
Since the final behaviour would be much the same -- possible "excess 
error" messages on targets which support 
__builtin_speculation_safe_value instead of on targets which don't -- a 
simple `regexp` would work for this patch just as well.

Shall I make that change?
Jason Merrill Feb. 14, 2025, 7:56 a.m. UTC | #3 
On 2/13/25 11:38 AM, Matthew Malcomson wrote:
> On 2/12/25 23:30, Jason Merrill wrote:
>>>
>>> In the new `check_known_compiler_messages_nocache` procedure I use some
>>
>> Why is it not enough to look for the message with "[regexp" like
>> check_alias_available does?
> 
> The goal was that I wanted to be able to query "the warnings/errors are 
> *only* about this thing", rather than "warnings mention this thing".
> 
> That said, since my use-case here is to give a boolean, the hypothetical 
> case of "extra" messages has to be categorised in one or the other bucket.
> Since the final behaviour would be much the same -- possible "excess 
> error" messages on targets which support 
> __builtin_speculation_safe_value instead of on targets which don't -- a 
> simple `regexp` would work for this patch just as well.
> 
> Shall I make that change?

Please.

Jason
Matthew Malcomson Feb. 14, 2025, 10:48 a.m. UTC | #4 
On 2/14/25 07:56, Jason Merrill wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2/13/25 11:38 AM, Matthew Malcomson wrote:
>> On 2/12/25 23:30, Jason Merrill wrote:
>>>>
>> Shall I make that change?
> 
> Please.
> 
> Jason
> 

Much simpler patch attached ;-)

Ok for trunk?
Jason Merrill Feb. 14, 2025, 10:03 p.m. UTC | #5 
On 2/14/25 11:48 AM, Matthew Malcomson wrote:
> 
> 
> On 2/14/25 07:56, Jason Merrill wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 2/13/25 11:38 AM, Matthew Malcomson wrote:
>>> On 2/12/25 23:30, Jason Merrill wrote:
>>>>>
>>> Shall I make that change?
>>
>> Please.
>>
>> Jason
>>
> 
> Much simpler patch attached ;-)
> 
> Ok for trunk?

OK.

Jason
diff mbox series

Patch

diff --git a/gcc/testsuite/g++.dg/template/builtin-speculation-overloads.def b/gcc/testsuite/g++.dg/template/builtin-speculation-overloads.def
index 39d9b748d52..ada13e6f77c 100644
--- a/gcc/testsuite/g++.dg/template/builtin-speculation-overloads.def
+++ b/gcc/testsuite/g++.dg/template/builtin-speculation-overloads.def
@@ -15,14 +15,17 @@  class X{};
 class Large { public: int arr[10]; };
 class Incomplete;
 
+/* Using `true_def` in order to account for the fact that if this target
+ * doesn't support __builtin_speculation_safe_value at all everything fails to
+ * substitute.  */
 #define SPECULATION_ASSERTS                                                    \
-  MAKE_SPECULATION_ASSERT (int, true)                                          \
+  MAKE_SPECULATION_ASSERT (int, true_def)                                      \
   MAKE_SPECULATION_ASSERT (float, false)                                       \
   MAKE_SPECULATION_ASSERT (X, false)                                           \
   MAKE_SPECULATION_ASSERT (Large, false)                                       \
   MAKE_SPECULATION_ASSERT (Incomplete, false)                                  \
-  MAKE_SPECULATION_ASSERT (int *, true)                                        \
-  MAKE_SPECULATION_ASSERT (long, true)
+  MAKE_SPECULATION_ASSERT (int *, true_def)                                    \
+  MAKE_SPECULATION_ASSERT (long, true_def)
 
 int main() {
     SPECULATION_ASSERTS
diff --git a/gcc/testsuite/g++.dg/template/builtin-speculation-overloads1.C b/gcc/testsuite/g++.dg/template/builtin-speculation-overloads1.C
index bc8f1083a99..4c50d4aa6f5 100644
--- a/gcc/testsuite/g++.dg/template/builtin-speculation-overloads1.C
+++ b/gcc/testsuite/g++.dg/template/builtin-speculation-overloads1.C
@@ -1,5 +1,7 @@ 
 /* Check that overloaded builtins can be used in templates with SFINAE.  */
 // { dg-do compile { target c++17 } }
+// { dg-additional-options "-Dtrue_def=true" { target speculation_barrier_defined } }
+// { dg-additional-options "-Dtrue_def=false" { target { ! speculation_barrier_defined } } }
 
 /* Checks performed here:
    Various types (some that work, some that don't).  */
diff --git a/gcc/testsuite/g++.dg/template/builtin-speculation-overloads4.C b/gcc/testsuite/g++.dg/template/builtin-speculation-overloads4.C
index c024a21fa18..cc0b3131af7 100644
--- a/gcc/testsuite/g++.dg/template/builtin-speculation-overloads4.C
+++ b/gcc/testsuite/g++.dg/template/builtin-speculation-overloads4.C
@@ -1,5 +1,7 @@ 
 /* Check that overloaded builtins can be used in templates with SFINAE.  */
 // { dg-do compile { target c++17 } }
+// { dg-additional-options "-Dtrue_def=true" { target speculation_barrier_defined } }
+// { dg-additional-options "-Dtrue_def=false" { target { ! speculation_barrier_defined } } }
 
 /* Checks performed here:
    Optional parameter missing works same as with optional parameter specified.  */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index ff95b88f3c4..b8a667d9187 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -180,8 +180,51 @@  proc clear_effective_target_cache { } {
     array unset et_cache
 }
 
+# Like check_no_compiler_messages_nocache, but returns true if the compiler
+# printed a known set of messages.  `messages` is a list of messages to expect
+# *after* pruning with `prune_gcc_output` and removing empty lines.  Each entry
+# in the list should be a regexp pattern matching each line of the output in
+# turn.
+proc check_known_compiler_messages_nocache { messages args } {
+    verbose "Arguments: $args" 3
+    verbose "messages: \"$messages\"" 3
+    initialize_prune_notes
+    set result [eval check_compile $args]
+    set tool_output [lindex $result 0]
+    set output_file [lindex $result 1]
+    remote_file build delete $output_file
+    verbose "pruning output of: \"$tool_output\"" 3
+    set tool_output [prune_gcc_output $tool_output]
+    set lines [split $tool_output "\n"]
+    # Remove empty lines to make interface to this procedure nice.
+    verbose "Comparing \"$lines\" against \"$messages\"" 3
+    set lines [lmap value $lines {
+	if { ! [string match "" $value] } {
+	    string cat $value
+	} else {
+	    continue
+	}}]
+    verbose "After removing empty lines \"$lines\" against \"$messages\"" 3
+    if { [llength $lines] != [llength $messages] } {
+	verbose "message lengths different" 3
+	return 0
+    }
+    verbose "message lengths same" 3
+    foreach msg $messages line $lines {
+	verbose "Comparing \"$msg\" against \"$line\"" 3
+	if { ! [regexp $msg $line] } {
+	    verbose "line did not compare equal" 3
+	    return 0
+	}
+    }
+    verbose "message values same" 3
+    return 1
+}
+
 # Like check_compile, but delete the output file and return true if the
 # compiler printed no messages.
+# Do not implement in terms of `check_known_compiler_messages_nocach` since
+# that procedure includes pruning of messages.
 proc check_no_compiler_messages_nocache {args} {
     set result [eval check_compile $args]
     set lines [lindex $result 0]
@@ -14304,3 +14347,22 @@  proc check_effective_target_alarm { } {
 	}
     }]
 }
+
+# Return true if a speculation barrier is defined on the target.
+proc check_effective_target_speculation_barrier_defined { } {
+    # N.b. not using the existing abstractions around `gcc_warning_prefix`
+    # since that's set depending on the tool that's getting tested while this
+    # effective target is used for all tools.  Moreover this effective target
+    # is always compiling C while different tools are often testing different
+    # languages.  Hence we always know the warning prefix that would be used
+    # for this test.
+    return [check_cached_effective_target speculation_barrier_defined {
+	expr ! [check_known_compiler_messages_nocache \
+	    [list "warning: this target does not define a speculation barrier.*"] \
+	    specbar assembly {
+		int main (int argc, char **argv) { 
+		    return __builtin_speculation_safe_value (argc);
+		}
+	    }]
+    }]
+}