Added ARM32 support

source/HotReload/README.md

@@ -8,11 +8,17 @@ reference; see this project's top-level `README.md` for a quicker
 overview, feature summary, and build/run instructions if you haven't
 already.
 
-`hot_reload.h` is the API (also fully Doxygen-documented -- see the
-top-level README's "Documentation" section for how to build that
-locally); `source/examples/HotReloadDemo` is a full worked example;
-this file is the overview `hot_reload.h`'s own doc comments don't have
-room for, especially the parts you should know are *not* guaranteed.
+`hot_reload::Context` (`include/hot_reload/Context.hpp`) is the primary,
+modern C++ API; `hot_reload.h` is a separate, independently stable plain
+C API built on top of it, for anything that specifically needs C
+linkage. Both are fully Doxygen-documented -- see the top-level README's
+"Documentation" section for how to build that locally, and its "Pick an
+API" section for which of the two fits what you're writing.
+`source/examples/HotReloadDemo` is a full worked example (through the C
+API; `source/tests/ContextTests/Main.cpp` is the equivalent through
+`Context` directly, concurrency stress test included); this file is the
+overview neither API's own doc comments have room for, especially the
+parts you should know are *not* guaranteed.
 
 To see it run:
 
@@ -37,17 +43,20 @@ as you want while it's live.
 
 ## How it works, in one paragraph
 
-`hot_reload_load()` loads a shared library and reads its exported
-symbol table. `hot_reload_reload()` loads a *new* build of that same
-library, reads its symbol table too, and for every name both builds
-export as a function, overwrites the first few bytes of the *old*
-function with an unconditional jump to the *new* one's address. Every
-existing call site -- direct calls, calls through the PLT/GOT, raw
-function pointers already sitting in some struct -- reaches the old
-address and immediately redirects, because that's now all that's
-there. Full byte-level detail, verified against a real assembler
-rather than transcribed from memory, is in
-`src/Architecture.cpp`'s file comment.
+`load()` loads a shared library and reads its exported symbol table.
+`reload()` loads a *new* build of that same library, reads its symbol
+table too, and for every name both builds export as a function,
+overwrites the first few bytes of the *old* function with an
+unconditional jump to the *new* one's address. Every existing call site
+-- direct calls, calls through the PLT/GOT, raw function pointers
+already sitting in some struct -- reaches the old address and
+immediately redirects, because that's now all that's there. Full
+byte-level detail, verified against a real assembler rather than
+transcribed from memory, is in `src/Architecture.cpp`'s file comment.
+(Written here as plain `load()`/`reload()` because the mechanism is the
+same regardless of which API calls it -- `hot_reload::Context::load()`
+directly, or `hot_reload_load()` through the C API, which calls that
+same method.)
 
 ## The two guarantees this library asked to make
 
@@ -55,7 +64,7 @@ rather than transcribed from memory, is in
 convention you have to follow -- it's mechanical. `src/SymbolTable.cpp`
 classifies every exported symbol using the loader's own metadata (an
 ELF symbol's type + its section's executable flag; a PE export's
-containing section's characteristics), and `HotReloadContext` refuses
+containing section's characteristics), and `Context` refuses
 to patch anything not classified as a function, on *either* side of a
 reload. `source/examples/HotReloadDemo` proves this concretely:
 `plugin_build_marker` is an exported `int` sitting right next to real
@@ -77,6 +86,94 @@ Neither guarantee depends on anything the hot-reloadable module's
 author has to remember to do correctly -- see "What this cannot
 detect" below for the one important thing that's true in reverse.
 
