[lldb] Part 2 of 2 - Refactor `CommandObject::DoExecute(...)` to return
`void` instead of ~~`bool`~~
Justifications:
- The code doesn't ultimately apply the `true`/`false` return values.
- The methods already pass around a `CommandReturnObject`, typically
with a `result` parameter.
- Each command return object already contains:
- A more precise status
- The error code(s) that apply to that status
Part 1 refactors the `CommandObject::Execute(...)` method.
- See
[https://github.com/llvm/llvm-project/pull/69989](https://github.com/llvm/llvm-project/pull/69989)
rdar://117378957
The `po` alias now matches the behavior of the `expression` command when
the it can apply a Fix-It to an expression.
Modifications
- Add has `m_fixed_expression` to the `CommandObjectDWIMPrint` class a
`protected` member that stores the post Fix-It expression, just like the
`CommandObjectExpression` class.
- Converted messages to present tense.
- Add test cases that confirms a Fix-It for a C++ expression for both
`po` and `expressions`
rdar://115317419
The `po` alias now matches the behavior of the `expression` command when
the it can apply a Fix-It to an expression.
Modifications
- Add has `m_fixed_expression` to the `CommandObjectDWIMPrint` class a
`protected` member that stores the post Fix-It expression, just like the
`CommandObjectExpression` class.
- Converted messages to present tense.
- Add test cases that confirms a Fix-It for a C++ expression for both
`po` and `expressions`
rdar://115317419
Co-authored-by: Pete Lawrence <plawrence@apple.com>
Follow up to "Suppress persistent result when running po" (D144044).
This change delays removal of the persistent result until after `Dump` has been called.
In doing so, the persistent result is available for the purpose of getting its object
description.
In the original change, the persistent result removal happens indirectly, by setting
`EvaluateExpressionOptions::SetSuppressPersistentResult`. In practice this has worked,
however this exposed a latent bug in swift-lldb. The subtlety, and the bug, depend on
when the persisteted result variable is removed.
When the result is removed via `SetSuppressPersistentResult`, it happens within the call
to `Target::EvaluateExpression`. That is, by the time the call returns, the persistent
result is already removed.
The issue occurs shortly thereafter, when `ValueObject::Dump` is called, it cannot make
use of the persistent result variable (instead it uses the `ValueObjectConstResult`). In
swift-lldb, this causes an additional expression evaluation to happen. It first tries an
expression that reference `$R0` etc, but that always fails because `$R0` is removed. The
fallback to this failure does work most of the time, but there's at least one bug
involving imported Clang types.
Differential Revision: https://reviews.llvm.org/D150619
When printing a value, allow the root value's name to be elided, without omiting the
names of child values.
At the API level, this adds `SetHideRootName()`, which joins the existing
`SetHideName()` function.
This functionality is used by `dwim-print` and `expression`.
Fixes an issue identified by @jgorbe in https://reviews.llvm.org/D145609.
Differential Revision: https://reviews.llvm.org/D146783
Change `dwim-print` to now disable persistent results by default, unless requested by
the user with the `--persistent-result` flag.
Ex:
```
(lldb) dwim-print 1 + 1
(int) 2
(lldb) dwim-print --persistent-result on -- 1 + 1
(int) $0 = 2
```
Users who wish to enable persistent results can make and use an alias that includes
`--persistent-result on`.
Updates: To recommit this, both TestPersistentResult.py and TestPAlias.py needed to be
updated, as well as the changes in D146230.
Differential Revision: https://reviews.llvm.org/D145609
Change `dwim-print` to now disable persistent results by default, unless requested by
the user with the `--persistent-result` flag.
Ex:
```
(lldb) dwim-print 1 + 1
(int) 2
(lldb) dwim-print --persistent-result on -- 1 + 1
(int) $0 = 2
```
Users who wish to enable persistent results can make and use an alias that includes
`--persistent-result on`.
Differential Revision: https://reviews.llvm.org/D145609
Context: The `expression` command uses artificial variables to store the expression
result. This result variable is unconditionally kept around after the expression command
has completed. These variables are known as persistent results. These are the variables
`$0`, `$1`, etc, that are displayed when running `p` or `expression`.
This change allows users to control whether result variables are persisted, by
introducing a `--persistent-result` flag.
This change keeps the current default behavior, persistent results are created by
default. This change gives users the ability to opt-out by re-aliasing `p`. For example:
```
command unalias p
command alias p expression --persistent-result false --
```
For consistency, this flag is also adopted by `dwim-print`. Of note, if asked,
`dwim-print` will create a persistent result even for frame variables.
Differential Revision: https://reviews.llvm.org/D144230
Adopt `expression`'s options in `dwim-print`.
This is primarily added to support the `--language`/`-l` flag.
Differential Revision: https://reviews.llvm.org/D144114
Remove the persistent result variable after executing `po`.
Without this change, the following behavior happens:
```
(lldb) p thing
(NSObject *) $0 = 0x600000008000
(lldb) po thing
<NSObject: 0x600000008000>
(lldb) p thing
(NSObject *) $2 = 0x600000008000
(lldb) p $1
(NSObject *) $1 = 0x600000008000
```
Even though `po` hides the persistent result variable, it's still created - as $1 in
this example. It can be accessed even though its existence is not evident.
With this change, the persistent result is removed after the object description has
printed. Instead, this is the behavior:
```
(lldb) p thing
(NSObject *) $0 = 0x600000008000
(lldb) po thing
<NSObject: 0x600000008000>
(lldb) p thing
(NSObject *) $1 = 0x600000008000
```
The difference here is that the `po` doens't silently create a persistent result.
Differential Revision: https://reviews.llvm.org/D144044
Before:
```
(lldb) expr --language abc -- 1 + 1
error: unknown language type: 'abc' for expression
```
After:
```
(lldb) expr --language abc -- 1 + 1
error: unknown language type: 'abc' for expression. List of supported languages:
c++
objective-c++
c++03
c++11
c++14
objc++
```
We choose to only list the languages which `expr` will actually
accept instead of all the language constants defined in `Language.cpp`
since that's what the user will most likely need.
Differential Revision: https://reviews.llvm.org/D142034
This patch mechanically replaces None with std::nullopt where the
compiler would warn if None were deprecated. The intent is to reduce
the amount of manual work required in migrating from Optional to
std::optional.
This is part of an effort to migrate from llvm::Optional to
std::optional:
https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716
When UserExpression::Evaluate() fails and doesn't return a ValueObject there is no vehicle for returning the error in the return value.
This behavior can be observed by applying the following patch:
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index f1a311b7252c..58c03ccdb068 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2370,6 +2370,7 @@ UserExpression *Target::GetUserExpressionForLanguage(
Expression::ResultType desired_type,
const EvaluateExpressionOptions &options, ValueObject *ctx_obj,
Status &error) {
+ error.SetErrorStringWithFormat("Ha ha!"); return nullptr;
auto type_system_or_err = GetScratchTypeSystemForLanguage(language);
if (auto err = type_system_or_err.takeError()) {
error.SetErrorStringWithFormat(
and then running
$ lldb -o "p 1"
(lldb) p 1
(lldb)
This patch fixes this by creating an empty result ValueObject that wraps the error.
Differential Revision: https://reviews.llvm.org/D135998
When UserExpression::Evaluate() fails and doesn't return a ValueObject there is no vehicle for returning the error in the return value.
This behavior can be observed by applying the following patch:
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index f1a311b7252c..58c03ccdb068 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2370,6 +2370,7 @@ UserExpression *Target::GetUserExpressionForLanguage(
Expression::ResultType desired_type,
const EvaluateExpressionOptions &options, ValueObject *ctx_obj,
Status &error) {
+ error.SetErrorStringWithFormat("Ha ha!"); return nullptr;
auto type_system_or_err = GetScratchTypeSystemForLanguage(language);
if (auto err = type_system_or_err.takeError()) {
error.SetErrorStringWithFormat(
and then running
$ lldb -o "p 1"
(lldb) p 1
(lldb)
This patch fixes this by creating an empty result ValueObject that wraps the error.
Differential Revision: https://reviews.llvm.org/D135998
Refactor the command option enum values and the command argument table
to connect the two. This has two benefits:
- We guarantee that two options that use the same argument type have
the same accepted values.
- We can print the enum values and their description in the help
output. (D129707)
Differential revision: https://reviews.llvm.org/D129703
Applied modernize-use-equals-default clang-tidy check over LLDB.
This check is already present in the lldb/.clang-tidy config.
Differential Revision: https://reviews.llvm.org/D121844
This patch is a smaller version of a previous patch https://reviews.llvm.org/D110804.
This patch modifies the output of "statistics dump" to be able to get stats from the current target. It adds 3 new stats as well. The output of "statistics dump" is now emitted as JSON so that it can be used to track performance and statistics and the output could be used to populate a database that tracks performance. Sample output looks like:
(lldb) statistics dump
{
"expressionEvaluation": {
"failures": 0,
"successes": 0
},
"firstStopTime": 0.34164492800000001,
"frameVariable": {
"failures": 0,
"successes": 0
},
"launchOrAttachTime": 0.31969605400000001,
"targetCreateTime": 0.0040863039999999998
}
The top level keys are:
"expressionEvaluation" which replaces the previous stats that were emitted as plain text. This dictionary contains the success and fail counts.
"frameVariable" which replaces the previous stats for "frame variable" that were emitted as plain text. This dictionary contains the success and fail counts.
"targetCreateTime" contains the number of seconds it took to create the target and load dependent libraries (if they were enabled) and also will contain symbol preloading times if that setting is enabled.
"launchOrAttachTime" is the time it takes from when the launch/attach is initiated to when the first private stop occurs.
"firstStopTime" is the time in seconds that it takes to stop at the first stop that is presented to the user via the LLDB interface. This value will only have meaning if you set a known breakpoint or stop location in your code that you want to measure as a performance test.
This diff is also meant as a place to discuess what we want out of the "statistics dump" command before adding more funcionality. It is also meant to clean up the previous code that was storting statistics in a vector of numbers within the lldb_private::Target class.
Differential Revision: https://reviews.llvm.org/D111686
If we applied a fix-it before evaluating an expression and that
expression didn't evaluate correctly, we should still tell users about
the fix-it we applied since that may be the reason why it didn't work
correctly.
Differential Revision: https://reviews.llvm.org/D109908
This is part 2, covering the commands source.
Some uses remain where it's tricky to see what the
logic is or they are not used with AppendError.
Reviewed By: teemperor
Differential Revision: https://reviews.llvm.org/D104448
The `--allow-jit` flag allows the user to force the IR interpreter to run the
provided expression.
The `--top-level` flag parses and injects the code as if its in the top level
scope of a source file.
Both flags just change the ExecutionPolicy of the expression:
* `--allow-jit true` -> doesn't change anything (its the default)
* `--allow-jit false` -> ExecutionPolicyNever
* `--top-level` -> ExecutionPolicyTopLevel
Passing `--allow-jit false` and `--top-level` currently causes the `--top-level`
to silently overwrite the ExecutionPolicy value that was set by `--allow-jit
false`. There isn't any ExecutionPolicy value that says "top-level but only
interpret", so I would say we reject this combination of flags until someone
finds time to refactor top-level feature out of the ExecutionPolicy enum.
The SBExpressionOptions suffer from a similar symptom as `SetTopLevel` and
`SetAllowJIT` just silently disable each other. But those functions don't have
any error handling, so not a lot we can do about this in the meantime.
Reviewed By: labath, kastiglione
Differential Revision: https://reviews.llvm.org/D91780
This reverts commit a01b26fb51c710a3a8ef88cc83b0701461f5b9ab, because it
breaks the "finish" command in some way -- the command does not
terminate after it steps out, but continues running the target. The
exact blast radius is not clear, but it at least affects the usage of
the "finish" command in TestGuiBasicDebug.py. The error is *not*
gui-related, as the same issue can be reproduced by running the same
steps outside of the gui.
There is some kind of a race going on, as the test fails only 20% of the
time on the buildbot.
Currently, the interpreter's context is not updated until a command is executed.
This has resulted in the behavior of SB-interface functions and some commands
depends on previous user actions. The interpreter's context can stay uninitialized,
point to a currently selected target, or point to one of previously selected targets.
This patch removes any usages of CommandInterpreter::UpdateExecutionContext.
CommandInterpreter::HandleCommand* functions still may override context temporarily,
but now they always restore it before exiting. CommandInterpreter saves overriden
contexts to the stack, that makes nesting commands possible.
Added test reproduces one of the issues. Without this fix, the last assertion fails
because interpreter's execution context is empty until running "target list", so,
the value of the global property was updated instead of process's local instance.
Differential Revision: https://reviews.llvm.org/D92164
Color the error: and warning: part of the CommandReturnObject output,
similar to how an error is printed from the driver when colors are
enabled.
Differential revision: https://reviews.llvm.org/D81058
Summary:
Usually when Clang emits an error Fix-It it does two things. It emits the diagnostic and then it fixes the
currently generated AST to reflect the applied Fix-It. While emitting the diagnostic is easy to implement,
fixing the currently generated AST is often tricky. That causes that some Fix-Its just keep the AST as-is or
abort the parsing process entirely. Once the parser stopped, any Fix-Its for the rest of the expression are
not detected and when the user manually applies the Fix-It, the next expression will just produce a new
Fix-It.
This is often occurring with quickly made Fix-Its that are just used to bridge temporary API changes
and that often are not worth implementing a proper API fixup in addition to the diagnostic. To still
give some kind of reasonable user-experience for users that have these Fix-Its and rely on them to
fix their expressions, this patch adds the ability to retry parsing with applied Fix-Its multiple time to
give the normal Fix-It experience where things Clang knows how to fix are not causing actual expression
error (at least when automatically applying Fix-Its is activated).
The way this is implemented is just by having another setting in the expression options that specify how
often we should try applying Fix-Its and then reparse the expression. The default setting is still 1 for everyone
so this should not affect the speed in which we fail to parse expressions.
Reviewers: jingham, JDevlieghere, friss, shafik
Reviewed By: shafik
Subscribers: shafik, abidh
Differential Revision: https://reviews.llvm.org/D77214
Summary:
LLDB keeps statistics of how many expression evaluations are 'successful' and 'failed'
which are updated after each expression evaluation (assuming statistics are enabled).
From what I understand the idea is that this could be used to define how well LLDB's
expression evaluator is working.
Currently all expressions are considered successful unless the user passes an explicit
positive element counting to the expression command (with the `-Z` flag) and then passes
an expression that successfully evaluates to a type that doesn't support element counting.
Expressions that fail to parse, execute or any other outcome are considered successful
at the moment which means we nearly always have a 100% expression evaluation
success rate.
This patch makes that expressions that fail to parse or execute to count as failed
expressions.
We can't know whether the expression failed because of an user error
of because LLDB couldn't correctly parse/compile it, but I would argue that this is
still an improvement. Assuming that the percentage of valid user expressions stays
mostly constant over time (which seems like a reasonable assumption), then this
way we can still see if we are doing relatively better/worse from release to release.
Reviewers: davide, aprantl, JDevlieghere
Reviewed By: aprantl
Subscribers: abidh
Differential Revision: https://reviews.llvm.org/D76280
The error_stream and result parameter were inconsistently checked for
being null, so we might as well make them references instead of crashing
in case someone passes a nullptr and hits one of the code paths that are
currently not doing a nullptr check on those parameters. Also change
output_stream for consistency.
This is how it should've been and brings it more in line with
std::string_view. There should be no functional change here.
This is mostly mechanical from a custom clang-tidy check, with a lot of
manual fixups. It uncovers a lot of minor inefficiencies.
This doesn't actually modify StringRef yet, I'll do that in a follow-up.
The GetRawLine currently returns the full command line used
to create the CompletionRequest. So for example for "foo b[tab] --arg"
it would return the whole string instead of "foo b". Usually
completion code makes the wrong assumption that the cursor is at
the end of the line and handing out the complete line will cause
that people implement completions that also make this assumption.
This patch makes GetRawLine() return only the string until the
cursor and hides the suffix (so that the cursor is always at the
end of this string) and adds another function GetRawLineWithUnusedSuffix
that is specifically the line with the suffix that isn't used by
the CompletionRequest for argument parsing etc.
There is only one user of this new function that actually needs the
suffix and that is the expression command which needs the suffix to
detect if it is in the raw or argument part of the command (by looking
at the "--" separator).
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
Summary: This removes most of unnecessary includes in the `source/Commands` directory. This was generated by IWYU and a script that fixed all the bogus reports from IWYU. Patch is tested on Linux and macOS.
Reviewers: JDevlieghere
Reviewed By: JDevlieghere
Subscribers: krytarowski, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D71489
Summary:
This patch removes File::SetStream() and File::SetDescriptor(),
and replaces most direct uses of File with pointers to File.
Instead of calling SetStream() on a file, we make a new file and
replace it.
My ultimate goal here is to introduce a new API class SBFile, which
has full support for python io.IOStream file objects. These can
redirect read() and write() to python code, so lldb::Files will
need a way to dispatch those methods. Additionally it will need some
form of sharing and assigning files, as a SBFile will be passed in and
assigned to the main IO streams of the debugger.
In my prototype patch queue, I make File itself copyable and add a
secondary class FileOps to manage the sharing and dispatch. In that
case SBFile was a unique_ptr<File>.
(here: https://github.com/smoofra/llvm-project/tree/files)
However in review, Pavel Labath suggested that it be shared_ptr instead.
(here: https://reviews.llvm.org/D67793)
In order for SBFile to use shared_ptr<File>, everything else should
as well.
If this patch is accepted, I will make SBFile use a shared_ptr
I will remove FileOps from future patches and use subclasses of File
instead.
Reviewers: JDevlieghere, jasonmolenda, zturner, jingham, labath
Reviewed By: labath
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D67891
llvm-svn: 373090
Summary:
We always have a dummy target, so any error handling regarding a missing dummy target is dead code now.
Also makes the CommandObject methods that return Target& to express this fact in the API.
This patch just for the CommandObject part of LLDB. I'll migrate the rest of LLDB in a follow-up patch that's WIP.
Reviewers: labath
Reviewed By: labath
Subscribers: abidh, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D66737
llvm-svn: 369939
Summary:
We currently have a bunch of code that is supposed to handle invalid command options, but
all this code is unreachable because invalid options are already handled in `Options::Parse`.
The only way we can reach this code is when we declare but then not implement an option
(which will be made impossible with D65386, which is also when we can completely remove
the `default` cases).
This patch replaces all this code with `llvm_unreachable` to make clear this is dead code
that can't be reached.
Reviewers: JDevlieghere
Reviewed By: JDevlieghere
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D66522
llvm-svn: 369625
