This patch is rearranging code a bit to add WatchpointResources to
Process. A WatchpointResource is meant to represent a hardware
watchpoint register in the inferior process. It has an address, a size,
a type, and a list of Watchpoints that are using this
WatchpointResource.
This current patch doesn't add any of the features of
WatchpointResources that make them interesting -- a user asking to watch
a 24 byte object could watch this with three 8 byte WatchpointResources.
Or a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte at
0x1003, these must both be served by a single WatchpointResource on that
doubleword at 0x1000 on a 64-bit target, if two hardware watchpoint
registers were used to track these separately, one of them may not be
hit. Or if you have one Watchpoint on a variable with a condition set,
and another Watchpoint on that same variable with a command defined or
different condition, or ignorecount, both of those Watchpoints need to
evaluate their criteria/commands when their WatchpointResource has been
hit.
There's a bit of code movement to rearrange things in the direction I'll
need for implementing this feature, so I want to start with reviewing &
landing this mostly NFC patch and we can focus on the algorithmic
choices about how WatchpointResources are shared and handled as they're
triggeed, separately.
This patch also stops printing "Watchpoint <n> hit: old value: <x>, new
vlaue: <y>" for Read watchpoints. I could make an argument for print
"Watchpoint <n> hit: current value <x>" but the current output doesn't
make any sense, and the user can print the value if they are
particularly interested. Read watchpoints are used primarily to
understand what code is reading a variable.
This patch adds more fallbacks for how to print the objects being
watched if we have types, instead of assuming they are all integral
values, so a struct will print its elements. As large watchpoints are
added, we'll be doing a lot more of those.
To track the WatchpointSP in the WatchpointResources, I changed the
internal API which took a WatchpointSP and devolved it to a Watchpoint*,
which meant touching several different Process files. I removed the
watchpoint code in ProcessKDP which only reported that watchpoints
aren't supported, the base class does that already.
I haven't yet changed how we receive a watchpoint to identify the
WatchpointResource responsible for the trigger, and identify all
Watchpoints that are using this Resource to evaluate their conditions
etc. This is the same work that a BreakpointSite needs to do when it has
been tiggered, where multiple Breakpoints may be at the same address.
There is not yet any printing of the Resources that a Watchpoint is
implemented in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).
"watchpoint set var" and "watchpoint set expression" take a size
argument which was previously 1, 2, 4, or 8 (an enum). I've changed this
to an unsigned int. Most hardware implementations can only watch 1, 2,
4, 8 byte ranges, but with Resources we'll allow a user to ask for
different sized watchpoints and set them in hardware-expressble terms
soon.
I've annotated areas where I know there is work still needed with
LWP_TODO that I'll be working on once this is landed.
I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116
(cherry picked from commit fc6b72523f3d73b921690a713e97a433c96066c6)
...and follow ups. As it has caused test failures on Linux Arm and AArch64: https://lab.llvm.org/buildbot/#/builders/96/builds/49126 https://lab.llvm.org/buildbot/#/builders/17/builds/45824 ``` lldb-shell :: Subprocess/clone-follow-child-wp.test lldb-shell :: Subprocess/fork-follow-child-wp.test lldb-shell :: Subprocess/vfork-follow-child-wp.test ``` This reverts commit a6c62bf1a4717accc852463b664cd1012237d334, commit a0a1ff3ab40e347589b4e27d8fd350c600526735 and commit fc6b72523f3d73b921690a713e97a433c96066c6.
This patch is rearranging code a bit to add WatchpointResources to
Process. A WatchpointResource is meant to represent a hardware
watchpoint register in the inferior process. It has an address, a size,
a type, and a list of Watchpoints that are using this
WatchpointResource.
This current patch doesn't add any of the features of
WatchpointResources that make them interesting -- a user asking to watch
a 24 byte object could watch this with three 8 byte WatchpointResources.
Or a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte at
0x1003, these must both be served by a single WatchpointResource on that
doubleword at 0x1000 on a 64-bit target, if two hardware watchpoint
registers were used to track these separately, one of them may not be
hit. Or if you have one Watchpoint on a variable with a condition set,
and another Watchpoint on that same variable with a command defined or
different condition, or ignorecount, both of those Watchpoints need to
evaluate their criteria/commands when their WatchpointResource has been
hit.
There's a bit of code movement to rearrange things in the direction I'll
need for implementing this feature, so I want to start with reviewing &
landing this mostly NFC patch and we can focus on the algorithmic
choices about how WatchpointResources are shared and handled as they're
triggeed, separately.
This patch also stops printing "Watchpoint <n> hit: old value: <x>, new
vlaue: <y>" for Read watchpoints. I could make an argument for print
"Watchpoint <n> hit: current value <x>" but the current output doesn't
make any sense, and the user can print the value if they are
particularly interested. Read watchpoints are used primarily to
understand what code is reading a variable.
This patch adds more fallbacks for how to print the objects being
watched if we have types, instead of assuming they are all integral
values, so a struct will print its elements. As large watchpoints are
added, we'll be doing a lot more of those.
To track the WatchpointSP in the WatchpointResources, I changed the
internal API which took a WatchpointSP and devolved it to a Watchpoint*,
which meant touching several different Process files. I removed the
watchpoint code in ProcessKDP which only reported that watchpoints
aren't supported, the base class does that already.
I haven't yet changed how we receive a watchpoint to identify the
WatchpointResource responsible for the trigger, and identify all
Watchpoints that are using this Resource to evaluate their conditions
etc. This is the same work that a BreakpointSite needs to do when it has
been tiggered, where multiple Breakpoints may be at the same address.
There is not yet any printing of the Resources that a Watchpoint is
implemented in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).
"watchpoint set var" and "watchpoint set expression" take a size
argument which was previously 1, 2, 4, or 8 (an enum). I've changed this
to an unsigned int. Most hardware implementations can only watch 1, 2,
4, 8 byte ranges, but with Resources we'll allow a user to ask for
different sized watchpoints and set them in hardware-expressble terms
soon.
I've annotated areas where I know there is work still needed with
LWP_TODO that I'll be working on once this is landed.
I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116
Most of our code was including Log.h even though that is not where the "lldb" log channel is defined (Log.h defines the generic logging infrastructure). This worked because Log.h included Logging.h, even though it should. After the recent refactor, it became impossible the two files include each other in this direction (the opposite inclusion is needed), so this patch removes the workaround that was put in place and cleans up all files to include the right thing. It also renames the file to LLDBLog to better reflect its purpose.
Differential Revision: https://reviews.llvm.org/D75711
Summary: A *.cpp file header in LLDB (and in LLDB) should like this: ``` //===-- TestUtilities.cpp -------------------------------------------------===// ``` However in LLDB most of our source files have arbitrary changes to this format and these changes are spreading through LLDB as folks usually just use the existing source files as templates for their new files (most notably the unnecessary editor language indicator `-*- C++ -*-` is spreading and in every review someone is pointing out that this is wrong, resulting in people pointing out that this is done in the same way in other files). This patch removes most of these inconsistencies including the editor language indicators, all the different missing/additional '-' characters, files that center the file name, missing trailing `===//` (mostly caused by clang-format breaking the line). Reviewers: aprantl, espindola, jfb, shafik, JDevlieghere Reviewed By: JDevlieghere Subscribers: dexonsmith, wuzish, emaste, sdardis, nemanjai, kbarton, MaskRay, atanasyan, arphaman, jfb, abidh, jsji, JDevlieghere, usaxena95, lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D73258
This patch replaces explicit calls to log::Printf with the new LLDB_LOGF
macro. The macro is similar to LLDB_LOG but supports printf-style format
strings, instead of formatv-style format strings.
So instead of writing:
if (log)
log->Printf("%s\n", str);
You'd write:
LLDB_LOG(log, "%s\n", str);
This change was done mechanically with the command below. I replaced the
spurious if-checks with vim, since I know how to do multi-line
replacements with it.
find . -type f -name '*.cpp' -exec \
sed -i '' -E 's/log->Printf\(/LLDB_LOGF\(log, /g' "{}" +
Differential revision: https://reviews.llvm.org/D65128
llvm-svn: 366936
A lot of comments in LLDB are surrounded by an ASCII line to delimit the begging and end of the comment. Its use is not really consistent across the code base, sometimes the lines are longer, sometimes they are shorter and sometimes they are omitted. Furthermore, it looks kind of weird with the 80 column limit, where the comment actually extends past the line, but not by much. Furthermore, when /// is used for Doxygen comments, it looks particularly odd. And when // is used, it incorrectly gives the impression that it's actually a Doxygen comment. I assume these lines were added to improve distinguishing between comments and code. However, given that todays editors and IDEs do a great job at highlighting comments, I think it's worth to drop this for the sake of consistency. The alternative is fixing all the inconsistencies, which would create a lot more churn. Differential revision: https://reviews.llvm.org/D60508 llvm-svn: 358135
to reflect the new license. We understand that people may be surprised that we're moving the header entirely to discuss the new license. We checked this carefully with the Foundation's lawyer and we believe this is the correct approach. Essentially, all code in the project is now made available by the LLVM project under our new license, so you will see that the license headers include that license only. Some of our contributors have contributed code under our old license, and accordingly, we have retained a copy of our old license notice in the top-level files in each project and repository. llvm-svn: 351636