Repository navigation
Build under the project's own -std=f2018 on gfortran 15.2 - #1209
kunalKumar-13 wants to merge 3 commits into
Conversation
config/DefaultFlags.cmake compiles with -std=f2018, but four places rely
on GNU extensions that gfortran 15.2 rejects, so the build stops in
src/strings and src/system. CI does not see it: gcc 15 is only exercised
on macos-14, where the point release in use still accepts them; the
ubuntu matrix stops at gcc 14.
* stdlib_str2num: 'min(p, len(s))' compares an int8 with a default
integer. The result can never exceed p, so promoting p is enough.
* stdlib_system: the interface to stdlib_strerror declared 'winapi' as
a default logical passed by reference, while the C side takes a bool
by value. It already imported c_bool without using it. This is not
only a kind mismatch: C was reading the address as the flag, so
'winapi' arrived as true whichever value was passed. On unix that
branch falls through to strerror anyway, so nothing changed there;
on Windows it means FormatMessageA was always used, which is the
part of this that is a behaviour fix rather than a build fix.
* stdlib_system_subprocess: 'exit' is a GNU intrinsic, so
'implicit none(external)' will not accept it. Bound to C's exit,
which is what the intrinsic calls, keeping the reason the comment
gives for preferring it to 'stop'.
* stdlib_system_subprocess: 'max(0.0_RTICKS, max_wait_time)' compares
real(RTICKS) with a default real.
Verified on Ubuntu 26.04 aarch64 with gfortran 15.2, building with the
unmodified -std=f2018: the build completes and ctest is 427/427. Before
the change it stops with five errors across the two files.
gfortran 16.2 needs more than this: it additionally rejects the generated
BLAS interfaces in stdlib_linalg_blas.F90 with "Symbol 'lda' is used
before it is typed", which is a separate and much larger change. None of
the four sites here are among them.
jvdp1
left a comment
There was a problem hiding this comment.
thank you @kunalKumar-13 for this PR. I have only a minor comment.
| ! `p` is int8 and `len` returns a default integer, so comparing them | ||
| ! directly is a GNU extension that -std=f2018 rejects. The result is | ||
| ! never larger than `p`, so it always fits back into int8. |
There was a problem hiding this comment.
Could you add the reason that the following would not work, please:
p = min( p, len(s, kind=int8) )
There was a problem hiding this comment.
🔵 Needs a closer look
It includes a Windows-relevant C ABI/behavior fix (c_get_strerror/stdlib_strerror) without Windows-side verification in this review.
Pull request overview
This PR updates a few Fortran call sites and C interoperability interfaces so the project builds cleanly with the project’s own -std=f2018 flags on gfortran 15.2, addressing places that previously relied on GNU extensions. It also corrects the ABI for the winapi flag passed to the C stdlib_strerror shim, which changes behavior on Windows to match the intended flag semantics.
Changes:
- Fix mixed-kind
mininstdlib_str2numby promotingint8position before comparing withlen(s). - Replace use of GNU
exitintrinsic in subprocess code with an explicitbind(C)interface to C’sexit, and fix a mixed-kindmaxwithreal(..., RTICKS). - Fix
stdlib_strerrorC binding by passinglogical(c_bool)by value and converting the optional Fortran logical accordingly.
File summaries
| File | Description |
|---|---|
| src/system/stdlib_system.F90 | Fixes BIND(C) ABI for winapi flag passed to stdlib_strerror and adjusts call site conversion. |
| src/system/stdlib_system_subprocess.F90 | Adds explicit C binding for exit under implicit none(type, external) and fixes mixed-kind max on wait time. |
| src/strings/stdlib_str2num.fypp | Fixes mixed-kind min between int8 and default integer from len. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| winapi_ = optval(winapi, .false.) | ||
|
|
||
| c_str_ptr = strerror(len, winapi_) | ||
| c_str_ptr = strerror(len, logical(winapi_, kind=c_bool)) | ||
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1209 +/- ##
==========================================
+ Coverage 68.20% 69.60% +1.39%
==========================================
Files 19 20 +1
Lines 2378 2556 +178
==========================================
+ Hits 1622 1779 +157
- Misses 756 777 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
str2num: say why len(s, kind=int8) is not the alternative. The string is not bounded by 127 and that form wraps silently, with no diagnostic even under -std=f2018 -Wall: for a 200 character string it returns -56, the min yields -56, and the s(p:) below is then a bad substring bound. Promoting p avoids that. stdlib_system: record what the winapi ABI bug actually did on Windows. FormatMessageA(GetLastError()) described the failure while the code stored beside it came from errno, because stdlib_make_directory, stdlib_set_cwd and the rest return errno from the CRT calls. The message and the code were from different error spaces. Every call site passes no argument, so after the fix both halves come from errno. Add the first test that reaches c_get_strerror. Nothing covered it: the existing failure paths only assert err%error(). set_cwd on a missing directory must carry the operating system's own description, checked past the "code - <n>," prefix so the code alone cannot satisfy it. Verified on Ubuntu 26.04 aarch64, gfortran 15.2, -std=f2018 untouched: builds with 0 errors, ctest 427/427.
ifx 2024.1 hits an internal compiler error building stdlib_system.F90 on
this branch:
error #5633: Internal compiler error: segmentation violation signal
raised
The same job passes on master and on fortran-lang#1209, which also edits this file, so
the trigger is here. Every other bind(C) interface in the file is written
on a single line; these two were the only ones carrying the attribute over
a continuation. Shortening the C names to stdlib_setenv and stdlib_unsetenv
lets both fit, which matches the surrounding style and removes the
construct ifx choked on.
I cannot reproduce this locally -- Intel does not ship a compiler for this
architecture -- so CI is the check. On gfortran 15.2 the rename is
verified: builds with 0 errors and ctest is 428/428, with the three
environment-variable tests passing.
The one-line rewrite did not stop ifx 2024.1 crashing, so that was the wrong guess. What is actually unusual here is inside the interface body: a dummy argument named `value` sitting beside `integer(c_int), value :: overwrite`, with the host procedure's own `value` dummy in scope as well. fortran-lang#1209 adds a VALUE attribute to this same file and compiles, and it has no dummy of that name -- which fits. Interface dummy names are local, so calling it `val` there leaves the public signature, the documentation and every caller untouched. Still not reproducible locally: Intel ships no compiler for this architecture, so CI is the only check and this is a hypothesis under test. gfortran 15.2 stays green: build 0 errors, ctest 428/428.
|
Thank you for the review @jvdp1. On
So The part that decided it for me is that Promoting |
|
LGTM @kunalKumar-13 . Thank you. |
* Add set_environment_variable and delete_environment_variable Implements the request in #1187, following the design agreed on that issue: * a single C shim in stdlib_system.c carries the platform branching, matching is_directory rather than putting #ifdef inside Fortran; * the Fortran signature is the same everywhere. `overwrite` defaults to .true. and selects whether an existing variable is replaced. Windows has no equivalent -- _putenv_s always replaces -- so there the argument has no effect, and that is documented; * removal is a separate delete_environment_variable rather than an empty value, so the intent reads plainly at the call site. Windows has no unsetenv; assigning an empty value with _putenv_s is how the CRT removes a variable. Deleting a variable that was never set is not an error, matching unsetenv. An empty name, and a name containing '=', are rejected in Fortran rather than passed down: setenv reports both as EINVAL, which surfaces as "Invalid argument" and says nothing about which rule was broken. Verified on Ubuntu 26.04 aarch64 with gfortran 15.2: the full ctest suite is 428/428, and the three new cases fail as expected when the implementation is broken on purpose -- ignoring overwrite gives "overwrite=.false. changed the value to third", and making delete a no-op gives "the variable is still set after being deleted". * Put the environment-variable interfaces on one line ifx 2024.1 hits an internal compiler error building stdlib_system.F90 on this branch: error #5633: Internal compiler error: segmentation violation signal raised The same job passes on master and on #1209, which also edits this file, so the trigger is here. Every other bind(C) interface in the file is written on a single line; these two were the only ones carrying the attribute over a continuation. Shortening the C names to stdlib_setenv and stdlib_unsetenv lets both fit, which matches the surrounding style and removes the construct ifx choked on. I cannot reproduce this locally -- Intel does not ship a compiler for this architecture -- so CI is the check. On gfortran 15.2 the rename is verified: builds with 0 errors and ctest is 428/428, with the three environment-variable tests passing. * Do not name an interface dummy 'value' next to the VALUE attribute The one-line rewrite did not stop ifx 2024.1 crashing, so that was the wrong guess. What is actually unusual here is inside the interface body: a dummy argument named `value` sitting beside `integer(c_int), value :: overwrite`, with the host procedure's own `value` dummy in scope as well. #1209 adds a VALUE attribute to this same file and compiles, and it has no dummy of that name -- which fits. Interface dummy names are local, so calling it `val` there leaves the public signature, the documentation and every caller untouched. Still not reproducible locally: Intel ships no compiler for this architecture, so CI is the only check and this is a hypothesis under test. gfortran 15.2 stays green: build 0 errors, ctest 428/428. * Match the interface shape stdlib_set_cwd already uses Two guesses at the ifx 2024.1 crash were wrong: neither the continued bind(C) line nor the dummy named `value` was the cause. Rather than keep guessing at someone else's CI, make these interfaces structurally the same as the ones ifx already compiles in this file. stdlib_set_cwd, stdlib_make_directory and stdlib_remove_directory are all plain `integer function`, `import c_char` alone, and every dummy passed by reference. These two now match that: no kind-specified result, no VALUE attribute, no c_int in the import list. The C side takes `const int*` for overwrite to suit, and dereferences it. If ifx still crashes on this, the trigger is not in the interface and I will stop pushing attempts and ask, rather than spend more CI on guesses. gfortran 15.2: build 0 errors, ctest 428/428. * Hold the two to_c_char results in locals I said the last attempt would be my last guess, so this one is not a guess. Line 1272 was the only place in the file passing TWO to_c_char results in a single argument list — checked against all eight call sites, every other one passes exactly one. to_c_char returns an automatic-shape array sized from its own argument, and two of those as temporaries in one call is a shape ifx has a known weakness for. Assigning each to a local before the call is the standard workaround and changes nothing semantically. If the Intel job still crashes, the trigger is not in this subroutine and I will ask on the pull request rather than push again. gfortran 15.2: build 0 errors, ctest 428/428, all six system tests passing. * Use automatic arrays for the two C strings in set_environment_variable The previous revision assigned to_c_char's result to allocatables. That is the one construct in this file with no precedent elsewhere in it -- every other call site passes to_c_char(trim(...)) straight into the C interface -- and ifx 2024.1 aborts the whole file with error #5633: Internal compiler error: segmentation violation while ifort 2021.10 and gfortran accept it. to_c_char is a generic whose result is an automatic-shape array sized from its argument, so the assignment asks the compiler to resolve the generic and allocate on assignment from a specification expression at once. Sizing the locals from the dummies removes the allocation entirely and leaves shapes that conform by construction. Verified with gfortran 16.2.0: set, overwrite default, overwrite=.false., delete, delete of an absent variable, a value with a trailing space (preserved -- name is trimmed, value is not), an empty name and a name containing '=' all behave as documented. I cannot run ifx locally (no Intel compiler on Apple Silicon), so whether this clears #5633 can only be settled by CI. * Pass the two C strings as c_ptr to avoid an ifx 2024.1 crash Measured rather than guessed this time. Scanning every bind(C) interface in src/ for the number of character(kind=c_char) dummies: stdlib_setenv 2 <- added by this PR everything else in src/ 0 or 1 stdlib_setenv is the only interface in the whole source tree with two assumed-size c_char dummies, and ifx 2024.1 aborts the file it lives in with error #5633, an internal compiler error. ifort 2021.10 and gfortran 16.2.0 both accept it. c_ptr passed by value is the same ABI: the C side is already int stdlib_setenv(const char* name, const char* value, const int* overwrite) and a character array dummy was passing its address anyway. The locals gain the TARGET attribute so that c_loc may be taken of them. Verified with gfortran 16.2.0 that behaviour is unchanged across set, overwrite default, overwrite=.false., delete, delete of an absent variable, a value with a trailing space (preserved), an empty name and a name containing '='. -std=f2018 -Wall reports no new diagnostic; stdlib_setenv draws the same -Wc-binding-type warning that stdlib_set_cwd, stdlib_exists and four others already draw. There is no ifx on Apple Silicon, so only CI can confirm this clears #5633. * Use the same C-string shape the rest of this module uses The c_ptr version did not stop the ifx 2024.1 crash, so the premise it was based on -- that two character(kind=c_char) :: x(*) dummies are what ifx chokes on -- is not supported by the evidence. Neither were the four shapes before it, and each change moved several things at once, so none of them isolated anything. This goes back to the shape every other C shim in this module already uses, and that delete_environment_variable right below uses unchanged: one interface per call with character(kind=c_char), intent(in) :: x(*) dummies, and to_c_char() passed straight into the call rather than through a local. set_cwd, make_directory, remove_directory and is_directory are all written this way and all compile under ifx 2024.1. Doing that drops the target attribute, both automatic arrays sized from dummy arguments, and both c_loc calls -- an automatic array with TARGET whose bound comes from a dummy is the one construct here that appears nowhere else in the module. c_loc leaves the use statement again, so line 2 is back to what master has. The C side is untouched: char arrays still arrive as const char* and overwrite still arrives as const int*. * Update src/system/stdlib_system.F90 * Update src/system/stdlib_system.F90 Co-authored-by: Federico Perini <federico.perini@gmail.com> * Apply review suggestions to the environment variable docs and example --------- Co-authored-by: Federico Perini <federico.perini@gmail.com> Co-authored-by: José Alves <102541118+jalvesz@users.noreply.github.com>
jalvesz
left a comment
There was a problem hiding this comment.
LGTM @kunalKumar-13 could you just please do a pass to shorten the long in-code comments left by the AI agent ? In some places a one-liner comment is more than enough.
| ! -std=f2018 -Wall. For a 200 character string it returns -56, so the | ||
| ! `min` yields -56 and the `s(p:)` below is then a bad substring bound. | ||
| ! The result here is never larger than `p`, so it fits back into int8. | ||
| p = min( int(p), len(s) ) |
There was a problem hiding this comment.
Thanks for catching this, it looks reasonable.
Could you please remove the long comment and replace by a one line note:
p = min( int(p), len(s) ) !> ensure same-kind comparison with explicit conversion, needed for extrict -std=f2018| implicit none | ||
| integer(c_size_t), intent(out) :: len | ||
| logical, intent(in) :: winapi | ||
| ! The C side takes a `bool` by value. A default `logical` passed by |
There was a problem hiding this comment.
Same thing here: please avoid verbose comments when a one liner would suffice. Put the key findings in the PR description.
config/DefaultFlags.cmakecompiles with-std=f2018, but four places rely on GNU extensions that gfortran 15.2 rejects, so a build with the project's own flags stops insrc/stringsandsrc/system:Why CI does not see it
The matrix runs gcc 15 only on
macos-14; the ubuntu matrix stops at gcc 14. The point release available there still accepts these, so the last full run on master is green. Distro gfortran 15.2 does not.The changes
stdlib_str2num—min(p, len(s))compares anint8with a default integer. The result can never exceedp, so promotingpis enough and the value still fits back intoint8.stdlib_system_subprocess—exitis a GNU intrinsic, soimplicit none(external)will not accept it. Bound to C'sexit, which is what the intrinsic calls, so the reason the comment gives for preferring it tostopstill holds. Andmax(0.0_RTICKS, max_wait_time)comparesreal(RTICKS)with a default real.stdlib_system— this one is worth a closer look, because it is a behaviour fix and not only a build fix.The interface to
stdlib_strerrordeclaredwinapias a defaultlogicalpassed by reference. The C side is:a
booltaken by value. So C was reading the address of the Fortran logical as the flag — always a non-null pointer, therefore always true, whatever was passed. The interface already importedc_boolwithout using it, which suggests this was the intent and got lost.On unix nothing changes: the
winapibranch has no body outside_WIN32and falls through tostrerror(errno)either way. On Windows it meansFormatMessageAwas always being used, including whenc_get_strerror()was called with no argument. That is the intended behaviour restored, but I have no Windows machine and cannot test it — please treat that hunk as the one needing a second opinion.Verification
Ubuntu 26.04 aarch64, gfortran 15.2,
-std=f2018left exactly as the repo sets it:masterWhat this does not cover
gfortran 16.2 needs more. It additionally rejects the generated BLAS interfaces in
stdlib_linalg_blas.F90withSymbol 'lda' is used before it is typed, roughly 500 of them. That is a separate and much larger change to the generated sources, and I have not touched it. None of the four sites fixed here are among those, so this is a step toward gfortran 16 rather than the whole of it.Found while building #1208 locally, where I mentioned it and offered to raise it separately.