[flang-commits] [flang] [llvm] [flang-rt] Initialize I/O unit storage read by short-circuit predicates (PR #221126)
Eugene Epshteyn via flang-commits
flang-commits at lists.llvm.org
Thu Sep 3 20:38:29 PDT 2026
https://github.com/eugeneepshteyn created https://github.com/llvm/llvm-project/pull/221126
Fixes #220633.
`ConnectionState` and `OpenFile` hold `common::optional` members that are read through predicates of the form `opt && x < *opt`. Those predicates never use an indeterminate value in the abstract machine, but compilers routinely if-convert the short-circuit `&&` into a branchless compare and select, which speculates the payload load. Because these objects are placement-new'd into `malloc`'d storage by `UnitMap::Create()`, a memory checker sees the payload bytes as never written and reports a conditional branch that depends on uninitialized memory.
This is a false positive — the machine result is provably correct, since the engaged flag is 0 and both arms of the select are 0 — but it fires for *every* Fortran program that writes a record on AArch64, so the noise is unavoidable for anyone running Valgrind on Fortran code.
### Evidence
Same program, same Valgrind 3.27.1:
| | |
|---|---|
| AArch64, flang built from `7a1f6ad89e57` | `ERROR SUMMARY: 2 errors from 2 contexts`, both in `AdvanceRecord`, origin `UnitMap::Create` |
| x86-64, flang built from `dd7236de4812` | `ERROR SUMMARY: 0 errors from 0 contexts` |
| `libgfortran`, same program, same AArch64 host | `ERROR SUMMARY: 0 errors from 0 contexts` |
Disassembling the AArch64 `libflang_rt.runtime.so` — the two addresses Memcheck reports match these by page offset:
```asm
; site 1 — IsAfterEndfile()
ldr x11, [x0, #40] ; currentRecordNumber
ldr x10, [x0, #80] ; *endfileRecordNumber <-- undefined payload
ldrb w9, [x0, #88] ; engaged flag (defined, 0)
cmp x11, x10 ; NZCV now undefined
csel w12, wzr, w9, le
tbz w12, #0, ... ; <-- report #1
```
```asm
; site 2 — IsAtEOF(), reusing the same compare
cmp x11, x10
csel w9, wzr, w9, lt
cbz w9, ... ; <-- report #2
strb wzr, [x0, #88] ; endfileRecordNumber.reset()
```
The threshold is `-O1` in the runtime's own build: 0 errors at `-O0`, 2 at `-O1`/`-O2`/`-O3` on AArch64, 0 at every level on x86-64.
### What this changes
`common::ResetWithDefinedPayload()` leaves an optional disengaged but writes its payload storage; it is called at construction for the optionals in `ConnectionAttributes`, `ConnectionState` and `OpenFile`. This changes no observable behavior — it only makes the speculated read defined.
The helper lives in `flang/Common/optional.h` because both `connection.h` and `file.h` already include it directly, so no new include edges are needed. Happy to move it to a runtime-local header if you would rather it not sit in `Common`.
Two members also get plain initializers:
* `OpenFile::pathLength_` is read through the same if-convertible shape (`path() && pathLength() == n`, in `external-unit.cpp` and `unit-map.cpp`). Note `OpenStatementState::pathLength_` in `io-stmt.h` is already initialized this way.
* `OpenFile::nextId_` is **a genuine uninitialized read**, not just a speculated one: it is assigned only in `Predefine()`, which runs for the preconnected units, so asynchronous I/O on a unit created by `OPEN` increments an indeterminate value and returns it as the `ID=` result. The consequence is mild — the id is only ever used as a key into the `pending_` list, so it is self-consistent — but the value handed back to user code is unpredictable. Say the word if you would prefer this split into its own PR.
### Testing
`check-flang-rt` passes 365/365.
No new test is added, and I do not think one is possible in tree today: the difference is in shadow-memory definedness, which neither a LIT test nor a `flang-rt` unit test can observe, and there is no MSan or Valgrind CI for `flang-rt`. There is precedent for landing this kind of fix untested — 01f2f81f2b9 ("[flang-rt] Fixed uninitialized class member variable") is a one-line header change with no test.
To keep the claim honest, the fix was verified by compiling the **real, patched headers** into a standalone probe that reproduces `UnitMap::Create()`'s malloc + placement-new shape and calls the two predicates in `AdvanceRecord()`'s order:
```
arch=aarch64
unpatched: ok=1 currentRecordNumber=2 hasEndfile=0 | ERROR SUMMARY: 2 errors from 2 contexts
patched: ok=1 currentRecordNumber=2 hasEndfile=0 | ERROR SUMMARY: 0 errors from 0 contexts
```
Identical program output, reports gone. The same probe on x86-64 is clean both ways, as expected.
Not covered: architectures other than x86-64 and AArch64, Windows, and device/offload builds where `common::optional` is flang's own implementation rather than `std::optional` (the helper should behave identically there, but it was not exercised).
>From aa4495f90ce9ded24580b5a9e0841bec508862ea Mon Sep 17 00:00:00 2001
From: Eugene Epshteyn <eepshteyn at nvidia.com>
Date: Thu, 3 Sep 2026 20:35:42 -0700
Subject: [PATCH] [flang-rt] Initialize I/O unit storage read by short-circuit
predicates
ConnectionState and OpenFile hold common::optional members that are read
through predicates of the form `opt && x < *opt`. Those predicates never
use an indeterminate value in the abstract machine, but compilers routinely
if-convert the short-circuit && into a branchless compare and select, which
speculates the payload load. Because these objects are placement-new'd into
malloc'd storage by UnitMap::Create(), a memory checker sees the payload
bytes as never written and reports a conditional branch that depends on
uninitialized memory.
On AArch64 this fires for any Fortran program that writes a record:
program repro
write (*, 10) 'hello'
10 format(' ', a)
end program repro
produces two "Conditional jump or move depends on uninitialised value(s)"
reports in ExternalFileUnit::AdvanceRecord(), one from IsAfterEndfile() and
one from IsAtEOF(). x86-64 is unaffected, and libgfortran is clean on the
same program and host.
Add common::ResetWithDefinedPayload(), which leaves an optional disengaged
but writes its payload storage, and call it at construction for the
optionals in ConnectionAttributes, ConnectionState and OpenFile. This
changes no observable behavior; it only makes the speculated read defined.
Also give OpenFile::pathLength_ and OpenFile::nextId_ initializers.
pathLength_ is read through the same if-convertible shape
(`path() && pathLength() == n`). nextId_ is a genuine uninitialized read:
it is assigned only in Predefine(), which runs for preconnected units, so
asynchronous I/O on a unit created by OPEN increments an indeterminate value
and returns it as the ID= result.
No test is added: the difference is in shadow-memory definedness, which
neither a LIT test nor a flang-rt unit test can observe, and there is no
MSan or Valgrind CI for flang-rt today. Verified by compiling the real
headers into a standalone probe on AArch64 -- unpatched reports 2 errors,
patched reports 0, with identical program output -- and by check-flang-rt
(365/365 passing).
Co-authored-by: Razvan Lupusoru <razvan.lupusoru at gmail.com>
---
.../include/flang-rt/runtime/connection.h | 14 +++++++++++
flang-rt/include/flang-rt/runtime/file.h | 12 +++++++--
flang/include/flang/Common/optional.h | 25 +++++++++++++++++++
3 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/flang-rt/include/flang-rt/runtime/connection.h b/flang-rt/include/flang-rt/runtime/connection.h
index 3e783af1fa748..48b44d006256e 100644
--- a/flang-rt/include/flang-rt/runtime/connection.h
+++ b/flang-rt/include/flang-rt/runtime/connection.h
@@ -25,6 +25,14 @@ enum class Access { Sequential, Direct, Stream };
// These characteristics of a connection are immutable after being
// established in an OPEN statement.
struct ConnectionAttributes {
+ // See common::ResetWithDefinedPayload(): the optionals below are read through
+ // short-circuit predicates that compilers may if-convert, so their payload
+ // storage is written once here to keep memory checkers quiet.
+ RT_API_ATTRS ConnectionAttributes() {
+ common::ResetWithDefinedPayload(isUnformatted);
+ common::ResetWithDefinedPayload(openRecl);
+ }
+
Access access{Access::Sequential}; // ACCESS='SEQUENTIAL', 'DIRECT', 'STREAM'
common::optional<bool> isUnformatted; // FORM='UNFORMATTED' if true
bool isUTF8{false}; // ENCODING='UTF-8'
@@ -45,6 +53,12 @@ struct ConnectionAttributes {
};
struct ConnectionState : public ConnectionAttributes {
+ RT_API_ATTRS ConnectionState() {
+ common::ResetWithDefinedPayload(recordLength);
+ common::ResetWithDefinedPayload(leftTabLimit);
+ common::ResetWithDefinedPayload(endfileRecordNumber);
+ }
+
RT_API_ATTRS bool IsAtEOF() const {
// true when read has hit EOF or endfile record
return endfileRecordNumber && currentRecordNumber >= *endfileRecordNumber;
diff --git a/flang-rt/include/flang-rt/runtime/file.h b/flang-rt/include/flang-rt/runtime/file.h
index 25942c053bbe1..347648e43478f 100644
--- a/flang-rt/include/flang-rt/runtime/file.h
+++ b/flang-rt/include/flang-rt/runtime/file.h
@@ -27,6 +27,14 @@ class OpenFile {
public:
using FileOffset = std::int64_t;
+ // See common::ResetWithDefinedPayload(): openPosition_ and knownSize_ are
+ // read through short-circuit predicates that compilers may if-convert, so
+ // their payload storage is written once here to keep memory checkers quiet.
+ OpenFile() {
+ common::ResetWithDefinedPayload(openPosition_);
+ common::ResetWithDefinedPayload(knownSize_);
+ }
+
int fd() const { return fd_; }
const char *path() const { return path_.get(); }
std::size_t pathLength() const { return pathLength_; }
@@ -90,7 +98,7 @@ class OpenFile {
int fd_{-1};
OwningPtr<char> path_;
- std::size_t pathLength_;
+ std::size_t pathLength_{0};
bool mayRead_{false};
bool mayWrite_{false};
bool mayPosition_{false};
@@ -102,7 +110,7 @@ class OpenFile {
bool isTerminal_{false};
bool isWindowsTextFile_{false}; // expands LF to CR+LF on write
- int nextId_;
+ int nextId_{0};
OwningPtr<Pending> pending_;
};
diff --git a/flang/include/flang/Common/optional.h b/flang/include/flang/Common/optional.h
index 72991111be577..d3188fc6a43c1 100644
--- a/flang/include/flang/Common/optional.h
+++ b/flang/include/flang/Common/optional.h
@@ -237,6 +237,31 @@ using std::nullopt_t;
using std::optional;
#endif // !STD_OPTIONAL_UNSUPPORTED
+// Leaves an optional disengaged, but with its payload storage written rather
+// than left indeterminate.
+//
+// Predicates of the form `opt && x < *opt` test the engaged flag before
+// reading the payload, so they never use an indeterminate value. Compilers
+// do, however, routinely if-convert the short-circuit `&&` into a branchless
+// compare and select, which speculates the payload load. On targets where
+// that happens the select's condition is computed from indeterminate storage,
+// and memory checkers then report a conditional branch that depends on
+// uninitialized memory. This matters for runtime objects that are
+// placement-new'd into malloc'd storage, where a checker can see that the
+// payload bytes were never written.
+//
+// Writing the payload once at construction makes those bytes defined without
+// changing any observable behavior: the optional is still disengaged, and the
+// speculated read is still meaningless. The assignment below is therefore NOT
+// dead code -- it exists solely for its effect on the payload storage, and
+// reset() on a trivially destructible payload merely clears the engaged flag.
+template <typename A>
+FORTRAN_OPTIONAL_INLINE_WITH_ATTRS void ResetWithDefinedPayload(
+ optional<A> &opt) {
+ opt = A{};
+ opt.reset();
+}
+
} // namespace Fortran::common
#endif // FORTRAN_COMMON_OPTIONAL_H
More information about the flang-commits
mailing list