PR69455

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

PR69455

Iain Sandoe
Hello Steve,

You made comment #9 in this PR that you can see that a small patch (two-liner) would fix this, but never posted the patch.

Given that this is a regression since 5.x, and we’re heading for the closure of 7 and a release on 8 in the not too distant future, it would be a great time to fix this if your patch would still work.

thanks
Iain

Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Steve Kargl
On Fri, Aug 30, 2019 at 09:22:12AM +0100, Iain Sandoe wrote:

> Hello Steve,
>
> You made comment #9 in this PR that you can see that a small
> patch (two-liner) would fix this, but never posted the patch.
>
> Given that this is a regression since 5.x, and we’re heading
> for the closure of 7 and a release on 8 in the not too distant
> future, it would be a great time to fix this if your patch
> would still work.
>

Unfortunately, that patch is long gone.  Likely, a victim
of reverting my tree while working on some other bug.  I
would need to (re)debug the issue.  

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Iain Sandoe-5
Hello Steve, Fortran folks,

Steve Kargl <[hidden email]> wrote:

> On Fri, Aug 30, 2019 at 09:22:12AM +0100, Iain Sandoe wrote:

>> You made comment #9 in this PR that you can see that a small
>> patch (two-liner) would fix this, but never posted the patch.
>>
>> Given that this is a regression since 5.x, and we’re heading
>> for the closure of 7 and a release on 8 in the not too distant
>> future, it would be a great time to fix this if your patch
>> would still work.
>
> Unfortunately, that patch is long gone.  Likely, a victim
> of reverting my tree while working on some other bug.  I
> would need to (re)debug the issue.  

My guess is that the schedule for closing the 7 branch is about to be
announced, so if there’s any chance of Fortran folks looking at this it
would be great - a shame to have another release branch closed with
no fix (and AFAIR, this is reproducible on Linux).

I did look at the analysis in the PR, but I don’t know my way around the
Fortran FE or language well enough to make sensible headway.

thanks
Iain

Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Steve Kargl
On Thu, Oct 17, 2019 at 07:48:33PM +0100, Iain Sandoe wrote:

> Hello Steve, Fortran folks,
>
> Steve Kargl <[hidden email]> wrote:
>
> > On Fri, Aug 30, 2019 at 09:22:12AM +0100, Iain Sandoe wrote:
>
> >> You made comment #9 in this PR that you can see that a small
> >> patch (two-liner) would fix this, but never posted the patch.
> >>
> >> Given that this is a regression since 5.x, and we’re heading
> >> for the closure of 7 and a release on 8 in the not too distant
> >> future, it would be a great time to fix this if your patch
> >> would still work.
> >
> > Unfortunately, that patch is long gone.  Likely, a victim
> > of reverting my tree while working on some other bug.  I
> > would need to (re)debug the issue.  
>
> My guess is that the schedule for closing the 7 branch is about to be
> announced, so if there’s any chance of Fortran folks looking at this it
> would be great - a shame to have another release branch closed with
> no fix (and AFAIR, this is reproducible on Linux).
>
> I did look at the analysis in the PR, but I don’t know my way around the
> Fortran FE or language well enough to make sensible headway.
>
This patch is against trunk.  It's unfortunately that very few others
have an interest in fixing gfortran.

2019-10-17  Steven G. Kargl  <[hidden email]>

        PR fortran/69455
        * trans-decl.c (generate_local_decl): Avoid misconstrcuted
        ISO_C_BINDING module in BLOCK construct.

2019-10-17  Steven G. Kargl  <[hidden email]>

        PR fortran/69455
        * gfortran.dg/pr69455.f90: New test.

--
Steve

pr69455.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Iain Sandoe-5
Thanks Steve,

Steve Kargl <[hidden email]> wrote:

> On Thu, Oct 17, 2019 at 07:48:33PM +0100, Iain Sandoe wrote:
>> Hello Steve, Fortran folks,
>>
>> Steve Kargl <[hidden email]> wrote:
>>
>>> On Fri, Aug 30, 2019 at 09:22:12AM +0100, Iain Sandoe wrote:
>>
>>>> You made comment #9 in this PR that you can see that a small
>>>> patch (two-liner) would fix this, but never posted the patch.
>>>>
>>>> Given that this is a regression since 5.x, and we’re heading
>>>> for the closure of 7 and a release on 8 in the not too distant
>>>> future, it would be a great time to fix this if your patch
>>>> would still work.
>>>
>>> Unfortunately, that patch is long gone.  Likely, a victim
>>> of reverting my tree while working on some other bug.  I
>>> would need to (re)debug the issue.  
>>
>> My guess is that the schedule for closing the 7 branch is about to be
>> announced, so if there’s any chance of Fortran folks looking at this it
>> would be great - a shame to have another release branch closed with
>> no fix (and AFAIR, this is reproducible on Linux).
>>
>> I did look at the analysis in the PR, but I don’t know my way around the
>> Fortran FE or language well enough to make sensible headway.
>
> This patch is against trunk.  

