-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Gdb server #2168 #2333
Gdb server #2168 #2333
Conversation
Hi @kumargu. The gdb changes should be pointed towards the branch called |
fd53103
to
ae7f807
Compare
16a29b0
to
69eba2c
Compare
Hi @kumargu Wanted to know if you are still interested to contribute to this PR? Is there any help needed from us to get you unblocked? |
Hi, |
42f69f2
to
cce931a
Compare
Hi @kumargu. Seems that the commits should be signed. Please do that and resubmit. |
Ack. |
9dc6061
to
0d95863
Compare
}; | ||
|
||
pub use gdbstub::GdbStubError; | ||
#[allow(unused_imports)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove #[allow(unused_imports)]
?
The purpose of imports is to have a clear understanding of what is used from other crates, but here we're just importing everything.
|
||
extern crate vm_memory; | ||
|
||
#[allow(dead_code, unused_imports)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for allow(dead_code)
.
#[allow(dead_code)] | ||
fn wait_for_tcp(port: u16) -> DynResult<TcpStream> { | ||
let sockaddr = format!("0.0.0.0:{}", port); | ||
eprintln!("Waiting for a GDB connection on {:?}...", sockaddr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of eprintln
we could add these messages to the logger.
let mut debugger = GdbStub::new(connection); | ||
match debugger.run(&mut target)? { | ||
DisconnectReason::Disconnect => { | ||
println!("Disconnected from GDB."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
println
statements could also be directed through the logger.
pub fn insert_bp(&mut self, linear_addr: u64, translate: bool) -> Result<(), DebuggerError> { | ||
// Opcode specific to x86 architecture that triggers a trap when | ||
// encountered during cpu execution | ||
let int3: u8 = 0xCC; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put this as a constant somewhere.
let mut opcode: Option<u8> = None; | ||
if self.breakpoints_linear.contains_key(&linear_addr) { | ||
let val = self.breakpoints_linear.get_mut(&linear_addr).unwrap(); | ||
if !val.2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these tuples represent? Can we add a comment explaining the logic?
sorry for the delay, will pick the comments this week. |
Anything I can do to help here? I might have some free time to help push this over the line if you'll point me in the right direction. |
Hi @LegNeato! We are open to external contributors here as we don't have any work planned for this functionality. The to-do items here would be:
The items above are not enumerated by priority. I'd say that (1), (5) and optionally (2) are must have to start working towards merging the PR. If you're interested in working on any of these items I propose that we discuss here or on our slack server together with @kumargu. Let us know what you think! |
81dc415
to
f6d7cd9
Compare
f6d7cd9
to
5e973ea
Compare
Did a comparison of translated address using the current mechanism (page walks + reading phdr headers) vs multi level page walks. Some logs -
Breakpoints -
|
Great to see this move forward @kumargu! |
I further did some more analysis 🧐 translations through page walks fails only for addresses mapped to early bootup phase, because there is no notion of virtual address. To handle this baciumar@ had this nice logic to look at elf headers and adjust the addresses -- similar to what the linux kernel does [link]. we would debate if we need this logic of parsing elf headers or just throw an exception if Firecracker GDB debuggers try to access memory regions in the early boot up phase. |
@kumargu so page walking works after the mapping is done? |
@gc-plp Yes, it works after the mapping is done. there was a small check that was missing - I will update the PR. |
5c75782
to
b088770
Compare
Update -
Next steps -
|
b088770
to
e946840
Compare
We will close this PR for the moment. @kumargu feel free to reopen if you have the bandwidth to resume work here. |
Is there a roadmap / list of code changes required for gdb-server to work on Firecracker? |
Reason for This PR
Initial implementation of a GDB server for guest debugging (#2168)
Description of Changes
Created a new package implementing the gdbstub interface for client-server communication
Implemented the following functionalities: breakpoint, single-step, reading of registers and memory
Implemented virtual-to-physical address translation, used by any guest memory access performed as part of the above-mentioned functionalities
Created a new thread for handling client requests and two channels for vCPU - GDB server thread communication
Added a "--debugger" command-line option for the firecracker executable
This functionality can be added in
rust-vmm
.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.
PR Checklist
[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]
git commit -s
).unsafe
code is properly documented.firecracker/swagger.yaml
.CHANGELOG.md
.