[susilehtola]

Closes #4.

libspill had no Windows implementation at all — not a missing include, but a
POSIX-only I/O and threading layer. Psi4's clang-cl CI stops at open.c's
<unistd.h> while building libspill as a bundled dependency for the libpsio
port (psi4/psi4#3540), and libpsio is core, so there is no "skip it on
Windows" option on that side.

The shape

Everything the library asks of the operating system now goes through
src/os.h, implemented once per platform in src/os_posix.c and
src/os_win32.c. No other file names a system call — that is checkable with a
grep, and it is the property worth keeping:

$ grep -nE '\b(pread|pwrite|ftruncate|fsync|getpid|unlink|fstat)\s*\(' src/*.c \
    | grep -v '^src/os_' | grep -v ls_os_
(nothing)

Threading moved the same way: ls_mutex / ls_cond / ls_thread, which is
pthread_* on POSIX and SRWLOCK / CONDITION_VARIABLE / _beginthreadex on
Win32. Deliberately not a shim under the pthread_ names — a header that
defines pthread_mutex_lock is a trap for anyone who later links a real
pthreads on Windows. The rename is mechanical and the suite covers it.

Two things the Win32 side had to get right, not merely compile

• The transfers stay positional. §5a promises concurrent ls_read/ls_write
  on one store need no external locking, and pread/pwrite deliver that by
  carrying the offset. Win32 gets OVERLAPPED ReadFile/WriteFile, which is
  positional in the same sense even on a synchronous handle. _lseeki64 +
  _read would have compiled and quietly broken the promise; there is a comment
  in os_win32.c saying so, because the next person will reach for it.
• Offsets are 64-bit throughout. off_t is 32 bits in the MSVC CRT, so the
  (off_t) casts this code used to do would have truncated silently past
  2 GB — inside the range this library exists for, not beyond it. Nothing below
  os.h mentions off_t any more.

Mapping

The one facility with no cheap equivalent: ls_map reserves the logical span
PROT_NONE and maps each extent over its slice with MAP_FIXED, which on
Windows needs a placeholder reservation (VirtualAlloc2 + MapViewOfFile3,
Win10 1803+). Rather than carry an untested version of that, LS_MAPPED is
refused at ls_open where LS_HAVE_MMAP is absent, with LS_ERR_MODE — the
answer callers already get for the other unsupported mode combinations. Every
other mode works, and Psi4's shim never uses mapped mode. Happy to do the real
thing in a follow-up if you would rather not ship the gap.

One bug found on the way

LS_API was empty on _WIN32, so a shared build there would have produced a
DLL exporting nothing. It now means dllexport while the library is built and
dllimport in a consumer — the same intent -fvisibility=hidden already
carries elsewhere, which is why the comment above it still reads true.

Verification

All seven CI jobs green, including both Windows toolchains:

job	result
windows (msvc)	9/9, test_mapped skipped
windows (clang-cl)	9/9, test_mapped skipped
make check (gcc, clang)	pass
cmake + ctest + install	pass
no mapping (the Windows path, on Linux)	pass
no HDF5, no Fortran, no numpy	pass

The claim that mattered — that OVERLAPPED ReadFile/WriteFile really is
positional in the sense §5a promises — is not inferred from "it compiled": it is
test_posix's "8 threads x 64 distinct keys: no error, no corruption",
running on Windows. That check passed on Windows even in the round where four of
its siblings were still failing, and test_posix now passes in full.

LS_NO_MMAP also compiles and runs the no-mapping branches under the whole
suite on Linux, so that path is not left to be discovered only on Windows. It
earned its place: it caught a regression mid-review that the ordinary Linux
build could not see.

What is still not proven: that a real consumer links this on Windows.
libspill building and passing its own suite there is necessary, not sufficient —
Psi4's psi4/psi4#3540 is the thing that will actually answer it.

Checked downstream

Psi4's libpsio shim builds and passes its conformance harness against this
branch installed from cmake --install — including the round trip and the
PSIOManager path assertions. On Linux; see above.

Six rounds, for the record

Each CI round found a different class of problem, which is the argument for the
Windows job existing rather than for my having been careful:

1. configure/compile — <unistd.h>, the POSIX-only I/O and threading layer
2. link — strerror_r, posix_memalign, and a stray duplicated LS_API
3. test compilation — pthreads and fork in the tests, EDQUOT, a 64-bit LCG
   declared unsigned long
4. test runtime — ENOENT: the default scratch directory was TMPDIR then
   /tmp
5. test assertions — tests and library each resolving that directory separately
6. packaging — the consumer project demanding a Fortran compiler

Three of those (the LS_API duplicate, the LCG, the Fortran-consumer
requirement) are pre-existing bugs that happen to be invisible on Linux, not
things this port introduced.

[susilehtola]

Review: do not merge yet — both Windows jobs are red, which is your own stated criterion. The design is right and I would take this shape.

(Posting as a comment; GitHub will not let this account request changes on its own PR.)

What I verified rather than took on trust

• POSIX is genuinely unchanged. Full make check green (C, C++, Fortran, Python, crayio at both integer widths, the -i8 caller, install/consume). 32 exported symbols, exactly the public API — the ls_os_*, ls_mutex, ls_cond, ls_thread layer is correctly not exported.
• The rewritten threading is clean under helgrind, on test_posix and test_surveyed, and memcheck is clean. Replacing pthreads wholesale was the part most likely to introduce a silent race, and it did not.
• LS_NO_MMAP does what you say, which is the cleverest thing here. I built and ran it: ordinary tests pass, test_mapped skips with 77. Putting the Windows-only branch under Linux CI rather than leaving it to be discovered on Windows is the right instinct.
• The Win32 primitives are correctly chosen, as far as reading can establish: OVERLAPPED ReadFile/WriteFile is positional on a synchronous handle, _get_osfhandle, _chsize_s (with the correct note that _chsize is not 64-bit), FlushFileBuffers, SleepConditionVariableSRW correctly paired with SRWLOCK, and _O_BINARY — the one that silently corrupts data if forgotten.

Two real bugs found along the way, worth more than the port itself: off_t truncating past 2 GB, inside the range this library exists for, and LS_API empty on _WIN32, so a shared build there would have exported nothing.

Why not merge

Both toolchains fail 6 of 9 tests. It compiles on Windows — real progress — but every failure is the first ls_open returning -2 (-ENOENT):

[FAIL] open t_rt: -2        (test_posix)
open failed: -2             (churn_libspill)
[FAIL] open t_app: -2       (test_surveyed)

One root cause, and the report gives nothing to work with. I could not diagnose it from here — no Windows machine — and I will not guess in a review. What would settle it in one run: log the constructed path and errno at the ls_os_open_rw failure. -ENOENT from _sopen_s with _O_CREAT means the directory is wrong, so the path is the thing to see.

Two cheap candidates to eliminate first:

1. build_path still hardcodes /. os.h abstracts the temp directory but not the separator, so the path is C:\...\Temp/t_rt.libspill. Windows normally accepts that, so probably not the cause — but it is the same class of omission the PR exists to fix, and worth closing regardless.
2. What GetTempPathA actually returns on windows-latest. The trailing-separator stripping is right; that it resolves on the runner is the untested part.

Separately: test_cxx exits 0xC0000409 — STATUS_STACK_BUFFER_OVERRUN, not an ordinary failure. Please do not assume it shares the root cause; a fastfail suggests something writing out of bounds in the Win32 path, and it may well survive the ls_open fix.

One correction to the description

No other file names a system call — that is checkable with a grep

Not quite, and os.h repeats the claim. mmap/munmap remain in store.c:836,844,849,887 and toc.c:93; the grep in the description does not include them. They are correctly guarded by #ifdef LS_HAVE_MMAP with a real #else, so nothing is broken — but the invariant as stated is stronger than the one that holds, and someone will lean on it. Either narrow the claim to "every system call except the mapping family, which is guarded", or move those behind os.h too.

Smaller

• LS_MAPPED refused with LS_ERR_MODE where mapping is absent is consistent with precedent, but a caller cannot distinguish "this build has no mapping" from "wrong mode for this call". LS_ERR_BACKEND is what LS_HDF5 gets for "not compiled in" and is the closer analogue. Not a blocker, and not worth churning without a real caller asking.
• Declining FILE_FLAG_NO_BUFFERING and reporting got_direct = 0 is right. Buffered I/O is a correct answer to direct_io; an untested aligned path would not be.

On the mapping gap

Refusing LS_MAPPED on Windows rather than carrying untested VirtualAlloc2 + MapViewOfFile3 is the right trade and I would not want the real thing in this PR. qp2 is the only consumer that needs mapped mode and it is Linux-only. Leave it.

Get the Windows jobs green and I will approve — the structure is not in question.

[susilehtola]

Approved — both Windows jobs are green, which was the condition. I re-ran the suite locally on the updated branch and it is clean.

The root cause was the class I guessed at, in the place I did not

I looked at build_path and found ls_os_tmpdir already there, said so, and stopped. The duplicated resolution was in the tests: they derived TMPDIR-then-/tmp for their own stat calls while the library asked the platform, which agreed on Linux by coincidence and diverged the moment the library started asking GetTempPath.

66e12c7 fixes it the right way round — state the directory once and pass it, rather than teach a second derivation about a third platform. "It is what a consumer inspecting its own scratch files would do anyway" is the part that makes it a fix rather than a patch, and it is exactly what Psi4's shim does.

test_cxx's 0xC0000409 — resolved, and benignly

I asked you not to assume it shared the root cause. Having looked: it did, and the mechanism is unremarkable. MSVC's abort() raises __fastfail(FAST_FAIL_FATAL_APP_EXIT), which surfaces as 0xC0000409 — so an uncaught ls::error from the failing constructor going through std::terminate produces exactly that code. Not an out-of-bounds write. Worth recording since a fastfail disappearing without explanation is the kind of thing that comes back.

Two things from my review you did not take, and I now agree with you on one

• build_path still hardcodes /. I said "worth closing regardless"; I withdraw that. Both toolchains now pass with forward slashes, so the concern was speculative and CI has answered it. Changing it would be churn against a green result. The one place it could still matter is exact_name on Windows, where a consumer doing its own path comparison might build the name with backslashes — but no exact_name consumer builds on Windows today, and that is a bridge for whoever does.
• os.h still says "nothing else in the library names a system call", which mmap/munmap in store.c and toc.c contradict. This one I do still think is worth correcting — a stated invariant that is not quite true is worse than a narrower one that is, and the guarding is good enough that the honest version costs nothing. I will push that as a one-line follow-up rather than hold the merge for it.

What this PR is actually worth

Beyond Windows: off_t silently truncating past 2 GB was a live bug on every platform with a 32-bit off_t, and LS_API being empty on _WIN32 meant a shared Windows build exported nothing. Neither needed Windows to be reported, and neither had been.

And LS_NO_MMAP — running the no-mapping branches under Linux CI instead of leaving them to be discovered on Windows — is a pattern worth reusing. It is the same instinct as tests/i8_caller.F90: put the configuration you do not build on under the CI you do have.

Ready to merge.

[susilehtola]

All seven jobs green at 5fb7085, both Windows toolchains 9/9 (test_mapped skipped by design). Point by point:

The blocking objection

-ENOENT on the first ls_open was one root cause, as you suspected: build_path resolved its default directory as TMPDIR then /tmp, and Windows has neither. ls_os_tmpdir joins the floor — GetTempPath there, unchanged behaviour on POSIX. Your suggested diagnostic (log the constructed path and errno at the ls_os_open_rw failure) was the right instinct and would have found it in one run rather than two.

Of your two candidates: the hardcoded / was not the cause, as you guessed — but it is fixed anyway, as LS_PATH_SEP, for the reason you give.

test_cxx 0xC0000409

You were right to tell me not to assume it shared the root cause. It did, but not by the route either of us would have guessed, and it is not an out-of-bounds write:

• ls::store’s constructor throws on failure (if (!s_) throw error(err, ...)), main does not catch, and MSVC routes std::terminate/abort through __fastfail, which reports 0xC0000409.
• In that run test_cxx printed zero checks — it died at the first store construction, which is what throwing looks like and is not what corruption looks like.
• It appeared in exactly one round. The next round, once most opens worked, it was an ordinary 18 checks, 1 failed. It now passes.

The invariant

Your correction was right and I have made the claim true rather than narrowing it, since the claim was the useful half. ls_os_map_extents/ls_os_unmap are in the floor — declared in internal.h rather than os.h because they speak in ls_extent, defined only in os_posix.c under LS_HAVE_MMAP. store.c keeps the policy (which record, the busy count, the table of contents) and hands the reserve-then-MAP_FIXED sequence to the platform, which is also where a VirtualAlloc2 + MapViewOfFile3 implementation would slot in if it is ever worth carrying. Your grep returns nothing now, including the mapping family.

Worth noting test_mapped passes in the ordinary configuration after the move, so the relocated code is exercised rather than merely compiled.

LS_ERR_MODE vs LS_ERR_BACKEND

You make the better argument — LS_ERR_BACKEND is what LS_HDF5 gets for "not compiled in" and is the closer analogue — and you also say it is not worth churning without a caller asking. Agreed on both, so it is unchanged, with the reasoning recorded in the commit message rather than left as a silent omission. Easy to revisit if a mapped-mode consumer ever appears on Windows.

What is still not proven

libspill building and passing its own suite on Windows is necessary, not sufficient. Whether a real consumer links it there is psi4/psi4#3540’s question, and that is still open.

Not merging on my own green run plus my own comment — over to you.