Make plugin API cleanup functions noexcept - #13590
Draft
bryancall wants to merge 2 commits into
Draft
Conversation
Exceptions have never been a supported error channel across the plugin API; failures are reported through return values. This writes that contract down for the functions plugins commonly call during cleanup (destroy/free/release/close/cancel, the mutex family, and TSError): - Annotate 28 API functions noexcept in ts.h and their definitions. - Convert each definition to a function-try-block so an internal exception that would have escaped now aborts with a message naming the function, instead of terminating with no context inside a plugin destructor (destructors are implicitly noexcept). - Annotate ts::do_abort [[noreturn]] noexcept and contain formatting failures inside it and inside ts::shared_mutex::_call_fatal, whose message formatting allocates and could itself throw before the abort. - Update matching prototypes in the developer guide and state the no-exceptions contract in the plugin getting-started chapter.
Contributor
Author
|
[approve ci autest 2] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Exceptions have never been a supported error channel across the plugin API: API functions report failure through return values, and no header or document has ever promised that a TS API call throws anything a plugin could catch. That contract was never written down, though, so any exception escaping from the core into plugin code is both undiagnosable and invisible to static analysis.
It matters most in destructors. Plugin destructors routinely call the API cleanup functions, destructors are implicitly
noexcept, and so an exception escaping one of those calls terminates the process with no attribution at all.What this does
noexceptints.h, the cleanup/teardown family that destructors call:TSContDestroy,TSHandleMLocRelease,TSIOBuffer*/TSMBufferDestroy,TSHttpHdrDestroy,TSMimeHdrDestroy,TSVConn{Close,Abort,Shutdown},TSActionCancel,TSCacheKeyDestroy,TSSslContextDestroy,TSFetchDestroy,TSTextLogObject*, theTSMutex*family,TSThread{Wait,Destroy},TSContData{Get,Set}, andTSError).ink_abort("exception escaped %s", __func__). An exception that would previously have escaped now produces a named, attributable abort instead of a barestd::terminateinside a plugin destructor.ts::do_abort[[noreturn]] noexceptand contains formatting failures inside it, and does the same forts::shared_mutex::_call_fatal. Both build their message withswoc::bwprint/Strerror, which allocate, so the abort path could itself throwbad_allocbefore reaching the abort, from inside~write_guard/~read_guard.Overhead
None on the happy path, as expected for table-driven exception handling. Comparing generated code for a representative wrapped function against the same function unwrapped, the instruction sequences are identical except that the final call can no longer be a tail call. Measured on
InkAPI.ccbuilt before and after with identical flags:__textgrew 896 bytes (+0.66%, about 45 bytes per wrapped function) and__gcc_except_tabgrew 500 bytes of read-only data that is never touched unless an exception unwinds.In the other direction this is a small optimization enabler: callers that can see
noexceptno longer need their own landing pads around these calls, so plugin cleanup code should get marginally smaller.Coverity
This should resolve 83 Coverity
UNCAUGHT_EXCEPTdefects.The checker flags this pattern because it has to assume any call in a destructor can throw. I arrived at that number by parsing the body of every destructor it flags and classifying what each one calls: 80 whose only calls are functions this PR makes
noexcept, plus 3ts::shared_mutexguard destructors covered by the_call_fatalfix. By area that is 61 in plugins, 13 intscpp/api, 5 in core, 2 in examples, and 2 in test code.Treat 83 as a projection rather than a measurement. It assumes the checker credits a
noexceptcallee and stops reporting the caller, which is the documented behavior but is not something I can confirm without a scan of the merged tree. The first analysis run after this lands will give the real figure, and I will follow up with it.Destructors whose throw path is a member subobject rather than an API call are not affected by this change and need their own root cause, as do the CLI tool entry points. Those are follow-up work.
ABI
Adding
noexceptdoes not change the ABI. In the Itanium C++ ABI an exception specification is part of a function type but is not encoded in the mangled name of a function declaration, and it does not affect the calling convention.I verified this rather than assuming it. Building
src/api/InkAPI.ccbefore and after with identical flags, both objects export 748 text symbols with zero name differences:TSContDestroyis_Z13TSContDestroyP10tsapi_contin both,TSErroris_Z7TSErrorPKczin both. As an end-to-end check, a caller compiled against the old declaration and linked against the newnoexceptdefinition resolves to the same symbol, links, and runs. So an existing plugin binary keeps working against a rebuiltlibtsapiand vice versa.Function pointers are also unaffected: a
noexceptfunction converts implicitly to a plain function-pointer type, sovoid (*p)(TSCont) = TSContDestroy;still compiles. Mangling would only shift if a parameter or template argument were spelled as anoexceptfunction type, which nothing in the API does. Nothing in the tree takes the address of any annotated function.The compatibility cost is at the source level, not the binary level, and is described below.
Notes for reviewers
catchexpecting to catch a core-internal exception would now see an abort. Exceptions were never a supported error channel here, so this standardizes existing practice rather than removing a documented one. It is a real change, though, and worth a release note.noexcepthas been part of the function type since C++17, so any translation unit that includests.hand separately declares one of these functions withoutnoexceptbecomes ill-formed. This applies equally to a plugin built as C++17 or C++20. Seven in-tree test stubs that redefine API functions needed the annotation added; out-of-tree plugins doing the same will need the same one-word change. The stub edits are mechanical and have to land in the same commit or the build breaks, which is why they are here rather than split out.TSMutexDestroy,TSMutexLockTry,TSMimeHdrDestroy,TSSslContextDestroy) so the boundary is not an artifact of a scanner hit list. Widening it to the whole API surface is a reasonable follow-up if reviewers prefer that; it did not seem like the right first step.