public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Harald Anlauf <anlauf@gmx.de>
To: Paul Richard Thomas <paul.richard.thomas@gmail.com>,
	"fortran@gcc.gnu.org" <fortran@gcc.gnu.org>,
	gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [Patch, fortran] PRs 105152, 100193, 87946, 103389, 104429 and 82774
Date: Sun, 23 Apr 2023 23:48:03 +0200	[thread overview]
Message-ID: <27ee85f7-f52b-9b41-377a-9d025ddadbee@gmx.de> (raw)
In-Reply-To: <CAGkQGiLQB=optugqew-T1a5bn=DA=XsN7fYT=hT4fY4UtpU7+Q@mail.gmail.com>

Hi Paul,

Am 22.04.23 um 10:32 schrieb Paul Richard Thomas via Gcc-patches:
> Hi All,
>
> As usual, I received a string of emails on retargeting for PRs for which I
> was either responsible or was on the cc list. This time I decided to take a
> look at them all, in order to reward the tireless efforts of Richi, Jakub
> and Martin with some attention at least.
>
> I have fixed the PRs in the title line: See the attached changelog, patch
> and testcases.
>
> OK for 14-branch?

the patch looks essentially good to me.

Can you please have a look at testcase pr100193.f90, which fails
for me because the module file is not generated and there is no
corresponding dg-pattern:

FAIL: gfortran.dg/pr100193.f90   -O  (test for excess errors)

                 === gfortran Summary ===

# of expected passes            1
# of unexpected failures        1

You could either simply omit the main program or add a pattern.
(The shortened testcase would still fail w/o the patch.)

> Of the others:
> PR100815 - fixed already for 12-branch on. Martin located the fix from
> Tobias, for which thanks. It's quite large but has stood the test of time.
> Should I backport to 11-branch?
> PR103366 - fixed on 12-branch on. I closed it.
> PR103715 - might be fixed but the report is for gcc with checking enabled.
> I will give that a go.
> PR103716 - a gimple problem with assumed shape characters. A TODO.
> PR103931 - I couldn't reproduce the bug, which involves 'ambiguous c_ptr'.
> To judge by the comments, it seems that this bug is a bit elusive.
> PR65381 - Seems to be fixed for 12-branch on
> PR82064 - Seems to be fixed.
> PR83209 - Coarray allocation - seems to be fixed.
> PR84244 - Coarray segfault. I have no acquaintance with the inner works of
> coarrays and so don't think that I can fix this one.
> PR87674 - Segfault in runtime with non-overridable proc-pointer. A TODO.
> PR96087 - A module procedure problem. A TODO.
>
> I have dejagnu-ified testcases for the already fixed PRs ready to go.
> Should these be committed or do we assume that the fixes already provided
> adequate tests?

I think this depends.  A testcase that is "sufficiently orthogonal" to
those in the testsuite may be worth to be added.  Otherwise randomly
adding testcases might just increase the runtime for regression testing,
which could be counter-productive for the development process.  So
better really decide on a case-by-case basis?

(Of course this is only my opinion, and other may have a different view
upon this.)

I checked PR100815.  The testcase in comment#0 appears to work for me
for all open branches (10 to 14).  The commit that supposedly fixed
the issue applies to 12-branch and newer.  Either there is something
else in 11-branch which fixed it in a different way, or the bisecting
unfortunately pointed to the wrong commit.  And since there is no
traceback information in the PR, I am simply confused.
So do you think this testcase improves coverage and thus adds value?

PR103715: with valgrind I get invalid reads, so I guess there is
something lurking here and it only appears to be fixed.

PR103931: it is indeed a bit elusive, but very sensitive to code
changes.  Also Bernhard had a look at it.  Given that there are
a couple of bugs related to module reading, and rename-on-use,
I'd recommend to leave that open for further analysis.

PR65381: works for me even on 11-branch.  I think this looks very
much like a duplicate of a PR that was fixed by Tobias.  Still
fails on 10-branch, but might not be worth fixing there.  Simply
close it as 10-only?

PR103716: I think that one is interesting, as there are a couple
of PRs involving inquiry functions.

> Regards
>
> Paul

Cheers,
Harald


WARNING: multiple messages have this Message-ID
From: Harald Anlauf <anlauf@gmx.de>
To: gcc-patches@gcc.gnu.org
Cc: fortran@gcc.gnu.org
Subject: Re: [Patch, fortran] PRs 105152, 100193, 87946, 103389, 104429 and 82774
Date: Sun, 23 Apr 2023 23:48:03 +0200	[thread overview]
Message-ID: <27ee85f7-f52b-9b41-377a-9d025ddadbee@gmx.de> (raw)
Message-ID: <20230423214803.bi071wu0wLQaIYPSjC8tlU1-yQ8un6CXRjypwRLu8jg@z> (raw)
In-Reply-To: <CAGkQGiLQB=optugqew-T1a5bn=DA=XsN7fYT=hT4fY4UtpU7+Q@mail.gmail.com>

Hi Paul,

Am 22.04.23 um 10:32 schrieb Paul Richard Thomas via Gcc-patches:
> Hi All,
> 
> As usual, I received a string of emails on retargeting for PRs for which I
> was either responsible or was on the cc list. This time I decided to take a
> look at them all, in order to reward the tireless efforts of Richi, Jakub
> and Martin with some attention at least.
> 
> I have fixed the PRs in the title line: See the attached changelog, patch
> and testcases.
> 
> OK for 14-branch?

the patch looks essentially good to me.

Can you please have a look at testcase pr100193.f90, which fails
for me because the module file is not generated and there is no
corresponding dg-pattern:

FAIL: gfortran.dg/pr100193.f90   -O  (test for excess errors)

                 === gfortran Summary ===

# of expected passes            1
# of unexpected failures        1

You could either simply omit the main program or add a pattern.
(The shortened testcase would still fail w/o the patch.)

> Of the others:
> PR100815 - fixed already for 12-branch on. Martin located the fix from
> Tobias, for which thanks. It's quite large but has stood the test of time.
> Should I backport to 11-branch?
> PR103366 - fixed on 12-branch on. I closed it.
> PR103715 - might be fixed but the report is for gcc with checking enabled.
> I will give that a go.
> PR103716 - a gimple problem with assumed shape characters. A TODO.
> PR103931 - I couldn't reproduce the bug, which involves 'ambiguous c_ptr'.
> To judge by the comments, it seems that this bug is a bit elusive.
> PR65381 - Seems to be fixed for 12-branch on
> PR82064 - Seems to be fixed.
> PR83209 - Coarray allocation - seems to be fixed.
> PR84244 - Coarray segfault. I have no acquaintance with the inner works of
> coarrays and so don't think that I can fix this one.
> PR87674 - Segfault in runtime with non-overridable proc-pointer. A TODO.
> PR96087 - A module procedure problem. A TODO.
> 
> I have dejagnu-ified testcases for the already fixed PRs ready to go.
> Should these be committed or do we assume that the fixes already provided
> adequate tests?

I think this depends.  A testcase that is "sufficiently orthogonal" to
those in the testsuite may be worth to be added.  Otherwise randomly
adding testcases might just increase the runtime for regression testing,
which could be counter-productive for the development process.  So
better really decide on a case-by-case basis?

(Of course this is only my opinion, and other may have a different view
upon this.)

I checked PR100815.  The testcase in comment#0 appears to work for me
for all open branches (10 to 14).  The commit that supposedly fixed
the issue applies to 12-branch and newer.  Either there is something
else in 11-branch which fixed it in a different way, or the bisecting
unfortunately pointed to the wrong commit.  And since there is no
traceback information in the PR, I am simply confused.
So do you think this testcase improves coverage and thus adds value?

PR103715: with valgrind I get invalid reads, so I guess there is
something lurking here and it only appears to be fixed.

PR103931: it is indeed a bit elusive, but very sensitive to code
changes.  Also Bernhard had a look at it.  Given that there are
a couple of bugs related to module reading, and rename-on-use,
I'd recommend to leave that open for further analysis.

PR65381: works for me even on 11-branch.  I think this looks very
much like a duplicate of a PR that was fixed by Tobias.  Still
fails on 10-branch, but might not be worth fixing there.  Simply
close it as 10-only?

PR103716: I think that one is interesting, as there are a couple
of PRs involving inquiry functions.

> Regards
> 
> Paul

Cheers,
Harald



  reply	other threads:[~2023-04-23 21:48 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-22  8:32 Paul Richard Thomas
2023-04-23 21:48 ` Harald Anlauf [this message]
2023-04-23 21:48   ` Harald Anlauf
2023-04-24 16:41   ` Bernhard Reutner-Fischer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=27ee85f7-f52b-9b41-377a-9d025ddadbee@gmx.de \
    --to=anlauf@gmx.de \
    --cc=fortran@gcc.gnu.org \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=paul.richard.thomas@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).