Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level (was: GCC 12.1 Release Candidate available from gcc.gnu.org)

Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level (was: GCC 12.1 Release Candidate available from gcc.gnu.org)

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

Hi!

On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
>> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
>>
>> The first release candidate for GCC 12.1 is available from
>>
>> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>>
>> and shortly its mirrors.  It has been generated from git commit
>> r12-8321-g621650f64fb667.

> [...] new fails (presumably because checking is off):

> XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (internal compiler error)
> FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (test for excess errors)
> XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (internal compiler error)
> FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (test for excess errors)
> XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (internal compiler error)
> FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (test for excess errors)
> XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (internal compiler error)
> FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (test for excess errors)

Confirmed, and sorry.  I had taken care to add explicit '-fchecking'
next to 'dg-ice', but that's in fact not the problem/cure here.
OK to push to the relevant branches the attached
"Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
consistently, regardless of checking level"?


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-c-c-common-goacc-kernels-decompose-pr100400-1-..patch --]
[-- Type: text/x-diff, Size: 5229 bytes --]

From 7827d018a9e0109b8d271ceb824f2c718bae508e Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Mon, 2 May 2022 15:15:26 +0200
Subject: [PATCH] Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c'
 behave consistently, regardless of checking level

Fix-up for commit c14ea6a72fb1ae66e3d32ac8329558497c6e4403
"Catch 'GIMPLE_DEBUG' misbehavior in OpenACC 'kernels' decomposition
[PR100400, PR103836, PR104061]".

For C++ compilation of 'c-c++-common/goacc/kernels-decompose-pr100400-1-2.c',
we first emit a 'sorry' diagnostic, and then a 'gcc_unreachable' (or 'abort',
see below) diagnostic, but for example, for '--enable-checking=release' (thus,
'!CHECKING_P'), the second one may actually be turned into a
'confused by earlier errors, bailing out' diagnostic.  (See
'gcc/diagnostic.cc:diagnostic_report_diagnostic': "When not checking, ICEs are
converted to fatal errors when an error has already occurred.")  Thus, make
'c-c++-common/goacc/kernels-decompose-pr100400-1-2.c' behave consistently via
'-Wfatal-errors', and thus only matching the 'sorry' diagnostic.

For example, for '--enable-checking=no' (thus, '!ENABLE_ASSERT_CHECKING'), a
call to 'gcc_unreachable' cannot be assumed emit an 'abort'-like diagnostic, so
explicitly call 'abort' in
'gcc/omp-oacc-kernels-decompose.cc:visit_loops_in_gang_single_region', in the
'GIMPLE_OMP_FOR' case, to avoid regressing
'c-c++-common/goacc/kernels-decompose-pr100400-1-3.c', and
'c-c++-common/goacc/kernels-decompose-pr100400-1-4.c'.

	PR middle-end/100400
	gcc/
	* omp-oacc-kernels-decompose.cc
	(visit_loops_in_gang_single_region) <GIMPLE_OMP_FOR>: Reliably
	'abort'.
	gcc/testsuite/
	* c-c++-common/goacc/kernels-decompose-pr100400-1-2.c: Specify
	'-Wfatal-errors'.
---
 gcc/omp-oacc-kernels-decompose.cc                    |  6 ++++++
 .../goacc/kernels-decompose-pr100400-1-2.c           | 12 +++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/gcc/omp-oacc-kernels-decompose.cc b/gcc/omp-oacc-kernels-decompose.cc
index 4386787ba3c..9f5c1ecb255 100644
--- a/gcc/omp-oacc-kernels-decompose.cc
+++ b/gcc/omp-oacc-kernels-decompose.cc
@@ -239,7 +239,13 @@ visit_loops_in_gang_single_region (gimple_stmt_iterator *gsi_p,
     case GIMPLE_OMP_FOR:
       /*TODO Given the current 'adjust_region_code' algorithm, this is
 	actually...  */
+#if 0
       gcc_unreachable ();
+#else
+      /* ..., but due to bugs (PR100400), we may actually come here.
+	 Reliably catch this, regardless of checking level.  */
+      abort ();
+#endif
 
       {
 	tree clauses = gimple_omp_for_clauses (stmt);
diff --git a/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c b/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c
index a643f109bf1..8b65e07c623 100644
--- a/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c
+++ b/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c
@@ -1,8 +1,8 @@
 /* { dg-additional-options "--param openacc-kernels=decompose" } */
 
-/* { dg-additional-options "-fchecking" }
-   { dg-ice TODO { c++ } }
-   { dg-prune-output "during GIMPLE pass: omp_oacc_kernels_decompose" } */
+/* Ensure consistent diagnostics, regardless of checking level:
+   { dg-additional-options -Wfatal-errors }
+   { dg-message {terminated due to -Wfatal-errors} TODO { target *-*-* } 0 } */
 
 /* { dg-additional-options "-g" } */
 /* { dg-additional-options "-O1" } so that we may get some 'GIMPLE_DEBUG's.  */
@@ -19,18 +19,16 @@ foo (void)
   /* { dg-bogus {sorry, unimplemented: 'gimple_debug' not yet supported} TODO { xfail *-*-* } .+1 } */
 #pragma acc kernels /* { dg-line l_compute1 } */
   /* { dg-note {OpenACC 'kernels' decomposition: variable 'p' in 'copy' clause requested to be made addressable} {} { target *-*-* } l_compute1 }
-     { dg-note {variable 'p' made addressable} {} { target *-*-* xfail c++ } l_compute1 } */
+     { dg-note {variable 'p' made addressable} {} { xfail *-*-* } l_compute1 } */
   /* { dg-note {variable 'c' declared in block is candidate for adjusting OpenACC privatization level} {} { xfail *-*-* } l_compute1 } */
   /* { dg-note {variable 'c\.0' declared in block isn't candidate for adjusting OpenACC privatization level: not addressable} {} { xfail *-*-* } l_compute1 } */
   {
-    /* { dg-bogus {note: beginning 'gang-single' part in OpenACC 'kernels' region} {w/ debug} { xfail c++ } .-1 }
-       { dg-bogus {note: beginning 'gang-single' part in OpenACC 'kernels' region} {w/ debug} { xfail c } .+1 } */
     int c;
 
     /* { dg-note {beginning 'gang-single' part in OpenACC 'kernels' region} {} { xfail *-*-* } .+1 } */
     p = &c;
 
-    /* { dg-note {parallelized loop nest in OpenACC 'kernels' region} {} { xfail c++ } .+1 } */
+    /* { dg-note {parallelized loop nest in OpenACC 'kernels' region} {} { xfail *-*-* } .+1 } */
 #pragma acc loop independent /* { dg-line l_loop_c1 } */
     /* { dg-note {variable 'c\.0' in 'private' clause isn't candidate for adjusting OpenACC privatization level: not addressable} {} { xfail *-*-* } l_loop_c1 } */
     /* { dg-note {variable 'c' in 'private' clause is candidate for adjusting OpenACC privatization level} {} { xfail *-*-* } l_loop_c1 }
-- 
2.35.1

Re: Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level (was: GCC 12.1 Release Candidate available from gcc.gnu.org)

[Richard Biener]

On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>
> Hi!
>
> On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
> >> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
> >>
> >> The first release candidate for GCC 12.1 is available from
> >>
> >> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >>
> >> and shortly its mirrors.  It has been generated from git commit
> >> r12-8321-g621650f64fb667.
>
> > [...] new fails (presumably because checking is off):
>
> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (internal compiler error)
> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (test for excess errors)
> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (internal compiler error)
> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (test for excess errors)
> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (internal compiler error)
> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (test for excess errors)
> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (internal compiler error)
> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (test for excess errors)
>
> Confirmed, and sorry.  I had taken care to add explicit '-fchecking'
> next to 'dg-ice', but that's in fact not the problem/cure here.
> OK to push to the relevant branches the attached
> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
> consistently, regardless of checking level"?

No,

+++ b/gcc/omp-oacc-kernels-decompose.cc
@@ -239,7 +239,13 @@ visit_loops_in_gang_single_region
(gimple_stmt_iterator *gsi_p,
     case GIMPLE_OMP_FOR:
       /*TODO Given the current 'adjust_region_code' algorithm, this is
        actually...  */
+#if 0
       gcc_unreachable ();
+#else
+      /* ..., but due to bugs (PR100400), we may actually come here.
+        Reliably catch this, regardless of checking level.  */
+      abort ();
+#endif

this doesn't look correct.  If you want a reliable diagnostic here please use
sorry ("...") or call internal_error () manually (the IL verifiers do this).

That said, having a testcase that checks for an ICE as a TODO is maybe not
the very best thing to have.  We have bugzilla for unfixed bugs.

Richard.

>
> Grüße
>  Thomas
>
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level

[Thomas Schwinge]

Hi Richard!

On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
>> >> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
>> >>
>> >> The first release candidate for GCC 12.1 is available from
>> >>
>> >> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>> >> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>> >>
>> >> and shortly its mirrors.  It has been generated from git commit
>> >> r12-8321-g621650f64fb667.
>>
>> > [...] new fails (presumably because checking is off):
>>
>> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (internal compiler error)
>> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (test for excess errors)
>> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (internal compiler error)
>> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (test for excess errors)
>> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (internal compiler error)
>> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (test for excess errors)
>> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (internal compiler error)
>> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (test for excess errors)
>>
>> Confirmed, and sorry.  I had taken care to add explicit '-fchecking'
>> next to 'dg-ice', but that's in fact not the problem/cure here.
>> OK to push to the relevant branches the attached
>> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
>> consistently, regardless of checking level"?
>
> No,
>
> +++ b/gcc/omp-oacc-kernels-decompose.cc
> @@ -239,7 +239,13 @@ visit_loops_in_gang_single_region
> (gimple_stmt_iterator *gsi_p,
>      case GIMPLE_OMP_FOR:
>        /*TODO Given the current 'adjust_region_code' algorithm, this is
>         actually...  */
> +#if 0
>        gcc_unreachable ();
> +#else
> +      /* ..., but due to bugs (PR100400), we may actually come here.
> +        Reliably catch this, regardless of checking level.  */
> +      abort ();
> +#endif
>
> this doesn't look correct.  If you want a reliable diagnostic here please use
> sorry ("...") or call internal_error () manually (the IL verifiers do this).

Hmm, I feel I'm going in circles...  ;-)

Here, 'sorry' isn't appropriate, because this is a plain bug, and not
"a language feature which is required by the relevant specification but
not implemented by GCC" ('gcc/diagnostic.cc').

I first had this as 'internal_error', but then saw the following source
code comment, 'gcc/diagnostic.cc':

    /* An internal consistency check has failed.  We make no attempt to
       continue.  Note that unless there is debugging value to be had from
       a more specific message, or some other good reason, you should use
       abort () instead of calling this function directly.  */
    void
    internal_error (const char *gmsgid, ...)
    {

Here, there's no "debugging value to be had from a more specific
message", and I couldn't think of "some other good reason", so decided to
"use abort () instead of calling this function directly".

But, if that's what we want, I'm happy to again switch to
'internal_error', and then we should update this source code comment,
too?


> That said, having a testcase that checks for an ICE as a TODO is maybe not
> the very best thing to have.  We have bugzilla for unfixed bugs.

As per the past 'dg-ice' discussions, there's certainly value seen for
having such test cases (and it already did help me during development,
elsewhere).

I do agree/acknowledge that it's a bit more difficult to make those
behave consistently across GCC configurations.


Grüße
 Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level

[Richard Biener]

On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
>
> Hi Richard!
>
> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >> On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
> >> >> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
> >> >>
> >> >> The first release candidate for GCC 12.1 is available from
> >> >>
> >> >> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >> >> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >> >>
> >> >> and shortly its mirrors.  It has been generated from git commit
> >> >> r12-8321-g621650f64fb667.
> >>
> >> > [...] new fails (presumably because checking is off):
> >>
> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (internal compiler error)
> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (test for excess errors)
> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (internal compiler error)
> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (test for excess errors)
> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (internal compiler error)
> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (test for excess errors)
> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (internal compiler error)
> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (test for excess errors)
> >>
> >> Confirmed, and sorry.  I had taken care to add explicit '-fchecking'
> >> next to 'dg-ice', but that's in fact not the problem/cure here.
> >> OK to push to the relevant branches the attached
> >> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
> >> consistently, regardless of checking level"?
> >
> > No,
> >
> > +++ b/gcc/omp-oacc-kernels-decompose.cc
> > @@ -239,7 +239,13 @@ visit_loops_in_gang_single_region
> > (gimple_stmt_iterator *gsi_p,
> >      case GIMPLE_OMP_FOR:
> >        /*TODO Given the current 'adjust_region_code' algorithm, this is
> >         actually...  */
> > +#if 0
> >        gcc_unreachable ();
> > +#else
> > +      /* ..., but due to bugs (PR100400), we may actually come here.
> > +        Reliably catch this, regardless of checking level.  */
> > +      abort ();
> > +#endif
> >
> > this doesn't look correct.  If you want a reliable diagnostic here please use
> > sorry ("...") or call internal_error () manually (the IL verifiers do this).
>
> Hmm, I feel I'm going in circles...  ;-)
>
> Here, 'sorry' isn't appropriate, because this is a plain bug, and not
> "a language feature which is required by the relevant specification but
> not implemented by GCC" ('gcc/diagnostic.cc').
>
> I first had this as 'internal_error', but then saw the following source
> code comment, 'gcc/diagnostic.cc':
>
>     /* An internal consistency check has failed.  We make no attempt to
>        continue.  Note that unless there is debugging value to be had from
>        a more specific message, or some other good reason, you should use
>        abort () instead of calling this function directly.  */
>     void
>     internal_error (const char *gmsgid, ...)
>     {
>
> Here, there's no "debugging value to be had from a more specific
> message", and I couldn't think of "some other good reason", so decided to
> "use abort () instead of calling this function directly".

I think that is misguided.

> But, if that's what we want, I'm happy to again switch to
> 'internal_error', and then we should update this source code comment,
> too?
>
>
> > That said, having a testcase that checks for an ICE as a TODO is maybe not
> > the very best thing to have.  We have bugzilla for unfixed bugs.
>
> As per the past 'dg-ice' discussions, there's certainly value seen for
> having such test cases (and it already did help me during development,
> elsewhere).
>
> I do agree/acknowledge that it's a bit more difficult to make those
> behave consistently across GCC configurations.

So maybe do

   internal_error ("PRnnnn");

then?

>
> Grüße
>  Thomas
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 6530 bytes --]

Hi Richard!

On 2022-05-03T12:53:50+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
>> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> >> On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
>> >> >> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
>> >> >>
>> >> >> The first release candidate for GCC 12.1 is available from
>> >> >>
>> >> >> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>> >> >> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>> >> >>
>> >> >> and shortly its mirrors.  It has been generated from git commit
>> >> >> r12-8321-g621650f64fb667.
>> >>
>> >> > [...] new fails (presumably because checking is off):
>> >>
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (test for excess errors)
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (test for excess errors)
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (test for excess errors)
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (test for excess errors)
>> >>
>> >> Confirmed, and sorry.  I had taken care to add explicit '-fchecking'
>> >> next to 'dg-ice', but that's in fact not the problem/cure here.
>> >> OK to push to the relevant branches the attached
>> >> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
>> >> consistently, regardless of checking level"?
>> >
>> > No,
>> >
>> > +++ b/gcc/omp-oacc-kernels-decompose.cc
>> > @@ -239,7 +239,13 @@ visit_loops_in_gang_single_region
>> > (gimple_stmt_iterator *gsi_p,
>> >      case GIMPLE_OMP_FOR:
>> >        /*TODO Given the current 'adjust_region_code' algorithm, this is
>> >         actually...  */
>> > +#if 0
>> >        gcc_unreachable ();
>> > +#else
>> > +      /* ..., but due to bugs (PR100400), we may actually come here.
>> > +        Reliably catch this, regardless of checking level.  */
>> > +      abort ();
>> > +#endif
>> >
>> > this doesn't look correct.  If you want a reliable diagnostic here please use
>> > sorry ("...") or call internal_error () manually (the IL verifiers do this).
>>
>> Hmm, I feel I'm going in circles...  ;-)
>>
>> Here, 'sorry' isn't appropriate, because this is a plain bug, and not
>> "a language feature which is required by the relevant specification but
>> not implemented by GCC" ('gcc/diagnostic.cc').
>>
>> I first had this as 'internal_error', but then saw the following source
>> code comment, 'gcc/diagnostic.cc':
>>
>>     /* An internal consistency check has failed.  We make no attempt to
>>        continue.  Note that unless there is debugging value to be had from
>>        a more specific message, or some other good reason, you should use
>>        abort () instead of calling this function directly.  */
>>     void
>>     internal_error (const char *gmsgid, ...)
>>     {
>>
>> Here, there's no "debugging value to be had from a more specific
>> message", and I couldn't think of "some other good reason", so decided to
>> "use abort () instead of calling this function directly".
>
> I think that is misguided.

So that I know which one to fix/reconsider: does your "that" refer to the
'gcc/diagnostic.cc:internal_error' source code comment cited above, or my
interpretation of it?

>> But, if that's what we want, I'm happy to again switch to
>> 'internal_error', and then we should update this source code comment,
>> too?

> So maybe do
>
>    internal_error ("PRnnnn");
>
> then?

Sure, but note that this just changes from:

    [...]
    during GIMPLE pass: omp_oacc_kernels_decompose
    dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
    source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
    source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: in visit_loops_in_gang_single_region, at omp-oacc-kernels-decompose.cc:247
       42 |       ;
          |       ^
    [backtrace]

... to:

    [...]
    during GIMPLE pass: omp_oacc_kernels_decompose
    dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
    source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
    source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: PR100400
       42 |       ;
          |       ^
    [backtrace]

..., and it wasn't clear to me that it's an actual improvement to
diagnose "internal compiler error: PR100400" instead of
"internal compiler error: in visit_loops_in_gang_single_region, at
omp-oacc-kernels-decompose.cc:247" (with that location pointing to
PR100400).

That's 'gcc/system.h':

    #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)

..., plus 'gcc/diagnostic.cc':

    /* Report an internal compiler error in a friendly manner.  This is
       the function that gets called upon use of abort() in the source
       code generally, thanks to a special macro.  */

    void
    fancy_abort (const char *file, int line, const char *function)
    {
      [...]
      internal_error ("in %s, at %s:%d", function, trim_filename (file), line);
    }

Anyway, fine by me, if you like that better?  ;-P

OK then to push to the relevant branches the attached
"Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
consistently, regardless of checking level"?


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-c-c-common-goacc-kernels-decompose-pr100400-1-..patch --]
[-- Type: text/x-diff, Size: 5291 bytes --]

From ff9e80d1aa257ec53637b14641c2b3027dbf7675 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Mon, 2 May 2022 15:15:26 +0200
Subject: [PATCH] Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c'
 behave consistently, regardless of checking level

Fix-up for commit c14ea6a72fb1ae66e3d32ac8329558497c6e4403
"Catch 'GIMPLE_DEBUG' misbehavior in OpenACC 'kernels' decomposition
[PR100400, PR103836, PR104061]".

For C++ compilation of 'c-c++-common/goacc/kernels-decompose-pr100400-1-2.c',
we first emit a 'sorry' diagnostic, and then a 'gcc_unreachable' (or
'internal_error', see below) diagnostic, but for example, for
'--enable-checking=release' (thus, '!CHECKING_P'), the second one may actually
be turned into a 'confused by earlier errors, bailing out' diagnostic.  (See
'gcc/diagnostic.cc:diagnostic_report_diagnostic': "When not checking, ICEs are
converted to fatal errors when an error has already occurred.")  Thus, make
'c-c++-common/goacc/kernels-decompose-pr100400-1-2.c' behave consistently via
'-Wfatal-errors', and thus only matching the 'sorry' diagnostic.

For example, for '--enable-checking=no' (thus, '!ENABLE_ASSERT_CHECKING'), a
call to 'gcc_unreachable' cannot be assumed emit an 'internal_error'-like
diagnostic, so explicitly call 'internal_error' in
'gcc/omp-oacc-kernels-decompose.cc:visit_loops_in_gang_single_region', in the
'GIMPLE_OMP_FOR' case, to avoid regressing
'c-c++-common/goacc/kernels-decompose-pr100400-1-3.c', and
'c-c++-common/goacc/kernels-decompose-pr100400-1-4.c'.

	PR middle-end/100400
	gcc/
	* omp-oacc-kernels-decompose.cc
	(visit_loops_in_gang_single_region) <GIMPLE_OMP_FOR>: Explicitly
	call 'internal_error'.
	gcc/testsuite/
	* c-c++-common/goacc/kernels-decompose-pr100400-1-2.c: Specify
	'-Wfatal-errors'.
---
 gcc/omp-oacc-kernels-decompose.cc                    |  6 ++++++
 .../goacc/kernels-decompose-pr100400-1-2.c           | 12 +++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/gcc/omp-oacc-kernels-decompose.cc b/gcc/omp-oacc-kernels-decompose.cc
index 4386787ba3c..ec9b0faab0a 100644
--- a/gcc/omp-oacc-kernels-decompose.cc
+++ b/gcc/omp-oacc-kernels-decompose.cc
@@ -239,7 +239,13 @@ visit_loops_in_gang_single_region (gimple_stmt_iterator *gsi_p,
     case GIMPLE_OMP_FOR:
       /*TODO Given the current 'adjust_region_code' algorithm, this is
 	actually...  */
+#if 0
       gcc_unreachable ();
+#else
+      /* ..., but due to bugs (PR100400), we may actually come here.
+	 Reliably catch this, regardless of checking level.  */
+      internal_error ("PR100400");
+#endif
 
       {
 	tree clauses = gimple_omp_for_clauses (stmt);
diff --git a/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c b/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c
index a643f109bf1..8b65e07c623 100644
--- a/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c
+++ b/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c
@@ -1,8 +1,8 @@
 /* { dg-additional-options "--param openacc-kernels=decompose" } */
 
-/* { dg-additional-options "-fchecking" }
-   { dg-ice TODO { c++ } }
-   { dg-prune-output "during GIMPLE pass: omp_oacc_kernels_decompose" } */
+/* Ensure consistent diagnostics, regardless of checking level:
+   { dg-additional-options -Wfatal-errors }
+   { dg-message {terminated due to -Wfatal-errors} TODO { target *-*-* } 0 } */
 
 /* { dg-additional-options "-g" } */
 /* { dg-additional-options "-O1" } so that we may get some 'GIMPLE_DEBUG's.  */
@@ -19,18 +19,16 @@ foo (void)
   /* { dg-bogus {sorry, unimplemented: 'gimple_debug' not yet supported} TODO { xfail *-*-* } .+1 } */
 #pragma acc kernels /* { dg-line l_compute1 } */
   /* { dg-note {OpenACC 'kernels' decomposition: variable 'p' in 'copy' clause requested to be made addressable} {} { target *-*-* } l_compute1 }
-     { dg-note {variable 'p' made addressable} {} { target *-*-* xfail c++ } l_compute1 } */
+     { dg-note {variable 'p' made addressable} {} { xfail *-*-* } l_compute1 } */
   /* { dg-note {variable 'c' declared in block is candidate for adjusting OpenACC privatization level} {} { xfail *-*-* } l_compute1 } */
   /* { dg-note {variable 'c\.0' declared in block isn't candidate for adjusting OpenACC privatization level: not addressable} {} { xfail *-*-* } l_compute1 } */
   {
-    /* { dg-bogus {note: beginning 'gang-single' part in OpenACC 'kernels' region} {w/ debug} { xfail c++ } .-1 }
-       { dg-bogus {note: beginning 'gang-single' part in OpenACC 'kernels' region} {w/ debug} { xfail c } .+1 } */
     int c;
 
     /* { dg-note {beginning 'gang-single' part in OpenACC 'kernels' region} {} { xfail *-*-* } .+1 } */
     p = &c;
 
-    /* { dg-note {parallelized loop nest in OpenACC 'kernels' region} {} { xfail c++ } .+1 } */
+    /* { dg-note {parallelized loop nest in OpenACC 'kernels' region} {} { xfail *-*-* } .+1 } */
 #pragma acc loop independent /* { dg-line l_loop_c1 } */
     /* { dg-note {variable 'c\.0' in 'private' clause isn't candidate for adjusting OpenACC privatization level: not addressable} {} { xfail *-*-* } l_loop_c1 } */
     /* { dg-note {variable 'c' in 'private' clause is candidate for adjusting OpenACC privatization level} {} { xfail *-*-* } l_loop_c1 }
-- 
2.25.1

Re: Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level

[Richard Biener]

On Tue, May 3, 2022 at 2:29 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>
> Hi Richard!
>
> On 2022-05-03T12:53:50+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> > On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> >> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >> >> On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
> >> >> >> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
> >> >> >>
> >> >> >> The first release candidate for GCC 12.1 is available from
> >> >> >>
> >> >> >> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >> >> >> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >> >> >>
> >> >> >> and shortly its mirrors.  It has been generated from git commit
> >> >> >> r12-8321-g621650f64fb667.
> >> >>
> >> >> > [...] new fails (presumably because checking is off):
> >> >>
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++98 (test for excess errors)
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++14 (test for excess errors)
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++17 (test for excess errors)
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c  -std=c++20 (test for excess errors)
> >> >>
> >> >> Confirmed, and sorry.  I had taken care to add explicit '-fchecking'
> >> >> next to 'dg-ice', but that's in fact not the problem/cure here.
> >> >> OK to push to the relevant branches the attached
> >> >> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
> >> >> consistently, regardless of checking level"?
> >> >
> >> > No,
> >> >
> >> > +++ b/gcc/omp-oacc-kernels-decompose.cc
> >> > @@ -239,7 +239,13 @@ visit_loops_in_gang_single_region
> >> > (gimple_stmt_iterator *gsi_p,
> >> >      case GIMPLE_OMP_FOR:
> >> >        /*TODO Given the current 'adjust_region_code' algorithm, this is
> >> >         actually...  */
> >> > +#if 0
> >> >        gcc_unreachable ();ember all reasons, but parts of them simply
               was: new and potentia
> >> > +#else
> >> > +      /* ..., but due to bugs (PR100400), we may actually come here.
> >> > +        Reliably catch this, regardless of checking level.  */
> >> > +      abort ();
> >> > +#endif
> >> >
> >> > this doesn't look correct.  If you want a reliable diagnostic here please use
> >> > sorry ("...") or call internal_error () manually (the IL verifiers do this).
> >>
> >> Hmm, I feel I'm going in circles...  ;-)
> >>
> >> Here, 'sorry' isn't appropriate, because this is a plain bug, and not
> >> "a language feature which is required by the relevant specification but
> >> not implemented by GCC" ('gcc/diagnostic.cc').
> >>
> >> I first had this as 'internal_error', but then saw the following source
> >> code comment, 'gcc/diagnostic.cc':
> >>
> >>     /* An internal consistency check has failed.  We make no attempt to
> >>        continue.  Note that unless there is debugging value to be had from
> >>        a more specific message, or some other good reason, you should use
> >>        abort () instead of calling this function directly.  */
> >>     void
> >>     internal_error (const char *gmsgid, ...)
> >>     {
> >>
> >> Here, there's no "debugging value to be had from a more specific
> >> message", and I couldn't think of "some other good reason", so decided to
> >> "use abort () instead of calling this function directly".
> >
> > I think that is misguided.
>
> So that I know which one to fix/reconsider: does your "that" refer to the
> 'gcc/diagnostic.cc:internal_error' source code comment cited above, or my
> interpretation of it?

The comment to "use abort ()".

> >> But, if that's what we want, I'm happy to again switch to
> >> 'internal_error', and then we should update this source code comment,
> >> too?
>
> > So maybe do
> >
> >    internal_error ("PRnnnn");
> >
> > then?
>
> Sure, but note that this just changes from:
>
>     [...]
>     during GIMPLE pass: omp_oacc_kernels_decompose
>     dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
>     source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
>     source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: in visit_loops_in_gang_single_region, at omp-oacc-kernels-decompose.cc:247
>        42 |       ;
>           |       ^
>     [backtrace]
>
> ... to:
>
>     [...]
>     during GIMPLE pass: omp_oacc_kernels_decompose
>     dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
>     source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
>     source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: PR100400
>        42 |       ;
>           |       ^
>     [backtrace]
>
> ..., and it wasn't clear to me that it's an actual improvement to
> diagnose "internal compiler error: PR100400" instead of
> "internal compiler error: in visit_loops_in_gang_single_region, at
> omp-oacc-kernels-decompose.cc:247" (with that location pointing to
> PR100400).

Well, it should be an improvement for consistency?  Ah, of course you still
get 'confused by earlier errors' then and a testsuite FAIL.

> That's 'gcc/system.h':
>
>     #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)
>
> ..., plus 'gcc/diagnostic.cc':
>
>     /* Report an internal compiler error in a friendly manner.  This is
>        the function that gets called upon use of abort() in the source
>        code generally, thanks to a special macro.  */
>
>     void
>     fancy_abort (const char *file, int line, const char *function)
>     {
>       [...]
>       internal_error ("in %s, at %s:%d", function, trim_filename (file), line);
>     }
>
> Anyway, fine by me, if you like that better?  ;-P
>
> OK then to push to the relevant branches the attached
> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
> consistently, regardless of checking level"?

OK for trunk and the GCC 12 branch after the 12.1 release.

Richard.

>
> Grüße
>  Thomas
>
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Advise to call 'internal_error' instead of 'abort' or 'fancy_abort'

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 2553 bytes --]

Hi!

On 2022-05-03T15:46:43+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> On Tue, May 3, 2022 at 2:29 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> On 2022-05-03T12:53:50+0200, Richard Biener <richard.guenther@gmail.com> wrote:
>> > On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> >> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
>> >> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> >> > +#if 0
>> >> >        gcc_unreachable ();
>> >> > +#else
>> >> > +      /* ..., but due to bugs (PR100400), we may actually come here.
>> >> > +        Reliably catch this, regardless of checking level.  */
>> >> > +      abort ();
>> >> > +#endif
>> >> >
>> >> > this doesn't look correct.  If you want a reliable diagnostic here please [...]
>> >> > call internal_error () manually (the IL verifiers do this).
>> >>
>> >> Hmm, I feel I'm going in circles...  ;-)

>> >> I first had this as 'internal_error', but then saw the following source
>> >> code comment, 'gcc/diagnostic.cc':
>> >>
>> >>     /* An internal consistency check has failed.  We make no attempt to
>> >>        continue.  Note that unless there is debugging value to be had from
>> >>        a more specific message, or some other good reason, you should use
>> >>        abort () instead of calling this function directly.  */
>> >>     void
>> >>     internal_error (const char *gmsgid, ...)
>> >>     {
>> >>
>> >> Here, there's no "debugging value to be had from a more specific
>> >> message", and I couldn't think of "some other good reason", so decided to
>> >> "use abort () instead of calling this function directly".
>> >
>> > I think that is misguided.
>>
>> So that I know which one to fix/reconsider: does your "that" refer to the
>> 'gcc/diagnostic.cc:internal_error' source code comment cited above, or my
>> interpretation of it?
>
> The comment to "use abort ()".

Does the attached
"Advise to call 'internal_error' instead of 'abort' or 'fancy_abort'"
capture what you had in mind?

(This is, obviously, not updating any of the many 'abort' or even a few
'fancy_abort' calls that we currently have.)


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Advise-to-call-internal_error-instead-of-abort-or-fa.patch --]
[-- Type: text/x-diff, Size: 1930 bytes --]

From a8017c7b5fa7b5e8210b6446acf7dd09989a7517 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 10 May 2022 15:56:08 +0200
Subject: [PATCH] Advise to call 'internal_error' instead of 'abort' or
 'fancy_abort'

	gcc/
	* diagnostic.cc: Don't advise to call 'abort' instead of
	'internal_error'.
	* system.h: Advise to call 'internal_error' instead of 'abort' or
	'fancy_abort'.

Suggested-by: Richard Biener <richard.guenther@gmail.com>
---
 gcc/diagnostic.cc | 4 +---
 gcc/system.h      | 6 ++++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
index 73324a728fe..fef11467b6f 100644
--- a/gcc/diagnostic.cc
+++ b/gcc/diagnostic.cc
@@ -1935,9 +1935,7 @@ fatal_error (location_t loc, const char *gmsgid, ...)
 }
 
 /* An internal consistency check has failed.  We make no attempt to
-   continue.  Note that unless there is debugging value to be had from
-   a more specific message, or some other good reason, you should use
-   abort () instead of calling this function directly.  */
+   continue.  */
 void
 internal_error (const char *gmsgid, ...)
 {
diff --git a/gcc/system.h b/gcc/system.h
index c25cd64366f..187763efcd6 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -770,8 +770,10 @@ extern int vsnprintf (char *, size_t, const char *, va_list);
 #endif
 #endif
 
-/* Redefine abort to report an internal error w/o coredump, and
-   reporting the location of the error in the source file.  */
+/* Redefine 'abort' to report an internal error w/o coredump, and
+   reporting the location of the error in the source file.
+   Instead of directly calling 'abort' or 'fancy_abort', GCC code
+   should normally call 'internal_error' with a specific message.  */
 extern void fancy_abort (const char *, int, const char *)
 					 ATTRIBUTE_NORETURN ATTRIBUTE_COLD;
 #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)
-- 
2.25.1

[PING] Advise to call 'internal_error' instead of 'abort' or 'fancy_abort'

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

Hi!

Ping.


Grüße
 Thomas


On 2022-05-10T16:03:12+0200, I wrote:
> Hi!
>
> On 2022-05-03T15:46:43+0200, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Tue, May 3, 2022 at 2:29 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>>> On 2022-05-03T12:53:50+0200, Richard Biener <richard.guenther@gmail.com> wrote:
>>> > On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
>>> >> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
>>> >> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>>> >> > +#if 0
>>> >> >        gcc_unreachable ();
>>> >> > +#else
>>> >> > +      /* ..., but due to bugs (PR100400), we may actually come here.
>>> >> > +        Reliably catch this, regardless of checking level.  */
>>> >> > +      abort ();
>>> >> > +#endif
>>> >> >
>>> >> > this doesn't look correct.  If you want a reliable diagnostic here please [...]
>>> >> > call internal_error () manually (the IL verifiers do this).
>>> >>
>>> >> Hmm, I feel I'm going in circles...  ;-)
>
>>> >> I first had this as 'internal_error', but then saw the following source
>>> >> code comment, 'gcc/diagnostic.cc':
>>> >>
>>> >>     /* An internal consistency check has failed.  We make no attempt to
>>> >>        continue.  Note that unless there is debugging value to be had from
>>> >>        a more specific message, or some other good reason, you should use
>>> >>        abort () instead of calling this function directly.  */
>>> >>     void
>>> >>     internal_error (const char *gmsgid, ...)
>>> >>     {
>>> >>
>>> >> Here, there's no "debugging value to be had from a more specific
>>> >> message", and I couldn't think of "some other good reason", so decided to
>>> >> "use abort () instead of calling this function directly".
>>> >
>>> > I think that is misguided.
>>>
>>> So that I know which one to fix/reconsider: does your "that" refer to the
>>> 'gcc/diagnostic.cc:internal_error' source code comment cited above, or my
>>> interpretation of it?
>>
>> The comment to "use abort ()".
>
> Does the attached
> "Advise to call 'internal_error' instead of 'abort' or 'fancy_abort'"
> capture what you had in mind?
>
> (This is, obviously, not updating any of the many 'abort' or even a few
> 'fancy_abort' calls that we currently have.)
>
>
> Grüße
>  Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Advise-to-call-internal_error-instead-of-abort-or-fa.patch --]
[-- Type: text/x-diff, Size: 1930 bytes --]

From a8017c7b5fa7b5e8210b6446acf7dd09989a7517 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 10 May 2022 15:56:08 +0200
Subject: [PATCH] Advise to call 'internal_error' instead of 'abort' or
 'fancy_abort'

	gcc/
	* diagnostic.cc: Don't advise to call 'abort' instead of
	'internal_error'.
	* system.h: Advise to call 'internal_error' instead of 'abort' or
	'fancy_abort'.

Suggested-by: Richard Biener <richard.guenther@gmail.com>
---
 gcc/diagnostic.cc | 4 +---
 gcc/system.h      | 6 ++++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
index 73324a728fe..fef11467b6f 100644
--- a/gcc/diagnostic.cc
+++ b/gcc/diagnostic.cc
@@ -1935,9 +1935,7 @@ fatal_error (location_t loc, const char *gmsgid, ...)
 }
 
 /* An internal consistency check has failed.  We make no attempt to
-   continue.  Note that unless there is debugging value to be had from
-   a more specific message, or some other good reason, you should use
-   abort () instead of calling this function directly.  */
+   continue.  */
 void
 internal_error (const char *gmsgid, ...)
 {
diff --git a/gcc/system.h b/gcc/system.h
index c25cd64366f..187763efcd6 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -770,8 +770,10 @@ extern int vsnprintf (char *, size_t, const char *, va_list);
 #endif
 #endif
 
-/* Redefine abort to report an internal error w/o coredump, and
-   reporting the location of the error in the source file.  */
+/* Redefine 'abort' to report an internal error w/o coredump, and
+   reporting the location of the error in the source file.
+   Instead of directly calling 'abort' or 'fancy_abort', GCC code
+   should normally call 'internal_error' with a specific message.  */
 extern void fancy_abort (const char *, int, const char *)
 					 ATTRIBUTE_NORETURN ATTRIBUTE_COLD;
 #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)
-- 
2.25.1

Re: [PING] Advise to call 'internal_error' instead of 'abort' or 'fancy_abort'

[Richard Biener]

On Tue, May 17, 2022 at 12:15 PM Thomas Schwinge
<thomas@codesourcery.com> wrote:
>
> Hi!
>
> Ping.

OK.

>
> Grüße
>  Thomas
>
>
> On 2022-05-10T16:03:12+0200, I wrote:
> > Hi!
> >
> > On 2022-05-03T15:46:43+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> >> On Tue, May 3, 2022 at 2:29 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >>> On 2022-05-03T12:53:50+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> >>> > On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >>> >> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> >>> >> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >>> >> > +#if 0
> >>> >> >        gcc_unreachable ();
> >>> >> > +#else
> >>> >> > +      /* ..., but due to bugs (PR100400), we may actually come here.
> >>> >> > +        Reliably catch this, regardless of checking level.  */
> >>> >> > +      abort ();
> >>> >> > +#endif
> >>> >> >
> >>> >> > this doesn't look correct.  If you want a reliable diagnostic here please [...]
> >>> >> > call internal_error () manually (the IL verifiers do this).
> >>> >>
> >>> >> Hmm, I feel I'm going in circles...  ;-)
> >
> >>> >> I first had this as 'internal_error', but then saw the following source
> >>> >> code comment, 'gcc/diagnostic.cc':
> >>> >>
> >>> >>     /* An internal consistency check has failed.  We make no attempt to
> >>> >>        continue.  Note that unless there is debugging value to be had from
> >>> >>        a more specific message, or some other good reason, you should use
> >>> >>        abort () instead of calling this function directly.  */
> >>> >>     void
> >>> >>     internal_error (const char *gmsgid, ...)
> >>> >>     {
> >>> >>
> >>> >> Here, there's no "debugging value to be had from a more specific
> >>> >> message", and I couldn't think of "some other good reason", so decided to
> >>> >> "use abort () instead of calling this function directly".
> >>> >
> >>> > I think that is misguided.
> >>>
> >>> So that I know which one to fix/reconsider: does your "that" refer to the
> >>> 'gcc/diagnostic.cc:internal_error' source code comment cited above, or my
> >>> interpretation of it?
> >>
> >> The comment to "use abort ()".
> >
> > Does the attached
> > "Advise to call 'internal_error' instead of 'abort' or 'fancy_abort'"
> > capture what you had in mind?
> >
> > (This is, obviously, not updating any of the many 'abort' or even a few
> > 'fancy_abort' calls that we currently have.)
> >
> >
> > Grüße
> >  Thomas
>
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955