+## The C++ API: design and thread safety
+
+`hot_reload::Context` (`include/hot_reload/Context.hpp`) is a thin
+public shell: one `std::unique_ptr<Impl>` member, with `Impl` defined
+entirely inside `Context.cpp` and never named in any header. Every
+other detail:: header this library has -- `ModuleHandle.hpp`,
+`SymbolTable.hpp`, the rest -- is included by `Context.cpp` alone,
+which is what lets `Context.hpp` stay free of all of them: a caller who
+only `#include`s that one header never sees any of this library's own
+internals, and a change to one of those internals never forces that
+caller to recompile. It also buys something PIMPL is well known for but
+worth saying plainly here: `Impl` holds a `std::shared_mutex`, which is
+neither copyable nor movable, yet `Context` itself is freely movable --
+moving one just moves the pointer -- confirmed directly in
+`source/tests/ContextTests/Main.cpp`'s own `runMoveSemantics()`. A
+moved-from `Context` is left in the same state a moved-from
+`std::unique_ptr` is: valid to destroy or move-assign to, nothing else.
+
+**Thread safety is a property of `Impl`'s `std::shared_mutex`, nothing
+more exotic.** `load()` and `reload()` take it exclusively
+(`std::unique_lock`); `findSymbol()` takes it shared
+(`std::shared_lock`), so concurrent lookups run in parallel with each
+other and only block when a `load()`/`reload()` is actually in
+progress. This is a claim about *this object's own bookkeeping* --
+the `std::unordered_map` of loaded modules, specifically -- not about
+the bytes a reload patches: nothing can make it safe to rewrite a
+function's machine code while some other thread is, at that exact
+moment, executing inside it, on any architecture, by any tool. Reload
+from a quiescent point for the module involved, the same as a
+single-threaded caller would; see "What this cannot detect" below for
+the full reasoning. Verified, not just designed this way and assumed
+correct: `source/tests/ContextTests/Main.cpp`'s own
+`runConcurrencyStress()` runs several threads calling `findSymbol()`
+against others calling `reload()` -- some on the very same module name
+as each other, a genuine race for which build ends up active that the
+test makes no assertion about, only that nothing crashes or corrupts --
+plus several more threads each `load()`-ing their own distinct module
+name into the same shared `Context`, all run under ThreadSanitizer
+across five separate executions specifically to cover more than one
+possible thread interleaving. Zero races reported, every time; see the
+top-level README's "What's actually been tested" table.
+
+**`Result` replaces the C API's split between a returned status and a
+separate, stateful `hot_reload_last_error()` call.** That split is
+awkward even single-threaded (an error you have to remember to go ask
+for, from a call that already finished) and genuinely ambiguous once
+more than one thread can be calling into the same context: whose
+"last" error is it? `Result::status()` and `Result::message()` are
+both bundled into the value `load()`/`reload()` themselves return, so
+there is no shared, stateful side-channel for two threads' calls to
+collide on in the first place -- not a mutex added around the old
+design, a different design that does not need one for this specific
+problem. The C API still offers `hot_reload_last_error()`, because
+that is part of its own, separately stable contract, and implements it
+correctly (each calling thread sees its own most recent error on a
+given context, never another thread's) with a `thread_local`
+`std::unordered_map` in `c-api/HotReloadApi.cpp`, keyed by a
+never-reused per-context ID rather than the context's own address --
+otherwise a destroyed context's memory being reused by an unrelated
+later one could, in a narrow window, read back a stale message that
+was never actually about it.
+
+**The patch-writing mechanism in `Architecture.cpp` itself is
+unchanged.** Each backend's `writeJumpAt()` still uses a plain
+`memcpy()` plus `std::atomic_thread_fence()` for its write-ordering
+protocol (payload first, the instruction that actually arms the jump
+last), not `std::atomic_ref`. This was a deliberate choice, not an
+oversight: `std::atomic_ref` requires the referenced object to be
+suitably aligned for its type, which this code cannot promise in
+general -- an x86/x86-64 function's entry point, and the literal
+address embedded a few bytes into it, can legally sit at any byte
+offset at all, with no alignment guarantee whatsoever. Applying
+`atomic_ref` somewhere that precondition does not provably hold would
+be undefined behavior, which is strictly worse than the fence-based
+approach already here -- so rather than apply it selectively (correct
+on, say, ARM64's and ARM32's naturally-4-byte-aligned literals, wrong
+everywhere else) for an inconsistent, easy-to-misapply-later gain,
+every backend keeps the one mechanism that is actually guaranteed
+correct on all four.
+
+**`hot_reload::demangle()`'s MSVC path gained a mutex.**
+`UnDecorateSymbolName`, and DbgHelp as a whole, is documented by
+Microsoft as unsafe to call from more than one thread at once --
+already true before this library had any concurrency story of its own,
+simply never written down. `Demangle.cpp` now serializes every MSVC-path
+call through one `std::mutex`; the GCC/Clang path (`abi::__cxa_demangle`)
+needs nothing equivalent, since it keeps no comparable shared state.
+
 ## Hot reloading C++ symbols (classes, virtual methods, operators)
 
 Everything above works for a real, unmodified C++ mangled name with
@@ -256,12 +353,13 @@ anything whose `hot_reload_demangle()`'d name starts with `"std::"`.
 
 | | x86 | x86-64 | ARM64 | ARM32 |
 |---|---|---|---|---|
-| Jump-patch backend | yes | yes | yes | not implemented |
+| Jump-patch backend | yes | yes | yes | yes |
 | Landing pad handled | ENDBR32 | ENDBR64 | BTI | -- |
 
-*"Any architecture"* was the ask; ARM32 is the one mainstream target
-deliberately left out, because it's the one case this project's
-testing couldn't back up with confidence -- see below.
+ARM32's own row needs a caveat the table can't carry: "yes" means a function's
+*address* decides ARM vs. Thumb state, not a build-wide setting, and both are
+handled -- see the "What was actually run" bullet below for exactly how that
+was confirmed, not just reasoned through.
 
 **Compilers.** GCC and Clang share one backend (including MinGW,
 which is one or the other targeting Windows); MSVC has its own
@@ -276,6 +374,40 @@ export-table reader in `SymbolTable.cpp`.
   twice -- once plain, once with `-mbranch-protection=bti` -- to
   exercise the BTI-landing-pad path on real (emulated) ARM64
   semantics, not just have it compile.
+- Cross-compiled to ARM32 and run under `qemu-arm`: the full demo and
+  test suite, across both of this architecture's incompatible
+  instruction-decoding states (the default Thumb-2, and ARM state
+  forced with `-marm`) and across `-O0` through `-O3`. This is the one
+  architecture here whose jump-patch backend cannot pick a single
+  instruction encoding at compile time the way the other three do --
+  which of the two states a given function is in is a property of
+  that function alone, read from the same bit the ARM EABI already
+  marks every function symbol with (set for Thumb, clear for ARM; a
+  `loadedBase + st_value` sum this library already computed the same
+  generic way for every architecture, so nothing in `SymbolTable.cpp`
+  needed to change for this). Thumb state carries a second wrinkle
+  on top: its own literal-load jump instruction needs to sit on a
+  4-byte boundary, which nothing guarantees for a Thumb function in
+  general (only 2-byte alignment is). Rather than trust that reasoning
+  alone, a plugin was built with a function whose compiled address was
+  confirmed, via its own symbol table entry, to land 2 bytes off that
+  boundary, then loaded, reloaded, and called through across the
+  reload under `qemu-arm` -- proving the one realigning NOP this
+  backend adds for exactly that case actually works, not just that it
+  assembles. Building the test suite here also surfaced a real,
+  architecture-specific gap in the suite itself: its "too small to
+  patch" function, reliably minimal on every other architecture as a
+  plain empty C++ body, picks up a big enough unoptimized
+  frame-pointer prologue on this one at `-O0` to tie this backend's
+  own jump size exactly -- closed with a third implementation of that
+  one function, hand-written at file scope instead of compiler-
+  generated, for this architecture specifically (see
+  `source/examples/HotReloadDemo/Shared/PluginApi.h`'s doc comment on
+  `plugin_tiny_function`).
+  32-bit ARM running under MSVC was not attempted -- no Windows-on-ARM32
+  environment was available to build or verify against, and it is a
+  rare enough target in practice that, unlike x86/x86-64/ARM64, this
+  library makes no claim about it either way.
 - Cross-compiled with MinGW and run under Wine: the full demo,
   exercising the real Win32 `LoadLibrary`/`VirtualProtect`/
   `FlushInstructionCache` calls and the PE export-table parser. This
@@ -313,20 +445,8 @@ export-table reader in `SymbolTable.cpp`.
   MinGW -- between the two, treat it as real-world-checked now, not
   merely reasoned through, though obviously with less first-hand
   confidence than everything that was directly, repeatedly run in
-  this project's own development loop (native GCC/Clang, ARM64 under
-  QEMU, MinGW under Wine).
-- **32-bit ARM was not implemented at all**, deliberately: it's the
-  one architecture in this list where getting it wrong is easy and
-  getting it verified is hard. Its instructions are either 4 bytes
-  (ARM mode) or a 2/4-byte mix (Thumb mode), and a function pointer's
-  low bit is the only thing that says which -- get the interworking
-  wrong and you don't get a clean crash, you get a CPU that's decoding
-  the wrong instruction stream. Nothing in this environment could
-  exercise that on real or emulated ARM32 hardware, so rather than
-  ship an ARM32 backend on the strength of a manual reading of the
-  ISA reference alone, it's left out. Adding it is a matter of
-  implementing one more branch of `src/Architecture.cpp`'s interface;
-  test it for real before trusting it.
+  this project's own development loop (native GCC/Clang, ARM64 and
+  ARM32 under QEMU, MinGW under Wine).
 - **macOS (Mach-O) was not implemented.** Nothing in the request named
   it, and it would need a third format reader alongside
   `SymbolTable.cpp`'s ELF and PE branches. `readExportedSymbols()` is
