This is an archive of the discontinued LLVM Phabricator instance.

FreeBSD watchpoint support
ClosedPublic

Authored by jwolfe on Jan 17 2014, 3:40 PM.

Details

Reviewers
emaste
Summary

Implement x86_64 debug register read/write in support of hardware watchpoints. Hoist LinuxThread::TraceNotify code back into POSIXThread::TraceNotify()

llvm.org/pr16706

Diff Detail

Event Timeline

emaste added inline comments.Jan 23 2014, 7:15 AM
source/Plugins/Process/FreeBSD/ProcessMonitor.h
123

I don't follow the FIXME comment - it is using the tid, no?

source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_x86.cpp
113

Instead of the FreeBSD-specific case here, can we just put the logic into ::ReadRegisterValue, giving it visibility into our register numbering?

Responded inline to Ed Maste's comments.

source/Plugins/Process/FreeBSD/ProcessMonitor.h
123

I patterned the methods after other FreeBSD register reads/writes which still have this FIXME comments in place. Those other routines have been updated to use tid.

I agree the FIXME is not relative here, but was a reminder to clean up the comments in the other register routines. The FIXME should be removed from these 2 routines and I would be happy to remove the FIXMEs from the other routines that have been converted to use tid at the same time.

source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_x86.cpp
113

Not easily. At present, all FreeBSD access to registers - GPR or FPR - are relative to their offset in that OS specific struct. For the present, I have implemented the debug register reads and writes relative to the register offset in the "struct dbreg". The debug registers dr0 - dr7 are at the end of the register enumeration. By the time of the ReadDebugRegisterValue call is made, the register number is expressed as an offset (in the set) and the offset for r15 matches the offset of dr0 (as currently implemented).

FreeBSD probably needs something akin to Linux's "struct UserArea", but maybe not quite as detailed. But that may impact other register operations for FreeBSD.

If the current patch is acceptable as a temporary FreeBSD solution, we can discuss the changes needed to differentiate GPR registers from DB registers in FreeBSD's Read/WriteRegisterValue()

jwolfe updated this revision to Unknown Object (????).Feb 2 2014, 4:05 PM

Updated the patch to properly report watchpoints hit in threads created after the watchpoint was initially set.

POSIXBreakpointProtocol::ForceWatchpointsInitialized() member function added.

emaste added inline comments.Feb 4 2014, 12:07 PM
source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
117

FYI git apply complained about whitespace on these lines:

/home/emaste/D2572.vs6513.id6823.whitespaceignore-all.diff:52: trailing whitespace.

char setget = (req == PT_GETDBREGS) ? 'G' : 'S';

/home/emaste/D2572.vs6513.id6823.whitespaceignore-all.diff:54: trailing whitespace.

for (int i = 0; i <= 7; i++)

/home/emaste/D2572.vs6513.id6823.whitespaceignore-all.diff:97: trailing whitespace.

else

/home/emaste/D2572.vs6513.id6823.whitespaceignore-all.diff:272: trailing whitespace.

source/Plugins/Process/POSIX/POSIXThread.cpp
80

or perhaps just
(void)status;
to avoid the #ifdefs

jwolfe updated this revision to Unknown Object (????).Feb 5 2014, 5:53 AM

Updated the patch to resolve trailing whitespace warnings and eliminate the "#ifndef NDEBUG" in the POSIXThread constructor.

emaste added inline comments.Feb 5 2014, 2:21 PM
source/Plugins/Process/POSIX/POSIXThread.cpp
80

In testing now I encounter this assertion in my testrun, in TestConcurrentEvents.py and TestWatchpointMultipleThreads.py.

feynman% python dotest.py --executable $lldb -C /usr/bin/clang -v -t -f ConcurrentEventsTestCase.test_watch_break_dwarf
['dotest.py', '--executable', '/tank/emaste/src/llvm/build-nodebug/bin/lldb', '-C', '/usr/bin/clang', '-v', '-t', '-f', 'ConcurrentEventsTestCase.test_watch_break_dwarf']
LLDB library dir: /tank/emaste/src/llvm/build-nodebug/bin
lldb version 3.5 ( revision )
lldb.pre_flight: None
lldb.post_flight: None
...
runCmd: expr num_delay_signal_threads=0
output: (unsigned int) $6 = 0

runCmd: expr num_delay_watchpoint_threads=0
output: (unsigned int) $7 = 0

Assertion failed: (status && "Failed to enable existing watchpoint for new thread"), function POSIXThread, file ../tools/lldb/source/Plugins/Process/POSIX/POSIXThread.cpp, line 83.
Abort trap (core dumped)

jwolfe updated this revision to Unknown Object (????).Feb 6 2014, 6:57 PM

The EnableHardwareWatchpoint() call in POSIXThread() is not needed. In existing code, the function call is within an assert(); ergo not in a production lldb.

Revised the comment before the ForceWatchpointsInitialized() call. Corrected a typo in RegisterContextPOSIX.h

emaste added inline comments.Feb 11 2014, 12:03 PM
source/Plugins/Process/POSIX/RegisterContextFreeBSD_x86_64.cpp
14

I believe this file will also be built on Linux, so we'll need to avoid including this header there.

jwolfe updated this revision to Unknown Object (????).Feb 11 2014, 9:47 PM

Good catch Ed.

I have remove the <machine/reg.h> include and added a suitable copy of the "struct dbreg" from http://svnweb.freebsd.org/base/head/sys/x86/include/reg.h in source/Plugins/Process/POSIX/RegisterContextFreeBSD_x86_64.cpp

Hey guys,

I gave this a run on Ubuntu 12.04 x86_64 using gcc 4.8.2 and a recent libedit (much newer than libedit-dev on 12.04, it's the July 2013 3.x version of libedit).

The test runs came back with no additional test failures. It looks fine on the Linux side.

Thanks for pinging me on it, Ed.

Sincerely,
Todd Fiala

Is this patch to enable watchpoints in FreeBSD ready to be checked-in?

Todd Fiala has responded that the changes do not present a problem for Ubuntu Linux.

emaste accepted this revision.Feb 19 2014, 10:26 AM