2:45 p.m.
Hi Dave,
I'm sorry for the last submission. It seems I forgot to refresh the
patches, so it was completely bogus. Should be fixed now. I'm also
attaching my changes as one big patch to this message.
Petr Tesarik
2:46 p.m.
New subject: [PATCHv2 01/11] Move cmd_p() main processing into its own function
Move gdb output filtering to its own function so that it can be reused
from other places.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
symbols.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
2:46 p.m.
New subject: [PATCHv2 02/11] New convenience function: get_cpumask_buf
Make a convenience function for allocating a cpumask buffer and
put it into tools.c. Since any caller of make_cpumask() needs
such a buffer, it is better to isolate the code in a function
rather than copy it all over the place.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
defs.h | 1 +
kernel.c | 5 +----
tools.c | 13 +++++++++++++
3 files changed, 15 insertions(+), 4 deletions(-)
2:47 p.m.
New subject: [PATCHv2 03/11] Implement percpu variables for "p"
Add per-cpu specifications to the "p" command. This allows to specify
CPUs for a per-cpu symbol after a colon.
Keep in mind that a colon can be part of a valid C expression, e.g. ':'
or the ternary operator, so if the part before the colon is not a symbol,
the colon must be restored. Fortunately, a symbol name followed by a colon
cannot start a valid C expression, so there is never any ambiguity.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
help.c | 13 ++++++-
symbols.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 113 insertions(+), 13 deletions(-)
2:47 p.m.
New subject: [PATCHv2 04/11] Fix percpu handling for symbols without debuginfo
When trying to show a percpu symbol without debuginfo, crash fails like
this:
crash> p fcoe_percpu:0
p: gdb request failed: p *(<data variable, no debug info>*) 0xffff88011e2d4900
This is because gdb does not really fail if the type is unknown:
crash> gdb whatis fcoe_percpu
type = <data variable, no debug info>
crash> gdb info variables fcoe_percpu
All variables matching regular expression "fcoe_percpu":
Non-debugging symbols:
0x0000000000014900 fcoe_percpu
Of course, the actual contents cannot be shown in this case, but crash should
at least display the address:
crash> p fcoe_percpu:0
&per_cpu(fcoe_percpu, 0) = $1 = (void *) 0xffff88011e214900
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
symbols.c | 3 +++
1 file changed, 3 insertions(+)
2:48 p.m.
New subject: [PATCHv2 05/11] Recognize "a" and "all" as CPU specifiers
Extend the syntax parsed by make_cpuset() so that "a" or "all" is
an alias for all CPUs in the system.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
help.c | 3 ++-
tools.c | 20 +++++++++++++-------
2 files changed, 15 insertions(+), 8 deletions(-)
2:49 p.m.
New subject: [PATCHv2 06/11] Convert cmd_datatype_common variables to flags
Convert flag variables in cmd_datatype_common() to flag values, so that
fewer values have to be passed when the main printing loop is converted
to a standalone function.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
symbols.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
2:49 p.m.
New subject: [PATCHv2 07/11] Cleanup handling of flags in cmd_datatype_common()
This patch improves handling of struct/union flags:
1. You can now specify a typedef with "*". Previously it failed like
this:
crash> *acct_t ffff8800982ad520
*: invalid argument
2. Report an error if a non-structured type is used, instead of
giving no output, e.g.:
crash> *pid_t
*: invalid argument
3. Produce an error message if the type does not match the command:
crash> union page
union: data type mismatch: page is not a union
crash> struct thread_union
struct: data type mismatch: thread_union is not a struct
Additionaly, these checks are now performed early, that is before
entering the main printing loop.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
symbols.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
2:50 p.m.
New subject: [PATCHv2 08/11] Helper function to add member info to struct datatype_member
Instead of parsing the complete type on each iteration, move the code
to a helper function. This might be slightly faster, but most importantly
the main loop doesn't need the "structname" and "separator"
variables
after this change.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
symbols.c | 44 +++++++++++++++++++++++---------------------
1 file changed, 23 insertions(+), 21 deletions(-)
2:50 p.m.
New subject: [PATCHv2 09/11] Fix struct -o with an address and array count
Previously, the array count was ignored with the "-o" option.
While I admit that repeating code is not "good style", the goto from
the middle of a loop was not nice either, and this variant removes
a dependency of the main loop on the "aflag" variable.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
symbols.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
2:51 p.m.
New subject: [PATCHv2 10/11] Move the main loop of cmd_datatype_common() to its own function
This change allows to reuse the main loop in a different context.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
symbols.c | 41 ++++++++++++++++++++++-------------------
1 file changed, 22 insertions(+), 19 deletions(-)
2:54 p.m.
New subject: [PATCHv2 11/11] Add percpu handling to struct/union/* commands
Extend the argument syntax of struct/union/* commands to recognize
percpu specifiers. Note that percpu addresses are not valid kernel
virtual addresses, so they must be parsed separately.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
help.c | 18 ++++++++++++++----
symbols.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 5 deletions(-)
2:37 p.m.
----- Original Message -----
Hi Dave,
I'm sorry for the last submission. It seems I forgot to refresh the
patches, so it was completely bogus. Should be fixed now. I'm also
attaching my changes as one big patch to this message.
Petr Tesarik
Hi Petr,
I've reviewed the code changes, and have been beating on the
patch, and I can't get it break. Really nice work...
The only things I'd like to change are trivial/cosmetic.
For clarity's sake I'd like to change the "p" help page
to separate expressions from symbols as arguments, and
then enforce the fact that the :cpuspec only applies to
symbol arguments. So instead of this:
crash> help p
NAME
p - print the value of an expression
SYNOPSIS
p [-x|-d][-u] expression[:cpuspec]
DESCRIPTION
This command passes its arguments on to gdb "print" command for evaluation.
expression The expression to be evaluated.
cpuspec CPU specification for per-cpu variables:
: CPU of the currently selected task.
:a[ll] all CPUs.
:#[-#][,...] CPU list(s), e.g. "1,3,5", "1-3",
or "1,3,5-7,10".
-x override default output format with hexadecimal format.
-d override default output format with decimal format.
-u the expression evaluates to a user address reference.
...
I'd change it to this:
crash> help p
NAME
p - print the value of an expression
SYNOPSIS
p [-x|-d][-u] [expression | symbol[:cpuspec]]
DESCRIPTION
This command passes its arguments on to gdb "print" command for evaluation.
expression an expression to be evaluated.
symbol a kernel symbol.
:cpuspec when appended to a symbol with a colon, a CPU specification
for per-cpu variables:
: CPU of the currently selected task.
:a[ll] all CPUs.
:#[-#][,...] CPU list(s), e.g. "1,3,5", "1-3",
or "1,3,5-7,10".
-x override default output format with hexadecimal format.
-d override default output format with decimal format.
-u the expression evaluates to a user address reference.
...
And maybe a minor indenting change for the cpu/address values,
from this:
crash> struct desc_ptr b0c8:1,3
[1]: ffff88021e24b0c8
struct desc_ptr {
size = 0,
address = 0
}
[3]: ffff88021e2cb0c8
struct desc_ptr {
size = 0,
address = 0
}
crash>
to this:
crash> struct desc_ptr b0c8:1,3
[1]: ffff88021e24b0c8
struct desc_ptr {
size = 0,
address = 0
}
[3]: ffff88021e2cb0c8
struct desc_ptr {
size = 0,
address = 0
}
crash>
But other than that, it looks good to go. Do you have anything
more to add?
Thanks,
Dave
7:50 a.m.
On Wed, 23 Oct 2013 15:37:42 -0400 (EDT)
Dave Anderson <anderson(a)redhat.com> wrote:
----- Original Message -----
> Hi Dave,
>
> I'm sorry for the last submission. It seems I forgot to refresh the
> patches, so it was completely bogus. Should be fixed now. I'm also
> attaching my changes as one big patch to this message.
>
> Petr Tesarik
Hi Petr,
Hi Dave,
I've reviewed the code changes, and have been beating on the
patch, and I can't get it break. Really nice work...
[...]
I'd change it to this:
crash> help p
NAME
p - print the value of an expression
SYNOPSIS
p [-x|-d][-u] [expression | symbol[:cpuspec]]
Yes! It shows that cpuspec cannot be specified for arbitrary
expressions. In fact, I thought about implementing that feature,
but I doubt I could get it to work without patching gdb itself.
[...]
And maybe a minor indenting change for the cpu/address values,
from this:
struct desc_ptr {
crash> struct desc_ptr b0c8:1,3
[1]: ffff88021e24b0c8
struct desc_ptr {
[...]
to this:
crash> struct desc_ptr b0c8:1,3
[1]: ffff88021e24b0c8
struct desc_ptr {
I copied this code verbatim from the "p" command, so the extra spaces
are forgotten rather than intended.
[...]
But other than that, it looks good to go. Do you have anything
more to add?
No, it looks good now. Should I refresh the patch set, or can you
make the cosmetic changes in your tree yourself?
Petr Tesarik
9:04 a.m.
----- Original Message -----
On Wed, 23 Oct 2013 15:37:42 -0400 (EDT)
Dave Anderson <anderson(a)redhat.com> wrote:
> ----- Original Message -----
> > Hi Dave,
> >
> > I'm sorry for the last submission. It seems I forgot to refresh the
> > patches, so it was completely bogus. Should be fixed now. I'm also
> > attaching my changes as one big patch to this message.
> >
> > Petr Tesarik
>
> Hi Petr,
Hi Dave,
> I've reviewed the code changes, and have been beating on the
> patch, and I can't get it break. Really nice work...
>
>[...]
>
> I'd change it to this:
> crash> help p
>
> NAME
> p - print the value of an expression
>
> SYNOPSIS
> p [-x|-d][-u] [expression | symbol[:cpuspec]]
Yes! It shows that cpuspec cannot be specified for arbitrary
expressions. In fact, I thought about implementing that feature,
but I doubt I could get it to work without patching gdb itself.
>[...]
> And maybe a minor indenting change for the cpu/address values,
> from this:
> struct desc_ptr {
> crash> struct desc_ptr b0c8:1,3
> [1]: ffff88021e24b0c8
> struct desc_ptr {
>[...]
> to this:
>
> crash> struct desc_ptr b0c8:1,3
> [1]: ffff88021e24b0c8
> struct desc_ptr {
I copied this code verbatim from the "p" command, so the extra spaces
are forgotten rather than intended.
>[...]
> But other than that, it looks good to go. Do you have anything
> more to add?
No, it looks good now. Should I refresh the patch set, or can you
make the cosmetic changes in your tree yourself?
No, I'll take it from here -- consider it queued for crash-7.0.3.
And thanks again for all your work on this, it's really a significant
enhancement.
Dave
9:56 a.m.
On Thu, 24 Oct 2013 10:04:43 -0400 (EDT)
Dave Anderson <anderson(a)redhat.com> wrote:
----- Original Message -----
> On Wed, 23 Oct 2013 15:37:42 -0400 (EDT)
> Dave Anderson <anderson(a)redhat.com> wrote:
>
> > ----- Original Message -----
> > > Hi Dave,
> > >
> > > I'm sorry for the last submission. It seems I forgot to refresh the
> > > patches, so it was completely bogus. Should be fixed now. I'm also
> > > attaching my changes as one big patch to this message.
> > >
> > > Petr Tesarik
> >
> > Hi Petr,
>
> Hi Dave,
>
> > I've reviewed the code changes, and have been beating on the
> > patch, and I can't get it break. Really nice work...
> >
> >[...]
> >
> > I'd change it to this:
> > crash> help p
> >
> > NAME
> > p - print the value of an expression
> >
> > SYNOPSIS
> > p [-x|-d][-u] [expression | symbol[:cpuspec]]
>
> Yes! It shows that cpuspec cannot be specified for arbitrary
> expressions. In fact, I thought about implementing that feature,
> but I doubt I could get it to work without patching gdb itself.
>
> >[...]
> > And maybe a minor indenting change for the cpu/address values,
> > from this:
> > struct desc_ptr {
> > crash> struct desc_ptr b0c8:1,3
> > [1]: ffff88021e24b0c8
> > struct desc_ptr {
> >[...]
>
> > to this:
> >
> > crash> struct desc_ptr b0c8:1,3
> > [1]: ffff88021e24b0c8
> > struct desc_ptr {
>
> I copied this code verbatim from the "p" command, so the extra spaces
> are forgotten rather than intended.
>
> >[...]
> > But other than that, it looks good to go. Do you have anything
> > more to add?
>
> No, it looks good now. Should I refresh the patch set, or can you
> make the cosmetic changes in your tree yourself?
No, I'll take it from here -- consider it queued for crash-7.0.3.
Thank you! This is the best possible ending to my SUSE Hackweek X
mini-project. ;-)
Petr Tesarik
4215
days inactive
4218
days old
devel@lists.crash-utility.osci.io
15 comments
2 participants
participants (2)
-
Dave Anderson -
Petr Tesarik