FreeBSD watchpoint support
ClosedPublic

Press ? to show keyboard shortcuts.
Author
jwolfe
Reviewers
emaste
Lint
Lint Skipped
Unit
Unit Tests Skipped
Apply Patch
arc patch D2572
Projects
None
Subscribers
tfiala, Restricted Mailing List
Summary

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

llvm.org/pr16706

emaste added inline comments.Via LegacyJan 23 2014, 7:15 AM
Inline Comments
source/Plugins/Process/FreeBSD/ProcessMonitor.h
127 ↗(On Diff #6513)

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

source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_x86.cpp
113 ↗(On Diff #6513)

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

jwolfe added a comment.Via LegacyJan 23 2014, 10:26 AM

Responded inline to Ed Maste's comments.

Inline Comments
source/Plugins/Process/FreeBSD/ProcessMonitor.h
127 ↗(On Diff #6513)

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 ↗(On Diff #6513)

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.Via LegacyFeb 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.Via LegacyFeb 4 2014, 12:07 PM
Inline Comments
source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
117 ↗(On Diff #6823)

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 ↗(On Diff #6823)

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

jwolfe updated this revision.Via LegacyFeb 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.Via LegacyFeb 5 2014, 2:21 PM
Inline Comments
source/Plugins/Process/POSIX/POSIXThread.cpp
80 ↗(On Diff #6884)

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.Via LegacyFeb 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.Via LegacyFeb 11 2014, 12:03 PM
Inline Comments
source/Plugins/Process/POSIX/RegisterContextFreeBSD_x86_64.cpp
14 ↗(On Diff #6932)

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

jwolfe updated this revision.Via LegacyFeb 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

tfiala added a comment.Via LegacyFeb 12 2014, 9:44 AM

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

jwolfe added a comment.Via LegacyFeb 18 2014, 10:04 AM

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.Via LegacyFeb 19 2014, 10:26 AM
emaste closed this revision.Via LegacyFeb 19 2014, 10:41 AM

Revision Update History

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 16513Jan 17 2014, 3:29 PM
Diff 26823Updated the patch to properly report watchpoints hit in threads created after the watchpoint was initially set. Feb 2 2014, 3:40 PM
Diff 36884Updated the patch to resolve trailing whitespace warnings and eliminate the "#ifndef NDEBUG" in the POSIXThread constructor.Feb 5 2014, 5:47 AM
Diff 46932The EnableHardwareWatchpoint() call in POSIXThread() is not needed. In existing code, the function call is within an assert(); ergo not in a production lldb. Feb 6 2014, 6:39 PM
Diff 57015Good catch Ed.Feb 11 2014, 9:36 PM

Diff 7015

source/Plugins/Process/FreeBSD/ProcessMonitor.h

Loading...

source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

Loading...

source/Plugins/Process/Linux/LinuxThread.h

Loading...

source/Plugins/Process/Linux/LinuxThread.cpp

Loading...

source/Plugins/Process/POSIX/POSIXThread.cpp

Loading...

source/Plugins/Process/POSIX/RegisterContextFreeBSD_i386.cpp

Loading...

source/Plugins/Process/POSIX/RegisterContextFreeBSD_x86_64.cpp

Loading...

source/Plugins/Process/POSIX/RegisterContextPOSIX.h

Loading...

source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_x86.cpp

Loading...

Add Comment