Page MenuHomePhabricator

[Windows] Add support of watchpoints to `ProcessWindows`
ClosedPublic

Authored by aleksandr.urakov on Sep 4 2019, 6:41 AM.

Details

Summary

This patch adds support of watchpoints to the old ProcessWindows plugin.

The ProcessWindows plugin uses the RegisterContext to set and reset watchpoints. The RegisterContext has some interface to access watchpoints, but it is very limited (e.g. it is impossible to retrieve the last triggered watchpoint with it), that's why I have implemented a slightly different interface in the RegisterContextWindows. Moreover, I have made the ProcessWindows plugin responsible for search of a vacant watchpoint slot, because watchpoints exist per-process (not per-thread), then we can place the same watchpoint in the same slot in different threads. With this scheme threads don't need to have their own watchpoint lists, and it simplifies identifying of the last triggered watchpoint.

It looks like it is enough to just implement corresponding methods in the NativeRegisterContextWindows-based classes to support the feature in the new NativeProcessWindows plugin. I'll do it in the next review.

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth added inline comments.Sep 4 2019, 3:27 PM
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
397 ↗(On Diff #218673)

The names here are a bit confusing: GetTriggeredHardwareBreakpointSlot doesn't return a slot; it returns the index of a slot, so slot also isn't a slot, but the index of a slot. m_watchpoint_slots is not a collection of slots but IDs, that's indexed by an index called a slot.

It may not be possible to completely rationalize this, but doing even a little could help future readers understand. For example, slot could be slot_index, and m_watchpoint_slots could be m_watchpoint_ids (or even just m_watchpoints).

525 ↗(On Diff #218673)

This no longer sets the thread ID. Was that intentional?

743 ↗(On Diff #218673)

Can you help me why we needed to get rid of the const on the HostThread &?

If native_new_thread were also a const ref, I don't see any conflicting constraint.

832 ↗(On Diff #218673)

Should we check if the watchpoint is already enabled? I noticed that DisableWatchpoint has an analogous guard clause.

852 ↗(On Diff #218673)

Since you have to explicitly downcast, wouldn't auto *reg_ctx on the left be sufficient and just as clear?

lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
82 ↗(On Diff #218673)

Clever! It took me a minute or two to figure out what the point of that was checking. Perhaps a comment to explain?

zturner added a subscriber: zturner.Sep 4 2019, 3:59 PM
zturner added inline comments.
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
82 ↗(On Diff #218673)

Isn't this equivalent to:

switch (size)
{
    case 1:
    case 2:
    case 4:
    case 8:
        break;
    default:
        return false;
}

? That definitely seems much clearer.

I'm also pretty sure that on x86 you can't add a 64-bit watch, So you'd have to do something different depending on the target bitness if you want this to be correct for x86.

Address the requested changes.

Address the requested changes.

aleksandr.urakov marked an inline comment as done.

Check bitness of the target when checking the watchpoint size.

aleksandr.urakov marked 5 inline comments as done.Sep 4 2019, 10:59 PM
aleksandr.urakov added inline comments.
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
397 ↗(On Diff #218673)

The most confusing thing here is that watchpoints have two different IDs: ID in LLDB as it is saw by an user, and the number of corresponding debug register in CPU (from 0 to 3). That's why I have selected completely different name for the last one and have called it slot. But you are right, m_watchpoint_slots is a wrong name, changed it to m_watchpoint_ids (because we already have m_watchpoints - it is map from LLDB IDs to watchpoint information).

525 ↗(On Diff #218673)

Before this commit m_new_threads kept HostThreads, not ThreadSPs. But a HostThread have no RegisterContext, which we need to set all watchpoints when a new thread is created. That's why we create ThreadSP on thread creation now, so we set the thread ID there (in OnCreateThread function).

743 ↗(On Diff #218673)

Oh, I'm sorry, it was left unintentionally after my previous implementation. Fixed it, thanks!

852 ↗(On Diff #218673)

Actually, I'm a big fan of auto, and some time ago @zturner have told me that normally it is not used so much in LLVM code, so I have reduced its usage in my patches :) But if it is OK to use auto here, I'll fix it (and in similar places too), thanks!

lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
82 ↗(On Diff #218673)

Yes, it is equivalent, I've chosen the previous form due to its less verbosity. But you are right, clearance is better (especially after adding the architecture check). Fixed it, thanks!

labath added a subscriber: labath.Sep 4 2019, 11:21 PM
labath added inline comments.
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
852 ↗(On Diff #218673)

Yeah, llvm does not use auto too match, but in this case the type is already explicitly present, so there's no ambiguity, and this is fine. (I would still use auto * instead of plain auto though..)

lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
82 ↗(On Diff #218673)

.... or, you could just use llvm::isPowerOf2_32 from MathExtras.h.

Determine whether 8-byte watchpoints are supported or not statically.

aleksandr.urakov marked 3 inline comments as done.Sep 5 2019, 5:49 AM
aleksandr.urakov added inline comments.
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
82 ↗(On Diff #218673)

I didn't know about the such function, thanks! But I think that Zachary's approach is better in exactly this case (taking in account the bitness check).

87–89 ↗(On Diff #218897)

In the old plugin a required register context is created based on the target architecture check and the bitness of LLDB, but cross-targets (e.g. WoW64) are not supported, and even the type of m_context is determined statically. So we can safely determine the max watchpoint size statically too.

amccarth accepted this revision.Sep 5 2019, 3:30 PM

Thanks for the changes! I think this looks good now.

This revision is now accepted and ready to land.Sep 5 2019, 3:30 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2019, 10:37 PM