RequestHeader::GetParamValue indexed parts[1] without checking the size. A query
parameter with no '=' at all ("?foo") makes SplitString return a single element, so
both the DEBUG_LOG and the assignment read off the end of the vector. Nothing calls
GetParamValue today, so this is latent rather than live, but it's driven straight
off the request line.
The 64-bit frame length was assembled with header[n] << 24 on uint8_t values, which
promote to int - a byte >= 0x80 in the top position shifts into the sign bit and then
sign-extends when widened to uint64_t. The resulting size was always rejected, just
by the wrong check and via signed overflow to get there. Cast first.
OutputSink::Block() had the same shape as the InputSink one this branch already
fixed: a broken socket is reported ready immediately and forever, so waiting on it
is a spin. Bail if the sink already knows it's broken.
Same root cause as the websocket spin this branch fixes, different symptom, so it
belongs with the InputSink EOF change rather than apart from it.
ReadBinaryUntilTerminator() returns 0 both when nothing has arrived yet and when
the peer is gone, and the multipart loop only exits on finding its terminator - so
a cancelled browser upload left a handler thread spinning forever. That also hangs
web server shutdown, since it joins its handler threads.
Now that InputSink reports EOF, ask it.
(Moved here from misc-correctness-fixes, which will be rebased on top of this.)
Follow-up to the previous commit, from Nemoumbra's questions - which found a worse
spin than the one that fix addressed.
InputSink couldn't tell "nothing right now" from "peer is gone": Fill() treats
recv() == 0 as no data and only sets hasError_ on a real error. Block() then waits
with WaitUntilReady(), which reports a closed socket as ready immediately and
forever, so TakeExact() looped on it without ever returning. A client that
disconnects with half a frame buffered - easy to do while blasting messages - put
the server in an infinite loop inside TakeExact, never even returning to Process().
Measured 7.95 CPU-seconds over 8 seconds; 0.08 after.
So: track EOF explicitly (sticky atEnd_, exposed as AtEnd()), and have Block() give
up when nothing more can arrive.
That information was being thrown away in three more places:
* Process() only tried to fill when the sink was already empty, so a disconnect went
unnoticed for as long as there were leftovers - and if those leftovers were a
partial frame, the read above never completed. Always fill, and close once the
peer is gone and we've consumed what it sent.
* ReadPending() uses TakeAtMost(), which returns 0 both for "nothing right now" and
"nothing ever again", and then reported success having consumed nothing. Ask the
sink which it was.
* Both TakeExact() call sites answered a failed read with POLICY_VIOLATION, blaming
the client for a protocol error when it had simply disconnected. Check the sink
and report ABNORMAL when that's what happened.
Also stop queueing data once our own close frame is queued. RFC 6455 5.5.1 forbids
data frames after a close, and beyond the protocol, anything appended afterwards
keeps the buffers non-empty and starves the "everything is flushed" check that ends
the connection. Observed the server pumping 167MB of log broadcasts after being
asked to close.
The repeated close-and-discard is now one helper.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2
Reported by Nemoumbra: the debugger server could get stuck in a tight select()
loop after a lot of traffic, burning a core.
Once OutputSink hits a real send() error it latches hasError_, after which Flush()
returns immediately without consuming anything, so out_->Empty() is false forever.
Process() waited for that to empty before finishing the close, kept the fd in the
write set, and select() reports an errored socket as ready every time - so it
returned true on every lap without ever making progress, and WebSocketDebuggerLoop
span. This needs sentClose_ to be set for it to be unrecoverable, since otherwise
the read side notices the disconnect and closes; a client that sends CLOSE (or
trips a protocol error) while output is backed up gets exactly that. Reproduced
with a client that queues ~120MB of responses, sends CLOSE, then resets the
connection without reading: 6.02 CPU-seconds over 6 seconds before, 0.06 after.
Treat an output error as fatal to the connection instead.
Also, select() returning -1 always returned true, so any error that doesn't fix
itself (a bad fd rather than EINTR) was a second busy-loop with no wait at all.
EINTR retries, everything else closes.
Finally, SendFlush() erased the drained bytes off the front of outBuf_ every lap.
With a backlog that's a memmove of the whole buffer per lap, i.e. quadratic in the
backlog, which burns CPU on its own while draining a slow client. Track a consumed
offset and only compact once the dead prefix is worth reclaiming.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2
CalculateDepthDraw guarded depthVertexCount_ with vertexCount, which is the
index count - but depthVertexCount_ grows by the number of decoded vertices,
which for an indexed draw can be far larger. Two draws with 6 indices spanning
40000 vertices each therefore passed the check and wrote ~700KB past the end of
depthTransformed_. It also never bounded depthIndexCount_ against depthIndices_
at all, which only has room for 3 indices per vertex slot while draws routinely
produce more. Pass the decoded count in separately and check both.
DepthRasterClipIndexedTriangles duplicated culling-disabled triangles twice: once
in the collect loop (added for Syphon Filter, #21498) and again in the output
stage, which was the older code and should have been removed then. So it emitted
four triangles per input triangle into buffers sized for one, and did twice the
rasterization work it needed to in that mode. Removed the output-stage copy, and
gave the function the output capacity so it stops when full - even at 2x, a
culling-disabled draw over ~32k indices doesn't fit.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2
Found by a review pass over Common/. Three of these affect code the JITs
actually emit today:
* ARM64 TryMOVI(8) returned true unconditionally ("can always do 8"), but MOVI
with an 8-bit element replicates imm8 into every byte, so it can only encode a
byte-uniform value. TryAnyMOVI always tries size 8 first, so it succeeded for
every constant. MOVI2FDUP(FLT_MAX) - VertexDecoderArm64's Jit_PosFloat - came
out as "movi v0.16b, #0xff", a quiet NaN, and since FMINNM/FMAXNM return the
other operand for a quiet NaN, the infinity clamp silently did nothing.
TryAnyMOVI's replication loop was also shifting by every bit position instead
of by multiples of the element size, and it now only tries an element size the
value actually repeats at. Regression test added.
* RISC-V SW()'s stack-pointer compression path called C_LWSP instead of C_SWSP,
turning a store into a load that clobbers rs2 whenever autocompress is on
(which RiscVJit and VertexDecoderRiscV both enable).
* LoongArch64 EncodeDFj passed the raw register enum instead of DecodeReg(fj),
so bit 10 was always set and MOVFR2GR_S emitted movfr2gr.d - live in the
LoongArch JIT's mfc1 and its FPU/vector compilers.
The rest have no callers today, but are wrong as written:
* ARM64: MOVI/MVNI computed the MSL cmode one too high (MSL #8 is 1100, not
1101); TryMOVI's MVNI-with-MSL branch passed the value instead of its
complement; TBZ/TBNZ put the register size in bit 31 where b5 belongs and
didn't mask the bit index to 5 bits; the LDR/LDRSW/PRFM literal form checked
the wrong mask for imm19 and wrote it unmasked; FCVTZS/FCVTZU's GPR-
destination branch skipped DecodeReg and derived the type field from the GPR
rather than from the float source.
* LoongArch64: LDPTR_D/STPTR_W/STPTR_D all passed Opcode32::LDPTR_W;
AMCAS_DB_D duplicated AMSWAP_DB_D's opcode; EncodeJK shifted rk by 5 instead
of 10; BYTEPICK_D masked its shift to 2 bits instead of 3.
* x64: VGATHERDPD/VGATHERQPS/VGATHERQPD used the wrong opcode/W combinations
(only VGATHERDPS was right).
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2
VulkanGraphicsContext::InitSurface() threw away VulkanContext::InitSurface()'s
VkResult and carried on, so a failed surface init surfaced as
_dbg_assert_(GetAvailablePresentModes().size() > 0) in the VKContext
constructor rather than as a graphics error with the usual backend fallback.
The vkCreate*SurfaceKHR failure path in ReinitSurface() didn't log anything
either, so the assert was the only trace of it.
Now ReinitSurface() logs and sets init_error_ for all three ways it can bail
(surface creation, ChooseQueue, present mode enumeration), InitSurface()
checks the result, and MainThreadFunc() passes the message back out instead of
writing it to a local it then drops - Windows/main.cpp was reporting
"Failed to initialize main thread function." to the user.
Also deletes the Application on that failure path, which was leaked.
The two long-standing bug reports had a shared root: the service loop could
end up in a state it never left.
- Exit hang: UPNP_CMD_EXIT was queued alongside port requests and only acted
on when it reached the front. A request that couldn't complete was never
popped, so exit sat behind it forever and join() blocked indefinitely.
Exit is a flag now, checked before anything else.
- CPU spin: wait_for() with a predicate returns immediately when the predicate
already holds, so a stuck queue head meant a tight loop. sceNetInet's bind()
queues UPnP_Add regardless of the setting, so this hit whenever UPnP was off
and a game used sockets. The loop always blocks now, and requests are dropped
while UPnP is off.
- Failed discovery was retried every 5s forever, each time a full 2s SSDP round
plus an error toast. Now backs off 5s -> 300s and reports once.
Other things found while in here:
- Every failed Initialize() leaked a UPNPUrls + IGDdatas, so ~every 5 seconds
for anyone with UPnP on and no router. The manual miniwget/parserootdesc/
GetUPNPUrls block was also redundant - UPNP_GetValidIGD does all of it and
memsets over the result, leaking the URLs and costing an extra HTTP round
trip per attempt.
- UPNP_GetValidIGD's status was never checked, so we could go DONE with no
usable IGD and hand a NULL controlURL to UPNP_GetConnectionTypeInfo.
- miniupnpc's strncpy into the port-mapping-entry buffers doesn't guarantee a
terminator; an 80-char description ran std::string off the end of desc[80].
- Add() marked another app's port "taken" only after our own add succeeded, so
a failed add left their mapping deleted and never restored.
- Clear() walked the router's entire table at exit, one HTTP round trip per
index. It now deletes only what we know we mapped, and the exit cleanup has
a time budget so an unreachable router can't stall shutdown.
- The in-flight request stayed in the queue during the router call, so a
same-port request arriving concurrently could erase it and be dropped
unexecuted.
- The queue is bounded, and last-write-wins per port collapses the churn from
games that rebind in a loop.
- The mapping description is built when the request is queued rather than read
off g_paramSFO from the UPnP thread later.
The thread now only exists while the setting is on - turning it off makes it
remove its mappings and exit, turning it on starts one. That means __UPnPInit()
has to run after the config is actually loaded; g_Config.Init() only builds a
lookup table. QueueRequest() reconciles too, so a per-game config or a libretro
core option enabling UPnP works without a notify at every call site.
The settings checkbox is disabled in-game, since sceNet latches related
settings at boot and a game that already mapped its ports wouldn't cope with
them disappearing.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2
The mangling encodes the source signature, so a demangler correctly prints
"Son::~Son()" - but the emitted function takes a second argument (a short in
a1) and returns "this". Anything deriving a function signature from the name
gets it wrong. Verified against both binaries: the flag is only tested for
being positive, which is what selects the operator delete call, and callers
pass -1 far more often than anything else.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01SF5eS5QDNexLksRDeDZvwY
The file holds the Hebrew strings in visual order, for renderers with no bidi
of their own, but only 101 of its 269 Hebrew entries were actually a reversal
of he_IL. The rest were left unreversed, reversed by word instead of by
character, or garbled outright ("Win" read as gibberish either way round).
Derived them all mechanically instead. The transform reproduces the entries
that were already right, and keeps as single units the things that must not be
spelled backwards: the \n escape, %N placeholders, runs of Latin and digits,
and an & accelerator together with the character it marks.
The [Dialog] save/OSK strings are the exception - he_IL already stores those
reversed, since PPGeDraw draws them and has no bidi, so the invert file just
mirrors he_IL for those 22 keys.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01R9fKXvYBrnqtp1QQGaGWVv
The old one was reverse engineered from a handful of symbols and got the
shape of the format wrong - it required a digit right after the kind
character, which most real symbols don't have. Measured against a PSP
executable that shipped with its symbol table intact, it decoded 238 of
4662 mangled symbols, most of those incorrectly.
Worked out properly from that binary, the format turns out to be:
__0 <kind> <name...> <params> [_ <return type>] [<qualifier>]
where the kind character (member function, free function, operator, data)
is the only thing that says how many name components follow, since nothing
separates the last one from the first parameter. Lengths are letters
(A = 0, a = 26); "5" marks an enclosing namespace; "7...._" is a template
argument list, with "4" plus a compact integer for a non-type argument and
"9<index>A" for a back-reference to one; "T<index>" and "N<count><index>"
repeat an earlier parameter; a trailing "K" is const and a trailing "T" is
a static member function. Also handles __TID_/__T_ (the two halves of a
class's RTTI) and __sti__ (a translation unit's static initializers).
That decodes 4661 of the 4662. The one holdout is an STL symbol whose
template argument is a reference to a member of another template.
Declarator wrapping is shared with the CodeWarrior demangler now, so
pointers to arrays come out as "short (**)[64]" in both.
docs/SNSystemsMangling.md describes the format, marking what's inferred
rather than attested.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01SF5eS5QDNexLksRDeDZvwY
div.s now maintains fcr31's Cause.Z and (when the trap is masked) sticky
Flag.Z bits, in the standard MIPS bit positions. Only a finite non-zero
dividend counts, so 0/0, inf/0 and NaN operands are excluded per IEEE 754.
When the guest has the trap unmasked, the new Core_FPUException() reports it
with the usual module suffix and MIPS call stack, and fd is left unwritten as
hardware would. That's gated behind a new developer setting, off by default:
PSP threads start with fcr31 = 0x00000e00, i.e. three of the traps already
enabled, and games divide by zero without meaning anything by it. The fcr31
bits are updated either way, so what the game reads back doesn't depend on
the setting.
Interpreter only - the JITs are unchanged, and none of this is reachable
under them.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01R9fKXvYBrnqtp1QQGaGWVv
Checked against two PSP binaries that shipped with intact symbol tables,
which turned up several constructs the format's usual description doesn't
mention:
- Template arguments are written literally inside the length-prefixed name
("39CList<Q38hlScreen5Brwsr13CContentsUnit>"), not with a "__PT" prefix,
and they nest. Function templates put theirs in the base name instead,
followed by the return type.
- A family of "@"-decorated symbols for things with no C++ name: thunks
("@12@__dt__3SonFv"), string literals, function-local statics and their
guard variables. Plus __vt__/__RTTI__/__sinit_, printed in the same style
as the Itanium special names.
- Types are now built as a split declarator, so a pointer to a function
comes out as "int (*)(int)" rather than "int (int) *".
Also stop the lenient pass from turning plain C names with a "__" in them
into nonsense - "I3dClut__FlushCache" became "I3dClut(long, ...)". It now
requires a class qualifier, which costs nothing: over ~10000 symbols the
lenient pass rescued none and only produced those false positives.
Symbol map names go from 128 to 256 characters, since a demangled name
keeps its parameters and templates make short work of 128.
docs/CodeWarriorMangling.md describes the format, marking the parts that
are inferred from cfront rather than attested in a real binary.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01SF5eS5QDNexLksRDeDZvwY
Older PSP binaries weren't built with GCC, so the Itanium demangler doesn't
help with them. Add two more, tried in turn by DemangleSymbolName():
- Metrowerks CodeWarrior, a descendant of the AT&T cfront scheme
("getDistance__6KzUtilFP7st_unitP7st_unit"). Handles Q<n> qualified names,
the cfront type codes including T/N back-references, cv-qualifiers, and the
operator/ctor/dtor name codes.
- SN Systems SNC/ProDG ("__0f5DstdIbad_castEwhatvK"), which encodes name
component lengths as letters. Reverse engineered from a small sample, so
the parts that are guesses are marked as such - they don't affect the name.
Both are rougher than the Itanium one: they aim for a correctly qualified name
plus a plausible parameter list, and print "..." for a parameter they can't
decode rather than throwing the name away. Results come back as a
DemangledSymbol with the name, parameters, return type and qualifiers kept
separate, in case a caller wants more than the printed string.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01EFV5DUTc9ZYAKgsCMZGwX8
GLRenderManager is documented as "emu thread records, render thread executes",
but GL has to record device object creation as init steps rather than just doing
it, and InitGPU() runs on the ExecLoader thread - GPU_GLES's constructor builds
DrawEngineGLES, whose InitDeviceObjects() reaches initSteps_ through
CreatePushBuffer and CreateInputLayout. The emu thread is still drawing the
loading screen into the same FastVec until the loader thread is joined, so two
concurrent push_uninitialized() can both reallocate, and one writes its step into
a freed buffer - losing a shader or buffer creation, or scribbling an owned
pointer into freed memory.
frameData_[].activePushBuffers is genuinely three-threaded too: inserted into by
whoever creates a push buffer, erased on the render thread via GLDeleter, and
walked on the render thread each frame.
A mutex each, uncontended in practice. Note this makes the existing access safe
rather than fixing the layering - Vulkan avoids the problem by creating objects
directly and deferring the rest to FinishInitOnMainThread, which GPU_GLES has
never had. Moving GL's device object creation there would be the better fix.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8
The loop queue-deleted the VkPipeline and nulled the slot without deleting the
Promise the array owns - DestroyVariantsInstant right below it shows the intended
ownership. That's one leaked Promise per destroyed variant per cached pipeline,
on every MSAA or resolution change.
It also wrote pipeline[] without taking mutex_, which the header documents as
protecting that array and which the render thread holds while reading and
replacing the same slots in PerformRenderPass. The two have to be fixed together:
the missing delete was the only thing keeping this a leak rather than a
use-after-free.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8
GetRenderPass() looks up and inserts into renderPasses_ from the main thread
(EndCurRenderStep, CreateGraphicsPipeline) and from the render thread
(PerformBindFramebufferAsRenderTarget), unsynchronized. The render thread really
does insert rather than only hit: PreprocessSteps rewrites the load actions to
CLEAR when it merges a clear-only pass into a later one, after the main thread
already looked up the pre-merge key. DenseHashMap::Insert can Grow(), which
reallocates the buckets out from under a concurrent Get().
VKRRenderPass::Get() has the same problem one level down - it creates the passes
lazily and is called from both threads on the same object, so two threads hitting
an empty slot each create a pass and one gets overwritten and leaked, while the
sample-count branch can queue a pass for deletion that the other thread is about
to hand to vkCreateGraphicsPipelines.
A mutex each. Handing the VKRRenderPass pointer out from under the map lock is
fine, entries are only ever erased all at once in DestroyDeviceObjects.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8
Core_ProcessStepping() returns immediately when the CPU is stopped with nothing
queued, so Core_RunLoopUntil() returns immediately, so whatever drives it comes
straight back. headless does that in a loop with no frame pacing at all, so a
paused emulator sat at 100% of a core: measured 6.02 CPU-seconds over 6 wall
seconds parked at startBreak. A debugger session is stopped most of the time, so
this also dominated any profile taken of one - showing up as synchronization
overhead around Core_RunOnCPUThread, which was just the hottest thing inside the
spin rather than a problem with the queue.
The CPU thread now blocks on a condition variable in that case. Anything that
gives it something to do wakes it - Core_RunOnCPUThread() on push (with the
queue mutex held, so it can't sleep on a task already queued),
Core_RequestCPUStep(), and Core_Resume() - so the 2ms timeout is only a backstop
for state changed without a wake, never how work is normally noticed.
The wait is deliberately short rather than indefinite: callers do real work after
Core_RunLoopUntil() returns, and in the app build that includes rendering the
ImGui debugger from this same thread, so this has to bound how long a paused
frame takes rather than replace the frame loop.
Now 0.05 CPU-seconds over the same 6 seconds. No measurable cost to anything
else: an identical scripted boot runs in 2514ms vs 2476ms before, and 20
consecutive cpu.stepInto still complete promptly. 55 unit tests pass, 314/314
pspautotests with --graphics=software.
Also: wsdbg's README claimed a raw JSON line gets a ticket auto-assigned when it
lacks one. It doesn't - the code deliberately sends raw lines exactly as written,
and omitting the ticket is how you say "not waiting for an answer". Corrected.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Henrik Rydgård
keycross
Nab