Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support text-based non-fatal errors via E,errtext packet #162

Open
daniel5151 opened this issue Jan 6, 2025 · 0 comments
Open

Support text-based non-fatal errors via E,errtext packet #162

daniel5151 opened this issue Jan 6, 2025 · 0 comments
Labels
API-ergonomics Nothing's "broken", but the API could be improved design-required Getting this right will require some thought good first issue Good for newcomers help wanted Extra attention is needed new-protocol-extension New feature or request

Comments

@daniel5151
Copy link
Owner

Support for text-based non-fatal errors seems to be a relatively recent addition to GDB. Indeed, the qSupported feature (error-message) which signals support for this packet variant was only added in June 2024, via bminor/binutils-gdb@ddb3f3d

Allowing Target implementations to report detailed error messages directly back to the GDB client seems incredibly useful, so this is a feature that gdbstub should definitely implement.

Of course, the devil is in the details, and actually implementing support for these sorts of error messages may be tricker than expected...

TL;DR:

  • Basic support for &'static str and String-based error messages should be easy to implement, and wouldn't require any API breaking changes. It would be a great beginner friendly PR!
  • Advanced support for dynamic error messages on no_std + no_alloc targets will be a lot trickier to support, and more API design work is required

Limitations on error message content

One pitfall of the GDB implementation is that - from my superficial reading of the spec - the choice to use inline ASCII in the error packet means that the error message cannot include the $ or # chars. It would be good to confirm this, and possibly open an issue on the GDB bug-tracker so they can update their docs / implementation.

Dealing with older clients that don't support textual errors

Even though the underlying text-based error packet doesn't report a error code (it simply returns E,errtext, as opposed to EXX,errtext), it seems likely that we'd nonetheless want Target implementations to specify an error code alongside their error text, just-in-case they're communicating with an older client that doesn't support error strings.

That said, there's nothing stopping us from also including a NonFatalMsg-like helper variant, which doesn't require a user to explicitly specify an error code, and simply defaults to using errno 121 (AKA: EREMOTEIO). This is similar to how the existing NonFatal variant works.

Supporting dynamic error messages on no_std + no_alloc platforms

While it would certainly be straightforward to add a TargetError::ErrnoWithMsgString(u8, String) variant on std / alloc targets, and a TargetError::ErrnoWithMsgStaticStr(u8, &'static str) variant for no_std targets... it would be really nice if there was also a way to allow incrementally building an error-string without an intermediate allocation, using an API like the monitor_cmd extension's ConsoleOutput object.

Some ideas on how that might be possible:

  • We do some massive surgery across the gdbstub codebase in order to plumb through an additional &mut ErrorStringOutput parameter, as well as coming up with a scheme that prevents existing callback-based APIs from intermingling their streaming output alongside error output*. This has the benefit of allowing implementations to build up an error string using whatever transient data they currently have access to in their stack frame, without needing to "stash" it anywhere for gdbstub to later access.
  • We add a TargetError::NonFatalDeferred variant, with some corresponding top-level state-machine APIs to push out a dynamically constructed error. This would allow the stub to stash away an implementation-specific error type somewhere in &mut self prior to returning the NonFatalDeferred error, and then using that stashed error to construct an error string later, in its top-level error handling logic. This is ergonomically unfortunate, and requires keeping around a persistent Option<TargetSpecificNonFatalError> buffer in memory... but it is certainly the easiest approach to "bolt on" to the current gdbstub infrastructure.

I'm not thrilled by the tradeoffs in either of these approaches.

That said, these are just two ideas that come to mind, and I'm by no means claiming these are the only two approaches we can take here. More thought is required here, for sure.

*though, now that I think about it... that might already be an issue with current API designs. I should double-check that...


I personally wouldn't block landing support for error-strings on solving the tricky no_std + no_alloc dynamic error message question. A v1 implementation should just keep things simple, and add an alloc-gated TargetError::ErrnoWithMsg(u8, Cow<'static, str>) variant, with a helper method to easily construct it from either a dynamic String, or a static &'static str.

Once that support lands, we can independently mull over how we'd want to support dynamic error strings in no_std + no_alloc scenarios.


Lastly, I should also note that LLDB has had a similar extension to the GDB RSP for quite a while now.

They use a QEnableErrorStrings packet to signal support, and then expect a slightly different error-packet variant from GDB when reporting textual errors. LLDB's textual error packets take the form of EXX;errtext, where errtext is hex encoded ASCII, as opposed to GDB's choice to use native ASCII in the packet.

I would strongly prefer if we implemented support for both the new native GDB variant and the existing LLDB variant simultaneously.

One consideration to keep in mind: What packet variant should be sent if we detect both a error-message feature in qSupported, as well as a QEnableErrorStrings packet? Presumably, we'd need to offer some kind of policy-choice to the Target implementation, allowing it to choose which packet variant it prefers to send back to the client. I'm not sure if its reasonable to hard-code one over the other, as the two packet variants made different tradeoffs with their respective encodings - LLDB supporting all of ASCII, at the expense of extra payload length, vs. GDB supporting all of ASCII except # and $, but avoiding hex-encoding overhead.

@daniel5151 daniel5151 added new-protocol-extension New feature or request help wanted Extra attention is needed good first issue Good for newcomers API-ergonomics Nothing's "broken", but the API could be improved design-required Getting this right will require some thought labels Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-ergonomics Nothing's "broken", but the API could be improved design-required Getting this right will require some thought good first issue Good for newcomers help wanted Extra attention is needed new-protocol-extension New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant