Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

variance_correction: small issue with debug mode of gfortran 7 #171

Closed
wants to merge 2 commits into from

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Apr 11, 2020

Gfortran 7 with debug options (but not Gfortran 9) failed on, e.g.,

i32 = d

So, I replaced the line by:

allocate(i32, source = int(d)

@ivan-pi
Copy link
Member

ivan-pi commented Apr 13, 2020

In #169 (comment) @milancurcic reckoned we should avoid workarounds for what is supposed to be valid code.

At the same time, it would be a nuisance to bring in preprocessor macros to differentiate between compiler versions and avoid spurious warnings. Then again, your change is valid Fortran code, just a bit more explicit, so I suppose this is the easier solution in the short-term.

@milancurcic
Copy link
Member

milancurcic commented Apr 13, 2020

@jvdp1 What exactly fails here for gfortran 7?

If gfortran 7 didn't support reallocation on assignment (I thought it did), I don't see a good reason to babysit the compiler here--the feature is reasonably mature (F2003). I'd rather recommend upgrading the compiler to a later release.

However, if the feature is supported but has a bug, we should file a bug report rather than inserting a workaround into stdlib.

We don't need to support all earlier releases of compilers. We just need to make a pragmatic decision what we want and don't want to support. It's fine for various things to be unsupported or broken until somebody complains.

Of course, this is all just my opinion. I'm curious to hear what others think, @certik @nncarlson @nshaffer

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 13, 2020

@jvdp1 What exactly fails here for gfortran 7?

Here are more details:

$ gfortran --version
GNU Fortran (GCC) 7.2.0
Copyright © 2017 Free Software Foundation, Inc.

Additional options used for compilation:

-fimplicit-none -Wall -fcheck=all -fbacktrace

Issue with test_far (and similarly for test_varn):

At line 60 of file /home/WUR/vande018/stdlib/src/tests/stats/test_var.f90
Fortran runtime error: Array bound mismatch for dimension 1 of array 's' (1/4)

Error termination. Backtrace:
#0  0x402096 in ???
#1  0x43ad54 in ???
#2  0x2aaaab81b544 in ???
#3  0x401538 in ???
#4  0xffffffffffffffff in ???
<end of output>

Correspinding line 60 in test_var.f90:

s = d

Note: there is no problem with gfortran 7 and without debug options.

@certik
Copy link
Member

certik commented Apr 13, 2020

It's this automatic reallocation of LHS, which used to be buggy in gfortran in various scenarios. We either require GFortran > 7, or we fix it by providing a workaround (one way or another) that works with GFortran 7.

Yes, in general valid code should work, but in reality many valid Fortran codes do not work with every compiler, and so providing workarounds for our supported compilers is necessary.

So it's just about deciding whether we want to support GFortran 7 or not.

@nncarlson
Copy link
Member

I agree with @certik. It's unfortunate but necessary.

I'd also add that the set of supported compilers will evolve in time, with some older compilers being dropped while new ones are added. In this regard it will be important to somehow tie individual workarounds to specific compiler versions and not just to the compiler (e.g., gfortran). That will allow workarounds for unsupported compilers to be easily removed from the code base. Otherwise the code base will become more and more littered with workarounds as time goes on.

@nncarlson
Copy link
Member

However, if the feature is supported but has a bug, we should file a bug report rather than inserting a workaround into stdlib.

Unless one is dealing with the NAG compiler where they almost always fix bugs within a couple of weeks, this is not an either-or proposition. Definitely always file a bug report. But then one will also need to implement a workaround if one wants to go ahead using the buggy feature. Every other compiler I've dealt with takes many months, sometimes years, to fix reported bugs.

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 13, 2020

Thank you for your feedbacks.

The same issue seems to be also present in gfortran 8!
I will report the bug.

For this PR, I let you decide what is best for the current and future situations.

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 14, 2020

Bug reported here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94585

@certik
Copy link
Member

certik commented Apr 14, 2020 via email

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 17, 2020

Because the GFortran 7-branch is closed, I am also ok to drop GFortran 7 support. If more people think that too, this PR can be closed without being merged.

@certik
Copy link
Member

certik commented Apr 17, 2020

@milancurcic any comments?

Btw I think you have some unrelated change in this PR that can go in (you should post it as a separate PR).

@milancurcic
Copy link
Member

I agree with not supporting GFortran 7 with this workaround.

@jvdp1
Copy link
Member Author

jvdp1 commented Apr 17, 2020

Btw I think you have some unrelated change in this PR that can go in (you should post it as a separate PR).

I will open another PR for these!

@jvdp1 jvdp1 mentioned this pull request May 16, 2020
@jvdp1
Copy link
Member Author

jvdp1 commented May 30, 2020

Btw I think you have some unrelated change in this PR that can go in (you should post it as a separate PR).

I will open another PR for these!

Done with PR #173

@jvdp1
Copy link
Member Author

jvdp1 commented May 30, 2020

I close this PR because it seems there is an agreement to not support GFortran 7 with this workaround,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants