This is an archive of the discontinued LLVM Phabricator instance.

FreeBSD ARM support for software single step.
ClosedPublic

Authored by dmikulin on Oct 18 2016, 4:28 PM.

Details

Summary

Implementation of software single step for FreeBSD on ARM.
The code is largely based on the Linux implementation of the same functionality.

I ran lldb test on an rpi2 board running FreeBSD 11.RC3. With the change the pass rate improves from the current 174
tests to 419. I used amd64 as a reference architecture where 517 tests pass on FreeBSD.

This is my first attempt to submit a patch for lldb, so I may need some guidance to work through the process.
How do reviewers get assigned for code reviews?

Diff Detail

Repository
rL LLVM

Event Timeline

dmikulin updated this revision to Diff 75086.Oct 18 2016, 4:28 PM
dmikulin retitled this revision from to FreeBSD ARM support for software single step..
dmikulin updated this object.
dmikulin set the repository for this revision to rL LLVM.
dmikulin added a project: Restricted Project.
dmikulin added a subscriber: lldb-commits.
dmikulin updated this revision to Diff 75090.Oct 18 2016, 4:35 PM

Fixed indentation.

labath added a subscriber: labath.Oct 19 2016, 3:42 AM

I'd love to see freebsd switch to lldb-server based debugging. Then we could make sure this code is actually reused instead of copied around (e.g. we are right now preparing a patch to improve this, which you could have gotten for free if the code was shared). </drive-by>

Are you saying someone is working on switching FreeBSD to lldb-server debugging? Can I get my hands on this patch?

No, I'm saying someone *should*. :P

Ed looked into that at some point but, I don't think get got too far with it. Adding @emaste, who should probably review this.

emaste edited edge metadata.Oct 20 2016, 9:55 AM

No, I'm saying someone *should*. :P

Ed looked into that at some point but, I don't think get got too far with it. Adding @emaste, who should probably review this.

Yes, it needs to be done. Maybe now that NetBSD support is progressing a few of us can work together to make it happen.

On a quick look this seems OK. I'll try to test/review in detail.

source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
925 ↗(On Diff #75090)

typo here

I'm not familiar enough with the code yet to pick up the work on moving FreeBSD to lldb-server. Any pointers to revision numbers, internal discussions, etc, related to the Linux lldb-server switch? Also, if there's any unfinished FreeBSD work, it would help me get started. Thanks!

I don't know of any good source of documentation for this, but I am willing to answer any questions you might have about this. Basically, the idea of lldb-server is that you use the same code path for remote debugging as you to for local. So all the debugging is done in a separate process called lldb-server, with which the client then communicates when it needs something done.

If we want to get lldb-server working on freebsd, I would propose a plan like this:

  • rename NativeProcessLinux into something more generic (NativeProcessPosix ?), make a new NativeProcessLinux, which inherits from that, but contains no code.
  • get NativeProcessPosix building on FreeBSD. Now, you will likely get compile errors as we may be using some OS interfaces that don't exist on freebsd. We will need to figure out how we can either: a) use interfaces which are available on both platforms; b) abstract the functionality so it can implemented in the derived classes. The biggest offender here will be ptrace (at one point we used signalfd(), but that is gone now), as it operates a bit differently on the two OSs. For the implementation of the FreeBSD part of it, you can use the existing ProcessFreeBSD classes for inspiration. All the other
  • get lldb-server test suite passing on FreeBSD (these are all the tests in tools/lldb-server), which test pure server functionality)
  • switch lldb client to use lldb-server instead of ProcessFreeBSD
  • get the rest of lldb tests passing
  • remove ProcessFreeBSD

When we were getting lldb-server working for linux, it really helped having osx as a reference, because it already used the same flow. I imagine that it could be quite helpful for you as well, if you could get a linux machine/VM to be able to compare how the code behaves on two platforms, but I think it should be possible to do it without it.

So, if you want to get started, I propose you do a preliminary invetigation -- set the build system so that in builds NativeProcessLinux on FreeBSD, and see how much code do you have to delete, so that it compiles. This should give us an idea of the scope of the work.

good luck :)

dmikulin marked an inline comment as done.Oct 21 2016, 10:55 AM

Thanks Pavel! I'll start working on it. Do you know when lldb-server Linux changes were committed? I want to use those patches as a template, but it's hard to locate them digging through thousands of commit log entries...

Ed, you mentioned NetBSD work. Do you know where they are in their implementation? Anything I can do to help? Or should I start from scratch?

On a quick look this seems OK. I'll try to test/review in detail.

Thanks Pavel! I'll start working on it. Do you know when lldb-server Linux changes were committed? I want to use those patches as a template, but it's hard to locate them digging through thousands of commit log entries...

I recalled discussing this when the Linux changes were going in and wanted to point you at a mailing list archive, but it seems it was only in private mail. I'll try to send you the relevant threads.

Have a look at rL212069 for starters.

Ed, you mentioned NetBSD work. Do you know where they are in their implementation? Anything I can do to help? Or should I start from scratch?

I hope @krytarowski can comment on that.

It's probably best to move this discussion to a new thread on lldb-dev.

NetBSD is still focused on amd64-only port.

I start a funded project November 1st on getting x86 process plugin to work. More details here:

http://blog.netbsd.org/tnf/entry/funded_contract_2016_2017

Estimated time is 4 months full-time.

However there every help is appreciated and donations to TNF (The NetBSD Foundation) make it possible.

@emaste What's the status of remote debugging on FreeBSD?

krytarowski added a comment.EditedOct 21 2016, 12:12 PM

I just scrolled the discussion so it's not really started. Thanks.

My plan is to work on ptrace(2) for about a month.

Then I will take the FreeBSD code as it is and port to NetBSD trying to make it functional. This is difficult part as there are everywhere OS specific interfaces and behavior. I hope to get it to some usable point after a month.

Then ,I will start working on remote debugging, as my previous patch for process tracing (it was buggy) was rejected without this property. I planned to merge my previous work with master as I was forced to keep catching up after the project changes and it was time expensive, and move my focus on proper platform support.

So far all my efforts were voluntary and NetBSD build bot is paid from my private resources. Now I will be able to spend on in full-time about 4 months.

So to make it short, hopefully I can join you in remote capabilities after New Year (according to my estimations).

emaste added inline comments.Nov 30 2016, 12:41 PM
source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
551–557 ↗(On Diff #75090)

according to LLVM style this should be promoted out of an else block because of the return above (use early exits / don't use else after a return).

As an aside it appears 0xe7fddefe is ARM-recommended breakpoint opcode and Linux is the outlier. So the generic Platform::GetSoftwareBreakpointTrapOpcode ought to use 0xFE,0xDE,0xFF,0xE7, with PlatformLinux::GetSoftwareBreakpointTrapOpcode having the override?

source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
957 ↗(On Diff #75090)

Few typo / nits:

only fills in
s/regsiter/register/
in some cases

1063 ↗(On Diff #75090)

We should really be returning an Error from this fn instead, no?

1101–1104 ↗(On Diff #75090)

Hope you don't mind a minor rewording of the comment:

The emulated instruction failed and it did not change the PC. Advance the PC by the size of the current opcode, as all PC-modifying instructions should be successfully emulated. The failure was most likely caused by an unsupported instruction.

dmikulin updated this revision to Diff 79987.Dec 1 2016, 2:52 PM
dmikulin edited edge metadata.

Addressed review comments.
Haven't had a chance to re-test it on an arm board.

@emaste , any chance to get this committed soon?

This revision was automatically updated to reflect the committed changes.