@@ -351,7 +471,7 @@ if the new code adds a field to its type. This is a correctness
 property of your own code across the reload boundary, not something a
 symbol table (which carries no field-offset information) can check.
 If you need this checked, the extension point is the same
-place: `HotReloadContext::reload()` -- diffing the two builds' debug
+place: `Context::reload()` -- diffing the two builds' debug
 info (DWARF or PDB) for layout changes in types touched by patched
 functions is real work, genuinely useful, and out of scope for what's
 here now.
@@ -402,13 +522,16 @@ that file's comment for the full reasoning.
 
 ## Diagnosing a failed load
 
-`HOT_RELOAD_ERROR_LOAD_FAILED` alone doesn't say *why* -- a missing
-file, a permissions problem, a corrupt or wrong-architecture image,
-and a genuinely undefined symbol all look the same from the status
-code alone. Call `hot_reload_last_error(ctx)` right after a failing
-`hot_reload_load()`/`hot_reload_reload()` for the platform's own
-description (`dlerror()` on POSIX, a formatted `GetLastError()` on
-Windows).
+`Status::LoadFailed` (`HOT_RELOAD_ERROR_LOAD_FAILED` through the C API)
+alone doesn't say *why* -- a missing file, a permissions problem, a
+corrupt or wrong-architecture image, and a genuinely undefined symbol
+all look the same from the status code alone. `Result::message()`
+carries the platform's own description (`dlerror()` on POSIX, a
+formatted `GetLastError()` on Windows) directly -- or, through the C
+API, call `hot_reload_last_error(ctx)` right after the failing
+`hot_reload_load()`/`hot_reload_reload()` for the same thing (see "The
+C++ API: design and thread safety" above for exactly how that call
+stays correct even with more than one thread sharing a context).
 
 The specific case that motivated adding this, reproduced directly
 while writing it: a **non-inline `static` class member that is
@@ -478,15 +601,15 @@ fix this in any shared library, hot-reloaded or not.
      plugin uses this style -- compare the two examples' exported symbol
      lists (`nm -D --defined-only <path> | c++filt`) to see the
      difference directly.
-4. Have your host `hot_reload_load()` it once at startup and
-   `hot_reload_reload()` it after every rebuild, from a point where
-   nothing else is calling into it (see "What this cannot detect"
-   above).
+4. Have your host `load()` it once at startup (`hot_reload::Context`,
+   or `hot_reload_load()` through the C API) and `reload()` it after
+   every rebuild, from a point where nothing else is calling into it
+   (see "What this cannot detect" above).
 5. Rebuilding to the *same output path* every time is fine --
-   `HotReloadContext` stages every load through a fresh, uniquely-named
+   `Context` stages every load through a fresh, uniquely-named
    copy internally specifically so your build system's exact
    write pattern can't cause a stale reload (a known gotcha for this
-   style of tool; see `HotReloadContext.cpp`'s
+   style of tool; see `Context.cpp`'s
    `stagePathForLoad()`). On Windows, each staged copy backing a
    still-loaded module can't be deleted for as long as that module
    stays mapped -- the rest of the process's lifetime, by design --