This works in my testing with a slightly amended testcase (and with the examples
given in the PR).

> It's unfortunately that very few others
> have an interest in fixing gfortran.

This is a problem with all parts of the compiler mainlined on a voluntary basis;
we are a very small resource, spread mighty thin.

> 2019-10-17  Steven G. Kargl  <[hidden email]>
>
> PR fortran/69455
> * trans-decl.c (generate_local_decl): Avoid misconstrcuted

miscontructed.

> ISO_C_BINDING module in BLOCK construct.
>
> 2019-10-17  Steven G. Kargl  <[hidden email]>
>
> PR fortran/69455
> * gfortran.dg/pr69455.f90: New test.
>

I think the testcase needs a small amendment to work for m32 multilibs,
ik in the second block needs to be c_long_long to have size 8 on 32b
platforms.

! { dg-do run }
program foo
   block
      use, intrinsic :: iso_c_binding, only: wp => c_float, ik => c_int
      if (ik /= 4) stop 1
      if (wp /= 4) stop 2
   end block
   block
      use, intrinsic :: iso_c_binding, only: wp => c_double, ik => c_long_long
      if (ik /= 8) stop 3
      if (wp /= 8) stop 4
   end block
end program foo


Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Steve Kargl
On Fri, Oct 18, 2019 at 12:40:03AM +0100, Iain Sandoe wrote:
> >
> > PR fortran/69455
> > * trans-decl.c (generate_local_decl): Avoid misconstrcuted
>
> miscontructed.
>

Thanks.  Typing too quickly.

> > ISO_C_BINDING module in BLOCK construct.
> >
> > 2019-10-17  Steven G. Kargl  <[hidden email]>
> >
> > PR fortran/69455
> > * gfortran.dg/pr69455.f90: New test.
> >
>
> I think the testcase needs a small amendment to work for m32 multilibs,
> ik in the second block needs to be c_long_long to have size 8 on 32b
> platforms.

Hmmm.  I'll probably use C_INT64_T.  Hopefully, all gcc targets
have a 64-bit integer.

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: PR69455

