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

do not fail on unknown HostIoOpenFlags #98

Closed
wants to merge 2 commits into from

Conversation

mrk-its
Copy link
Contributor

@mrk-its mrk-its commented Feb 19, 2022

Description

gdbstub fails when unknown file open flags are passed with:

gdbstub encountered a fatal error: Could not parse the packet into a valid command: MalformedCommand

This bug appeared with lldb sending platform put-file command, appending following File:open arguments:
612e6f7574,40000601,000001ed

The simple solution is to use from_bits_truncate to ignore unknown flags.

API Stability

  • This PR does not require a breaking API change

Checklist

  • Implementation

    • cargo build compiles without errors or warnings
    • cargo clippy runs without errors or warnings
    • cargo fmt was run
    • All tests pass
  • Documentation

    • Updated CHANGELOG.md

@daniel5151 daniel5151 self-requested a review February 19, 2022 01:22
@daniel5151
Copy link
Owner

While this PR does solve your immediate issue, it is not a proper solution to the problem.
Namely, it seems that the LLDB RSP deviates from GDB RSP wrt how it handles vFile packets.

See: https://github.com/llvm/llvm-project/blob/ec642ceebc1aacc8b16249df7734b8cf90ae2963/lldb/docs/lldb-platform-packets.txt#L6-L11

I'll try to find some time soon to think of the best way to work around this issue, but unfortunately, I cannot merge this PR in its current state.

Note that as it stands, gdbstub doesn't explicitly support lldb, so this isn't strictly a "bug" as it is a new feature request (i.e: supporting LLDB).

@daniel5151 daniel5151 closed this Feb 19, 2022
@daniel5151
Copy link
Owner

That said, I sincerely appreciate the PR, and for bringing this issue to my attention.

@mrk-its
Copy link
Contributor Author

mrk-its commented Feb 19, 2022

np, so I'll stay with my fork

@daniel5151
Copy link
Owner

daniel5151 commented Feb 19, 2022

I did a bit more digging and found this:

https://github.com/llvm/llvm-project/blob/ec642ceebc1aacc8b16249df7734b8cf90ae2963/lldb/include/lldb/Host/File.h#L47-L66

Look like LLDB's flags are a strict superset of GDB flags, which means it should be possible to support these additional flags without breaking existing GDB implementation / requiring any breaking API changes.

If you'd like to send a PR that adds these additional variants, I'd be happy to merge that one.

@daniel5151
Copy link
Owner

oh, wait, maybe not actually...

looks like this was only a recent change, and older versions of lldb will use different enum values: llvm/llvm-project@8bbef4f

I suspect we'd want to support older clients as well, which means this continues to be a tricky problem to solve nicely...

@mrk-its
Copy link
Contributor Author

mrk-its commented Feb 19, 2022

oh, wait, maybe not actually...

looks like this was only a recent change, and older versions of lldb will use different enum values: llvm/llvm-project@8bbef4f

I suspect we'd want to support older clients as well, which means this continues to be a tricky problem to solve nicely...

Currently gdbstub fails for both old and new versions, so adding support for new ones only would be a big improvement :)

@daniel5151
Copy link
Owner

Yeah, you know what - fair enough! We can punt the hard problem of supporting older LLDB clients down the line, and get you unblocked for now in a non-breaking way.

So sure, feel free to send a PR my way with these new variants :)

@daniel5151
Copy link
Owner

FYI, I've opened a tracking issue for the broader LLDB compatibility story over at #99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants