Change -stack-list-arguments call to be more performant in some cases#673
Merged
pieandcakes merged 1 commit intomasterfrom Dec 5, 2017
Merged
Change -stack-list-arguments call to be more performant in some cases#673pieandcakes merged 1 commit intomasterfrom
pieandcakes merged 1 commit intomasterfrom
Conversation
Made change to call -stack-list-arguments with --no-values when the value flag is not specified. This improves perf for large objects in stack walks but it does add an extra call per variable to -var-create and -var-delete to get the type. The calls to -var-create/-var-delete are faster overall than the -stack-list-arguments with --simple-values. If both value and types are requested, we fall back to requesting -stack-list-arguments with simple values which will return types and values.
chuckries
approved these changes
Dec 4, 2017
| { | ||
| args.Add(new SimpleVariableInformation(n, /*isParam*/ true, null, null)); | ||
| // If the types of the arguments are requested, get that from a call to -var-create | ||
| if (types) |
Member
There was a problem hiding this comment.
Doesn't this do both costly things now if I specify values && types? If we asked for values, don't we already have the type data here?
Collaborator
Author
There was a problem hiding this comment.
@chuckries @gregg-miskelly No, because you would not be in this block if you had both. Line 1762 would be true as if you went through the "simple values" your argList would be a ValueTypeValue and it would run through that if block and not this else block.
Collaborator
Author
There was a problem hiding this comment.
If it helps, without the value, its a NamedResultValue so it falls into the else block.
gregg-miskelly
approved these changes
Dec 5, 2017
Member
gregg-miskelly
left a comment
gregg-miskelly
left a comment
There was a problem hiding this comment.
Aside from Chuck's comment, LGTM.
saagarjha
pushed a commit
to ahjragaas/binutils-gdb
that referenced
this pull request
May 4, 2023
SUMMARY
The '--simple-values' argument to '-stack-list-arguments' and similar
GDB/MI commands does not take reference types into account, so that
references to arbitrarily large structures are considered "simple" and
printed. This means that the '--simple-values' argument cannot be used
by IDEs when tracing the stack due to the time taken to print large
structures passed by reference.
DETAILS
Various GDB/MI commands ('-stack-list-arguments', '-stack-list-locals',
'-stack-list-variables' and so on) take a PRINT-VALUES argument which
may be '--no-values' (0), '--all-values' (1) or '--simple-values' (2).
In the '--simple-values' case, the command is supposed to print the
name, type, and value of variables with simple types, and print only the
name and type of variables with compound types.
The '--simple-values' argument ought to be suitable for IDEs that need
to update their user interface with the program's call stack every time
the program stops. However, it does not take C++ reference types into
account, and this makes the argument unsuitable for this purpose.
For example, consider the following C++ program:
struct s {
int v[10];
};
int
sum(const struct s &s)
{
int total = 0;
for (int i = 0; i < 10; ++i) total += s.v[i];
return total;
}
int
main(void)
{
struct s s = { { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 } };
return sum(s);
}
If we start GDB in MI mode and continue to 'sum', the behaviour of
'-stack-list-arguments' is as follows:
(gdb)
-stack-list-arguments --simple-values
^done,stack-args=[frame={level="0",args=[{name="s",type="const s &",value="@0x7fffffffe310: {v = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}}"}]},frame={level="1",args=[]}]
Note that the value of the argument 's' was printed, even though 's' is
a reference to a structure, which is not a simple value.
See microsoft/MIEngine#673 for a case where this
behaviour caused Microsoft to avoid the use of '--simple-values' in
their MIEngine debug adapter, because it caused Visual Studio Code to
take too long to refresh the call stack in the user interface.
SOLUTIONS
There are two ways we could fix this problem, depending on whether we
consider the current behaviour to be a bug.
1. If the current behaviour is a bug, then we can update the behaviour
of '--simple-values' so that it takes reference types into account:
that is, a value is simple if it is neither an array, struct, or
union, nor a reference to an array, struct or union.
In this case we must add a feature to the '-list-features' command so
that IDEs can detect that it is safe to use the '--simple-values'
argument when refreshing the call stack.
2. If the current behaviour is not a bug, then we can add a new option
for the PRINT-VALUES argument, for example, '--scalar-values' (3),
that would be suitable for use by IDEs.
In this case we must add a feature to the '-list-features' command
so that IDEs can detect that the '--scalar-values' argument is
available for use when refreshing the call stack.
PATCH
This patch implements solution (1) as I think the current behaviour of
not printing structures, but printing references to structures, is
contrary to reasonable expectation.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29554
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Made change to call -stack-list-arguments with --no-values when the
value flag is not specified. This improves perf for large objects in stack walks but it does add an
extra call per variable to -var-create and -var-delete to get the type.
The calls to -var-create/-var-delete are faster overall than the
-stack-list-arguments with --simple-values.
If both value and types are requested, we fall back to requesting
-stack-list-arguments with simple values which will return types and
values.
Testing: Tested against 200 deep stack passing either a list of 6 ints per stack, or 3 vectors of size 5000 + 3 ints. The new solution looks to be more performant in both cases.