Page MenuHomePhabricator

[lldb] [Process/FreeBSDRemote] Introduce mips64 support
ClosedPublic

Authored by mgorny on Feb 1 2021, 10:47 AM.

Details

Summary

Introduce mips64 support to match the legacy FreeBSD plugin. Similarly
to the legacy plugin, the code does not support FPU registers at the
moment. The support for them will be submitted separately as it
requires changes to the register context shared by both plugins.

This also includes software single-stepping support that is moved from
the Linux plugin into a common Utility class. The FreeBSD code also
starts explicitly ignoring EINVAL from PT_CLEARSTEP since this is easier
to implement than checking whether hardware single-stepping were used.

Diff Detail

Event Timeline

mgorny created this revision.Feb 1 2021, 10:47 AM
mgorny requested review of this revision.Feb 1 2021, 10:47 AM
mgorny updated this revision to Diff 320524.Feb 1 2021, 10:50 AM

Remove #if 0s.

labath added a comment.Feb 4 2021, 5:26 AM

The software single stepping code looks very similar to the linux version. Have you considered merging the two implementations? The code looks generic enough to be placed into the base class, if needed...

mgorny added a comment.Feb 4 2021, 5:37 AM

Same way I did x86 watchpoints? I suppose I could try that.

Merging is a good idea as NetBSD might duplicate these code chunks too.

labath added a comment.Feb 4 2021, 5:54 AM

Same way I did x86 watchpoints? I suppose I could try that.

Possibly. Depends on the implementation, I guess. I was thinking we could just shove this into the NativeProcessProtocol class. Though, now I see that would complicate layering as that class is in Host. However, there's no hard reason why that class has to be in Host (it's pretty different than the rest of host functionality), so we could also solve that by moving it some place else....

mgorny updated this revision to Diff 321446.Feb 4 2021, 8:28 AM
mgorny edited the summary of this revision. (Show Details)

@labath, how does this look like? I've managed to make it into a mixin class without any virtual inheritance.

labath added a comment.Feb 6 2021, 5:51 AM

It doesn't look too bad, but what's up with the friend thingy? What's that used for? Can we get rid of it?

mgorny added a comment.Feb 6 2021, 6:23 AM

It doesn't look too bad, but what's up with the friend thingy? What's that used for? Can we get rid of it?

It's needed to access SetSoftwareBreakpoint(), though I'm not really sure why that method is not public. Maybe it'd be better to mark it public instead.

labath added a comment.Feb 6 2021, 6:28 AM

It doesn't look too bad, but what's up with the friend thingy? What's that used for? Can we get rid of it?

It's needed to access SetSoftwareBreakpoint(), though I'm not really sure why that method is not public. Maybe it'd be better to mark it public instead.

The public interface is SetBreakpoint(..., hardware=false), though I can't say I am particularly fond it...

mgorny updated this revision to Diff 322088.Feb 8 2021, 5:45 AM

Switch to SetBreakpoint(). remove friend and refactor size_hint to a variable while at it.

mgorny added a comment.Feb 8 2021, 5:45 AM

@labath, done. Does it look reasonably good now?

labath accepted this revision.Feb 8 2021, 5:56 AM

Ok, seems reasonable.

lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
20

This should be an anonymous namespace.

This revision is now accepted and ready to land.Feb 8 2021, 5:56 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 9:27 AM
Herald added a subscriber: jrtc27. · View Herald Transcript