diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7b9e062..2c77817 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -215,6 +215,32 @@ jobs: } subst mcpp.toml .spec + # THIS PACKAGE'S OWN TESTS HAD NEVER RUN. + # + # tests/ has held five suites since this implementation was written and no + # workflow invoked them. What ran was the specification's suite, which is a + # different instrument: it examines what every implementation must do, and + # these examine what this one does with the system beneath it. A defect + # visible only to the second kind was therefore invisible here, which is + # what happened to openkal.timeout -- there was no observation of it in + # either place that a wait upon the wrong descriptor did not satisfy. + # + # The step is openkal-linux's, unchanged, including the assertion that + # every suite ran: a suite that discovered nothing reports success. + - name: This package's own tests + run: | + set -euo pipefail + mcpp test 2>&1 | tee tests.log + # The list is derived from the files present rather than written out + # here: a hand-written list names the suites that existed when it was + # written, and one added afterwards escapes the assertion silently. + missing=0 + for f in tests/*.cpp; do + name="$(basename "$f" .cpp)" + grep -q "^$name \.\.\. ok" tests.log || { echo "did not run or did not pass: $name" >&2; missing=1; } + done + test "$missing" -eq 0 + # The other architecture, as far as this system allows it to be reached. # # The system-call numbers agree between the two --- measured, in the diff --git a/README.md b/README.md index e209a2c..d7eb4d7 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ written on the kernel's own calls. openkal = "0.9.0" [target.'cfg(os = "macos")'.dependencies] -openkal-macos = "0.6.0" +openkal-macos = "0.6.1" ``` Its purpose is as much to test the specification as to be used. A specification diff --git a/mcpp.toml b/mcpp.toml index d2dbc96..4d5bdae 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,7 +1,7 @@ [package] namespace = "mcpplibs" name = "openkal-macos" -version = "0.6.0" +version = "0.6.1" description = "An implementation of openkal for macOS, written on the kernel's own calls. Its purpose is as much to test the specification as to be used." license = "Apache-2.0" diff --git a/src/handle.h b/src/handle.h index 48f031c..e0ac745 100644 --- a/src/handle.h +++ b/src/handle.h @@ -24,6 +24,18 @@ inline okm_uptr pack(int fd) { } // Returns the descriptor, or -1 if the word does not name a live one. +// +// THIS ACCEPTS A WORD THAT WAS NEVER PACKED, AND SILENTLY. A bare descriptor N +// has the shape of a packed handle naming N-1 whose generation is still zero, +// so this returns N-1 for it rather than -1. Nothing here can tell the two +// apart: the word is one machine word and carries no tag. +// +// The consequence is that a handle of the OTHER discipline must never reach +// this function. openkal.stream's handles are bare descriptors (stream.cpp +// states why), and src/timeout.cpp used to pass one here and wait upon the +// descriptor below the one it then transferred upon. Owned handles --- kal_file, +// kal_dir, kal_net_listener, kal_net_conn, kal_datagram --- are the whole of +// this function's domain. inline int unpack(okm_uptr h) { const int fd = static_cast(h & 0xffffffffu) - 1; if (fd < 0 || fd >= kMaxDescriptor) return -1; diff --git a/src/timeout.cpp b/src/timeout.cpp index 439f2ef..b969054 100644 --- a/src/timeout.cpp +++ b/src/timeout.cpp @@ -59,6 +59,36 @@ int await(int fd, short events, kal_u64 ns) { return kal_ok; } +// A STREAM HANDLE IS A DESCRIPTOR AND IS NOT DECODED. stream.cpp states it in +// terms, kal_stream_read and kal_stream_write take it as one, and so must the +// wait that precedes them: the wait and the transfer that follows it have to +// name the same object or the wait answers about something else. +// +// THIS FILE USED TO DECODE IT AS AN OWNED HANDLE, AND THE DECODE SUCCEEDED. +// handle.h packs an owned handle as (generation << 32) | (fd + 1), so a bare +// descriptor N has exactly the shape of a packed handle naming N-1, and +// okm::unpack accepts it whenever generation N-1 is still zero -- which is +// every index at which no owned handle has yet been released. The wait was +// therefore performed upon descriptor N-1 and the transfer upon N. +// +// The former reading tested `unpack` and fell back when it failed, on the +// stated ground that the standard streams are not packed. That ground is +// correct and the conclusion drawn from it was not: NO stream handle is packed +// here, and of the three standard ones only kal_stdin, whose handle is zero, +// fails to decode. kal_stdout decoded to descriptor 0 and kal_stderr to 1. +// +// What a caller observed was an expiry for a stream that had bytes waiting, or +// a wait without bound inside an operation that states one, according to what +// happened to occupy the descriptor below. Both are functions of the process's +// descriptor history, so the same program answered differently when run alone +// and when run after something else -- and the index healed for good once an +// owned handle at N-1 had been released, because that advances the generation +// and makes the decode fail correctly. +// +// Found in openkal-linux, whose timeout.cpp is this file's counterpart and +// carried the same four call sites with the same two wrong. +int stream_fd(kal_stream s) { return static_cast(s.h); } + } // namespace extern "C" { @@ -68,13 +98,8 @@ kal_intptr kal_timeout_read(kal_stream s, void* buf, kal_uintptr len, kal_u64 ns // would turn a call that always succeeds into one that can expire. if (len == 0) return 0; - const int fd = okm::unpack(s.h); - // THE STANDARD STREAMS ARE NOT PACKED HANDLES. openkal.stream reports them - // as the descriptors themselves, so a word that does not unpack is taken to - // be one of those rather than being refused. - const int use = (fd >= 0) ? fd : static_cast(s.h); - - if (const int rc = await(use, static_cast(okm::poll_in), ns); rc != kal_ok) + if (const int rc = await(stream_fd(s), static_cast(okm::poll_in), ns); + rc != kal_ok) return -rc; return kal_stream_read(s, buf, len); } @@ -82,14 +107,19 @@ kal_intptr kal_timeout_read(kal_stream s, void* buf, kal_uintptr len, kal_u64 ns kal_intptr kal_timeout_write(kal_stream s, const void* buf, kal_uintptr len, kal_u64 ns) { if (len == 0) return 0; - const int fd = okm::unpack(s.h); - const int use = (fd >= 0) ? fd : static_cast(s.h); - - if (const int rc = await(use, static_cast(okm::poll_out), ns); rc != kal_ok) + if (const int rc = await(stream_fd(s), static_cast(okm::poll_out), ns); + rc != kal_ok) return -rc; return kal_stream_write(s, buf, len); } +// THE TWO OPERATIONS BELOW DO DECODE, AND THAT IS NOT AN INCONSISTENCY WITH THE +// TWO ABOVE. A listener and a datagram are owned: their handles are made by +// okm::pack and released by okm::retire, so the decode is the operation that +// recovers the descriptor and its failure is how a released handle is refused. +// A stream is borrowed and carries no generation. The four call sites divide +// exactly along that line, and the two that were wrong were the two that had a +// borrowed handle in hand. int kal_timeout_accept(kal_net_listener l, kal_u64 ns, kal_net_conn* out) { if (out == nullptr) return kal_err_invalid; const int fd = okm::unpack(l.h); diff --git a/tests/conformance_env_time.cpp b/tests/conformance_env_time.cpp index fc56909..874ef60 100644 --- a/tests/conformance_env_time.cpp +++ b/tests/conformance_env_time.cpp @@ -46,11 +46,11 @@ int main() { // The wall source is claimed by this implementation, so it must report a // time after the specification was written rather than zero. - check(kal::time::has(kal::time::prop_wall_available), "the wall source is claimed"); + check(kal::time::has(kal::time::wall_available), "the wall source is claimed"); check(kal::time::wall() > 1700000000ull * 1000000000ull, "the wall source reports a plausible time"); - const char ok[] = "openkal-linux: env and time conformance\n"; + const char ok[] = "openkal-macos: env and time conformance\n"; kal::write(kal::out(), ok, sizeof(ok) - 1); return failures == 0 ? 0 : 1; } diff --git a/tests/conformance_fs.cpp b/tests/conformance_fs.cpp index f490e0d..82e6e44 100644 --- a/tests/conformance_fs.cpp +++ b/tests/conformance_fs.cpp @@ -100,7 +100,7 @@ int main() { check(kal_fs_remove(root, "okl_dir", 7) == kal_ok, "the directory is removed"); check(kal_fs_remove(root, "okl_probe.txt", 13) == kal_ok, "the file is removed"); - const char ok[] = "openkal-linux: file system conformance\n"; + const char ok[] = "openkal-macos: file system conformance\n"; kal::write(kal::out(), ok, sizeof(ok) - 1); return failures == 0 ? 0 : 1; } diff --git a/tests/conformance_memory.cpp b/tests/conformance_memory.cpp index 9aec6eb..82396a5 100644 --- a/tests/conformance_memory.cpp +++ b/tests/conformance_memory.cpp @@ -16,7 +16,7 @@ int main() { // cannot be used. if (kal::alloc(0, 8) != nullptr) return 1; - const char ok[] = "openkal-linux: memory conformance\n"; + const char ok[] = "openkal-macos: memory conformance\n"; kal::write(kal::out(), ok, sizeof(ok) - 1); return 0; } diff --git a/tests/conformance_process_task.cpp b/tests/conformance_process_task.cpp index 5353253..cffe6e4 100644 --- a/tests/conformance_process_task.cpp +++ b/tests/conformance_process_task.cpp @@ -6,6 +6,20 @@ import openkal.stream; namespace { int failures = 0; + +void say(const char* s) { + kal_uintptr n = 0; while (s[n]) ++n; + kal::write(kal::err(), s, n); +} + +void say_num(int v) { + if (v < 0) { kal::write(kal::err(), "-", 1); v = -v; } + char b[12]; int i = 12; + if (v == 0) b[--i] = '0'; + while (v > 0) { b[--i] = static_cast('0' + v % 10); v /= 10; } + kal::write(kal::err(), b + i, static_cast(12 - i)); +} + void check(bool ok, const char* what) { if (ok) return; ++failures; @@ -57,17 +71,52 @@ int main() { const char* false_paths[] = { "bin/false", "usr/bin/false" }; const kal_uintptr false_lens[] = { 9, 13 }; + // THE CANDIDATE IS CHOSEN BY ASKING, AND IT USED TO BE CHOSEN BY + // SPAWNING AND RETRYING. That retry could not work and never ran: + // kal_process_spawn reports whether the DUPLICATE was made, and the + // program is replaced afterwards, inside a copy the caller no longer + // is. A path that does not exist therefore produces kal_ok and a + // duplicate that finishes with 127, so the first candidate was always + // taken and the second was unreachable code. + // + // On a system holding /usr/bin/true and no /bin/true the consequence + // was `status == 127' at the observation below, which is what this + // system reported the first time these suites were ever run. The + // observation after it -- that a non-zero status is reported as such -- + // held throughout, upon a program that was never started. + auto locate = [&](const char* const* paths, const kal_uintptr* lens_) -> int { + for (int i = 0; i < 2; ++i) { + kal_node_info info{}; info.self_size = sizeof info; + if (kal_fs_info(slash, paths[i], lens_[i], 0, kal::fs::field::kind, &info) != kal_ok) + continue; + if (info.kind != kal_node_absent) return i; + } + return -1; + }; + + const int t = locate(true_paths, true_lens); + const int fpath = locate(false_paths, false_lens); + check(t >= 0, "a program that succeeds is found"); + check(fpath >= 0, "a program that fails is found"); + kal_process p{}; const char* argv[] = { "openkal" }; const kal_uintptr lens[] = { 7 }; int rc = kal_err_invalid; - for (int i = 0; i < 2 && rc != kal_ok; ++i) - rc = kal_process_spawn(slash, true_paths[i], true_lens[i], argv, lens, 1, + if (t >= 0) + rc = kal_process_spawn(slash, true_paths[t], true_lens[t], argv, lens, 1, nullptr, nullptr, 0, nullptr, &p); check(rc == kal_ok, "a program is started"); if (rc == kal_ok) { int status = -1, terminated = -1; check(kal_process_wait(p, &status, &terminated) == kal_ok, "the program is waited for"); + // The observed status is reported when it is wrong. `127' names an + // image that was not replaced and `0' names one that ran; without + // the number the two arrive as the same line. + if (!(status == 0 && terminated == 0)) { + say(" status="); say_num(status); + say(" terminated="); say_num(terminated); say("\n"); + } check(status == 0 && terminated == 0, "the status it finished with is reported"); kal_process_close(p); } @@ -78,13 +127,19 @@ int main() { kal_process q{}; const char* qargv[] = { "openkal" }; int qrc = kal_err_invalid; - for (int i = 0; i < 2 && qrc != kal_ok; ++i) - qrc = kal_process_spawn(slash, false_paths[i], false_lens[i], qargv, lens, 1, + if (fpath >= 0) + qrc = kal_process_spawn(slash, false_paths[fpath], false_lens[fpath], qargv, lens, 1, nullptr, nullptr, 0, nullptr, &q); + check(qrc == kal_ok, "the program that fails is started"); if (qrc == kal_ok) { int status = -1, terminated = -1; kal_process_wait(q, &status, &terminated); - check(status != 0, "a non-zero status is reported as such"); + // NOT MERELY NON-ZERO. 127 is what a duplicate reports when the + // image was never replaced, so `!= 0' is satisfied by a program + // that did not run -- which is precisely how this observation held + // while the one above did not. + check(status == 1 && terminated == 0, + "the status of a program that fails is its own and not 127"); kal_process_close(q); } @@ -105,12 +160,18 @@ int main() { const char* script = "test \"$0\" = openkal-observed-argv0"; kal_uintptr script_len = 0; while (script[script_len]) ++script_len; + // Located rather than retried, for the reason recorded above: the retry + // this replaces could not distinguish a path that does not exist from + // one that does, so it always took the first. + const int sh = locate(sh_paths, sh_lens); + check(sh >= 0, "a shell is found"); + kal_process r{}; const char* rargv[] = { "openkal-observed-argv0", "-c", script }; const kal_uintptr rlens[] = { 22, 2, script_len }; int rrc = kal_err_invalid; - for (int i = 0; i < 2 && rrc != kal_ok; ++i) - rrc = kal_process_spawn(slash, sh_paths[i], sh_lens[i], rargv, rlens, 3, + if (sh >= 0) + rrc = kal_process_spawn(slash, sh_paths[sh], sh_lens[sh], rargv, rlens, 3, nullptr, nullptr, 0, nullptr, &r); check(rrc == kal_ok, "a shell is started"); if (rrc == kal_ok) { @@ -153,7 +214,7 @@ int main() { kal_task_yield(); check(kal_task_current() != 0, "the calling context has an identity"); - const char ok[] = "openkal-linux: process and task conformance\n"; + const char ok[] = "openkal-macos: process and task conformance\n"; kal::write(kal::out(), ok, sizeof(ok) - 1); return failures == 0 ? 0 : 1; } diff --git a/tests/conformance_stream.cpp b/tests/conformance_stream.cpp index 85d9ed9..3210d31 100644 --- a/tests/conformance_stream.cpp +++ b/tests/conformance_stream.cpp @@ -32,7 +32,7 @@ int main() { // A write transfers the whole buffer or reports why it could not. The // specification excludes a successful partial transfer, so a conforming // result reports either the full count or a non-zero error. - const char msg[] = "openkal-linux: conformance\n"; + const char msg[] = "openkal-macos: conformance\n"; // ⭐ ONE SIGNED WORD: the count, or the negated condition when no byte // moved. A caller never inspects two things to learn one thing. const kal_intptr r = kal::write(kal::out(), msg, sizeof(msg) - 1); diff --git a/tests/conformance_timeout.cpp b/tests/conformance_timeout.cpp new file mode 100644 index 0000000..0982784 --- /dev/null +++ b/tests/conformance_timeout.cpp @@ -0,0 +1,125 @@ +// Conformance: openkal.timeout, and in particular that a bounded transfer waits +// upon the stream it then transfers upon. +// +// THIS IS THE OBSERVATION THAT WAS MISSING while kal_timeout_read and +// kal_timeout_write waited upon the descriptor below the one they moved. Nothing +// in this package examined either operation: there was no test for openkal.timeout +// at all, and the specification's own suite examines them through observations +// that a wait upon the wrong descriptor satisfies -- a bounded read of the +// standard input is permitted to expire, so an implementation that expires for +// the wrong reason is indistinguishable there from one that is right. +import openkal.types; +import openkal.stream; +import openkal.process; +import openkal.timeout; + +namespace { + +int failures = 0; + +void say(const char* s) { + kal_uintptr n = 0; while (s[n]) ++n; + kal::write(kal::err(), s, n); +} + +void say_num(int v) { + char b[12]; int i = 12; + if (v == 0) b[--i] = '0'; + while (v > 0) { b[--i] = static_cast('0' + v % 10); v /= 10; } + kal::write(kal::err(), b + i, static_cast(12 - i)); +} + +void check(bool ok, const char* what) { + if (ok) return; + ++failures; + say("FAIL: "); say(what); say("\n"); +} + +// WHY SIXTEEN CHANNELS AND NOT ONE. Under the defect the wait was performed upon +// descriptor N-1, and whether that expires depends on what occupies N-1. For a +// channel made after another, N-1 is the previous channel's writing end, upon +// which input is never reported, so the read expires while bytes sit in the +// stream that was asked for. One channel would be answering about whichever +// descriptor happened to precede it; sixteen make the answer a property of the +// implementation rather than of the process that ran it. +// +// AND WHY THIS RUNS BEFORE ANYTHING ELSE. The mistaken decode succeeded only +// while the generation recorded for N-1 was still zero, so it corrected itself +// for every index at which an owned handle had already been released. Placed +// after anything that creates and releases a file, a listener or a datagram, +// this would examine reused descriptors and hold upon the defect. +void bounded_transfer_waits_on_the_stream() { + constexpr int n = 16; + kal_stream mine[n]{}, theirs[n]{}; + int made = 0; + + for (; made < n; ++made) { + if (kal_process_channel(&mine[made], &theirs[made]) != kal_ok) break; + const char byte = 'x'; + if (kal_stream_write(theirs[made], &byte, 1) != 1) break; + } + check(made == n, "sixteen channels are created and each is written to"); + + int transferred = 0; + for (int i = 0; i < made; ++i) { + char buf[1] = {}; + // One millisecond. Every one of these streams has a byte waiting, so a + // correct implementation does not reach the bound at all; the bound is + // present so that a wait upon the wrong descriptor ends the run rather + // than hanging it. + if (kal_timeout_read(mine[i], buf, 1, 1000000) == 1 && buf[0] == 'x') + ++transferred; + } + // The count rather than the first failure. Sixteen distinguishes a correct + // implementation from one answering about a neighbouring descriptor; the + // first failure alone would not say which. + if (transferred != made) { + say(" "); say_num(transferred); say(" of "); say_num(made); + say(" bounded reads transferred\n"); + } + check(transferred == made, + "a bounded read of a stream that has bytes waiting transfers them"); + + // The mirror, and it is a control rather than a discriminator: a stream with + // nothing in it expires under both readings. It is here so that the + // observation above cannot be satisfied by an implementation that never + // waits at all. + kal_stream empty_mine{}, empty_theirs{}; + if (kal_process_channel(&empty_mine, &empty_theirs) == kal_ok) { + char buf[1] = {}; + check(kal_timeout_read(empty_mine, buf, 1, 1000000) == -kal_err_again, + "a bounded read of a stream with nothing waiting expires"); + kal_process_channel_close(empty_theirs); + kal_process_channel_close(empty_mine); + } + + for (int i = 0; i < made; ++i) { + kal_process_channel_close(theirs[i]); + kal_process_channel_close(mine[i]); + } +} + +void the_rest_of_the_interface() { + check(kal_timeout_granularity() > 0, + "the granularity is a positive number of nanoseconds"); + + // A transfer of zero bytes does not wait and is not bounded. + check(kal_timeout_write(kal_stdout(), "", 0, 1) >= 0, + "a bounded transfer of zero bytes succeeds"); + + // A bound of zero denotes no bound, which is the convention kal_task_wait + // establishes. Observed by writing, which does not wait; a run that waited + // without bound would never report. + check(kal_timeout_write(kal_stdout(), "", 0, 0) >= 0, + "a bound of zero is accepted and denotes no bound"); +} + +} // namespace + +int main() { + bounded_transfer_waits_on_the_stream(); + the_rest_of_the_interface(); + + if (failures == 0) say("openkal-macos: timeout conformance\n"); + return failures == 0 ? 0 : 1; +}