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

Add "b" prefix when using DREQ_GET_MEM_BINARY #3902

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

khang06
Copy link

@khang06 khang06 commented Jan 21, 2025

Fixes #3901.

GDB errors out if the response doesn't start with "b" (see https://github.com/bminor/binutils-gdb/blob/master/gdb/remote.c#L9739). This works on GDB 16.1 and LLDB 19.1.7, but I haven't tested anything else.

@khuey
Copy link
Collaborator

khuey commented Jan 22, 2025

It appears older LLDB is unhappy with this.

@khang06
Copy link
Author

khang06 commented Jan 22, 2025

It appears older LLDB is unhappy with this.

Hmmm, the failing tests also break on latest LLDB too. It seems like LLDB doesn't skip the first character even though the protocol specification says it's required. Not really sure what to do about that other than disabling binary memory read/write command support, since LLDB appears to have a fallback path for when that isn't supported.

@rocallahan
Copy link
Collaborator

So GDB implemented this in a completely incompatible way to LLDB?

@rocallahan
Copy link
Collaborator

Yes, OK, that's what they did.

@khang06 can you contact the GDB people or file a GDB bug and tell them that they've implemented this incompatibly with LLDB and now all third-party gdbservers that implemented this in an LLDB-compatible way are broken?

It would be nice if they could fix this by accepting output either with or without the 'b', but that doesn't really work because 'b' could be legitimate data :-(.

If we have to we can work around this in rr by having an "LLDB mode" flag and setting that flag automatically by detecting use of LLDB-specific extensions, but it would really suck for all already-installed versions of rr to not work with gdb 16.1. Seems better to issue a gdb 16.1 point release as soon as possible.

@rocallahan
Copy link
Collaborator

Yeah so I think we should ask the GDB folks if they are willing to change to be LLDB compatible here.
Whether or not they agree to that, I think we should add an LLDB mode flag to rr with a command-line option to control it, and require LLDB users to explicitly opt-in to the LLDB flavour protocol. We should disable support for LLDB's extension packets when that flag is not set. Then if GDB makes more incompatible extensions (or LLDB does) hopefully rr won't break.

@rocallahan
Copy link
Collaborator

Yeah so I think we should ask the GDB folks if they are willing to change to be LLDB compatible here.

I'm doing this.

@khang06
Copy link
Author

khang06 commented Jan 22, 2025

Could you make a GDB 16.1 point release that removes the 'b'? AFAIK it serves no purpose.

I think that's there to differentiate successful cases from the "E" prefix error cases according to the protocol page.

@rocallahan
Copy link
Collaborator

@rocallahan
Copy link
Collaborator

I think that's there to differentiate successful cases from the "E" prefix error cases according to the protocol page.

Good point, that makes sense.

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.

Backtrace in Debian gdb 16.1-1 is broken
3 participants