dhumieres.dominique
In reply to this post by Iain Sandoe
The patch fixes the problem for iso_c_binding but not for
ISO_FORTRAN_ENV (pr69455#c1).

The later is fixed with

--- ../_clean/gcc/fortran/trans-decl.c 2019-10-11 11:28:20.000000000
+0200
+++ gcc/fortran/trans-decl.c 2019-10-18 12:52:46.000000000 +0200
@@ -5987,7 +5987,12 @@ generate_local_decl (gfc_symbol * sym)

        if (sym->ns && sym->ns->construct_entities)
  {
-  if (sym->attr.referenced)
+  /* Construction of the ISO_C_BINDING module within a BLOCK
+     construct where ONLY and RENAMED entities are included in
+     seems to be bogus.  See PR 69455.  */
+  if (sym->attr.referenced
+      && sym->from_intmod != INTMOD_ISO_C_BINDING
+      && sym->from_intmod != INTMOD_ISO_FORTRAN_ENV)
     gfc_get_symbol_decl (sym);
   sym->mark = 1;
  }

Note that I did not updated the comment nor the test.

Thanks for the patch.

Dominique
Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Steve Kargl
On Fri, Oct 18, 2019 at 01:01:18PM +0200, [hidden email] wrote:

> The patch fixes the problem for iso_c_binding but not for
> ISO_FORTRAN_ENV (pr69455#c1).
>
> The later is fixed with
>
> --- ../_clean/gcc/fortran/trans-decl.c 2019-10-11 11:28:20.000000000
> +0200
> +++ gcc/fortran/trans-decl.c 2019-10-18 12:52:46.000000000 +0200
> @@ -5987,7 +5987,12 @@ generate_local_decl (gfc_symbol * sym)
>
>         if (sym->ns && sym->ns->construct_entities)
>   {
> -  if (sym->attr.referenced)
> +  /* Construction of the ISO_C_BINDING module within a BLOCK
> +     construct where ONLY and RENAMED entities are included in
> +     seems to be bogus.  See PR 69455.  */
> +  if (sym->attr.referenced
> +      && sym->from_intmod != INTMOD_ISO_C_BINDING
> +      && sym->from_intmod != INTMOD_ISO_FORTRAN_ENV)
>      gfc_get_symbol_decl (sym);
>    sym->mark = 1;
>   }
>
> Note that I did not updated the comment nor the test.
>

If you don't have test code, then how do you know that your
modification fixes anything?  Without a testcase, I'll be
committing my original patch.

--
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow
Reply | Threaded
Open this post in threaded view
|

Re: PR69455

dhumieres.dominique
Le 2019-10-18 15:56, Steve Kargl a écrit :

> On Fri, Oct 18, 2019 at 01:01:18PM +0200, [hidden email]
> wrote:
>> The patch fixes the problem for iso_c_binding but not for
>> ISO_FORTRAN_ENV (pr69455#c1).
>>
>> The later is fixed with
>>
>> --- ../_clean/gcc/fortran/trans-decl.c 2019-10-11 11:28:20.000000000
>> +0200
>> +++ gcc/fortran/trans-decl.c 2019-10-18 12:52:46.000000000 +0200
>> @@ -5987,7 +5987,12 @@ generate_local_decl (gfc_symbol * sym)
>>
>>         if (sym->ns && sym->ns->construct_entities)
>>   {
>> -  if (sym->attr.referenced)
>> +  /* Construction of the ISO_C_BINDING module within a BLOCK
>> +     construct where ONLY and RENAMED entities are included in
>> +     seems to be bogus.  See PR 69455.  */
>> +  if (sym->attr.referenced
>> +      && sym->from_intmod != INTMOD_ISO_C_BINDING
>> +      && sym->from_intmod != INTMOD_ISO_FORTRAN_ENV)
>>      gfc_get_symbol_decl (sym);
>>    sym->mark = 1;
>>   }
>>
>> Note that I did not updated the comment nor the test.
>>
>
> If you don't have test code, then how do you know that your
> modification fixes anything?  Without a testcase, I'll be
> committing my original patch.

The test case is in pr69455 comment 1:

block
   use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL32, ik => INT32
   print *, ik, wp
end block

block
   use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL64, ik => INT64
   print *, ik, wp
end block
end

Dominique

Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Steve Kargl
On Fri, Oct 18, 2019 at 04:03:37PM +0200, [hidden email] wrote:

> The test case is in pr69455 comment 1:
>
> block
>    use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL32, ik => INT32
>    print *, ik, wp
> end block
>
> block
>    use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL64, ik => INT64
>    print *, ik, wp
> end block
> end
>

This won't work with dejagnu.

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Iain Sandoe
Steve Kargl <[hidden email]> wrote:

> On Fri, Oct 18, 2019 at 04:03:37PM +0200, [hidden email] wrote:
>> The test case is in pr69455 comment 1:
>>
>> block
>>   use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL32, ik => INT32
>>   print *, ik, wp
>> end block
>>
>> block
>>   use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL64, ik => INT64
>>   print *, ik, wp
>> end block
>> end
>
> This won't work with dejagnu.

something like this, perhaps (I regret my Fortran skills are in the f77 era):

! { dg-do run }
program foo
   block
      use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL32, ik => INT32
      if (ik /= 4) stop 1
      if (wp /= 4) stop 2
   end block
   block
      use, intrinsic :: ISO_FORTRAN_ENV, only: wp => REAL64, ik => INT64
      if (ik /= 8) stop 3
      if (wp /= 8) stop 4
   end block
end program foo

Reply | Threaded
Open this post in threaded view
|

Re: PR69455

Steve Kargl
On Fri, Oct 18, 2019 at 05:17:38PM +0100, Iain Sandoe wrote:
>
> something like this, perhaps (I regret my Fortran skills are in the f77 era):
>

If you know/knew F77 and have some working knowledge of C/C++ and
you want to see where modern Fortran sits, I recommend Modern Fortran
Explained iby Metcalf et al.  You can probably read it in a day.

For the record, here is the patch (see attached) committed to all
open branches.

With this commit, I'll be taking a long break from looking at
any gfortran bugs.

2019-10-18  Steven G. Kargl  <[hidden email]>

        PR fortran/69455
        * trans-decl.c (generate_local_decl): Avoid misconstructed
        intrinsic modules in a BLOCK construct.

2019-10-18  Steven G. Kargl  <[hidden email]>

        PR fortran/69455
        * gfortran.dg/pr69455_1.f90: New test.
        * gfortran.dg/pr69455_2.f90: Ditto.

--
Steve
25.16%

new_69455.diff (2K) Download Attachment