9:40 a.m.
Hi Dave,
No functionality change (hopefully). This patch is essentially a clean up
in preparation to introduce the "-f" option.
Please let me know your thoughts.
Signed-off-by: Aaron Tomlin <atomlin(a)redhat.com>
---
kernel.c | 265 ++++++++++++++++++++++-----------------------------------------
1 file changed, 91 insertions(+), 174 deletions(-)
diff --git a/kernel.c b/kernel.c
index 73d1983..3107014 100644
--- a/kernel.c
+++ b/kernel.c
@@ -1589,212 +1589,129 @@ cmd_dis(void)
return;
}
- do_load_module_filter = module_symbol(req->addr, NULL, NULL,
- NULL, *gdb_output_radix);
+ req->command = GNU_RESOLVE_TEXT_ADDR;
+ gdb_interface(req);
+ req->flags &= ~GNU_COMMAND_FAILED;
+ if (reverse || req->flags & GNU_FUNCTION_ONLY) {
+ if (sp) {
+ savename = sp->name;
+ if ((sp = next_symbol(NULL, sp)))
+ req->addr2 = sp->value;
+ else
+ error(FATAL,
+ "unable to determine symbol after %s\n",
+ savename);
+ } else {
+ if ((sp = value_search(req->addr, NULL))
+ && (sp = next_symbol(NULL, sp)))
+ req->addr2 = sp->value;
+ else
+ error(FATAL, dis_err, req->addr);
+ }
+ }
- if (!reverse) {
- req->command = GNU_RESOLVE_TEXT_ADDR;
- gdb_interface(req);
- if ((req->flags & GNU_COMMAND_FAILED) ||
- do_load_module_filter ||
- (req->flags & GNU_FUNCTION_ONLY)) {
- req->flags &= ~GNU_COMMAND_FAILED;
- if (sp) {
- savename = sp->name;
- if ((sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else
- error(FATAL,
- "unable to determine symbol after %s\n",
- savename);
- } else {
- if ((sp = value_search(req->addr, NULL))
- && (sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else
- error(FATAL, dis_err, req->addr);
- }
- }
+ if (reverse) {
+ revtarget = req->addr;
+ if ((sp = value_search(revtarget, NULL)) == NULL)
+ error(FATAL, "cannot resolve address: %lx\n", revtarget);
- do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
+ sprintf(buf1, "0x%lx", revtarget);
+ req->addr = sp->value;
+ } else
count = 0;
- open_tmpfile();
+ do_load_module_filter = module_symbol(req->addr, NULL, NULL,
+ NULL, *gdb_output_radix);
+
+ do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
+ open_tmpfile();
#ifdef OLDWAY
- req->command = GNU_DISASSEMBLE;
- req->fp = pc->tmpfile;
- gdb_interface(req);
+ req->command = GNU_DISASSEMBLE;
+ req->fp = pc->tmpfile;
+ gdb_interface(req);
#else
- sprintf(buf1, "x/%ldi 0x%lx",
- count_entered && req->count ? req->count :
+ if (reverse)
+ sprintf(buf5, "x/%ldi 0x%lx",
+ (revtarget - req->addr) ? revtarget - req->addr : 1,
+ req->addr);
+ else
+ sprintf(buf5, "x/%ldi 0x%lx",
+ count_entered && req->count ? req->count :
req->flags & GNU_FUNCTION_ONLY ?
req->addr2 - req->addr : 1,
req->addr);
- gdb_pass_through(buf1, NULL, GNU_RETURN_ON_ERROR);
+ gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
#endif
- if (req->flags & GNU_COMMAND_FAILED) {
- close_tmpfile();
- error(FATAL, dis_err, req->addr);
- }
+ if (req->flags & GNU_COMMAND_FAILED) {
+ close_tmpfile();
+ error(FATAL, dis_err, req->addr);
+ }
- rewind(pc->tmpfile);
- while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
- if (STRNEQ(buf2, "Dump of") ||
- STRNEQ(buf2, "End of"))
- continue;
+ rewind(pc->tmpfile);
+ while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
+ if (STRNEQ(buf2, "Dump of") ||
+ STRNEQ(buf2, "End of"))
+ continue;
- strip_beginning_whitespace(buf2);
+ strip_beginning_whitespace(buf2);
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
+ if (do_load_module_filter)
+ load_module_filter(buf2, LM_DIS_FILTER);
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ if (STRNEQ(buf2, "0x"))
+ extract_hex(buf2, &curaddr, ':', TRUE);
+ if (!reverse)
if ((req->flags & GNU_FUNCTION_ONLY) &&
(curaddr >= req->addr2))
break;
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
-
- if (req->flags & GNU_FUNCTION_ONLY) {
- if (req->flags &
- GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3,
- FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(
- pc->saved_fp, buf3);
- print_verbatim(
- pc->saved_fp,
"\n");
- strcpy(buf4, buf3);
- }
- }
-
- print_verbatim(pc->saved_fp, buf2);
- continue;
- } else {
- if (curaddr < req->addr)
- continue;
+ if (do_machdep_filter)
+ machdep->dis_filter(curaddr, buf2, radix);
- if (req->flags &
- GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3,
- FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(
- pc->saved_fp, buf3);
- print_verbatim(
- pc->saved_fp, "\n");
- strcpy(buf4, buf3);
- }
- }
-
- print_verbatim(pc->saved_fp, buf2);
-
- if (LASTCHAR(clean_line(buf2))
- != ':') {
- if (++count == req->count)
- break;
- }
+ if (req->flags & GNU_PRINT_LINE_NUMBERS) {
+ get_line_number(curaddr, buf3,
+ FALSE);
+ if (!STREQ(buf3, buf4)) {
+ print_verbatim(
+ pc->saved_fp, buf3);
+ print_verbatim(
+ pc->saved_fp, "\n");
+ strcpy(buf4, buf3);
}
- }
- close_tmpfile();
- }
- }
- else if (bug_bytes_entered)
- return;
- else cmd_usage(pc->curcmd, SYNOPSIS);
-
- if (!reverse) {
- FREEBUF(req->buf);
- FREEBUF(req);
- return;
- }
-
- revtarget = req->addr;
- if ((sp = value_search(revtarget, NULL)) == NULL)
- error(FATAL, "cannot resolve address: %lx\n", revtarget);
-
- sprintf(buf1, "0x%lx", revtarget);
-
- open_tmpfile();
-
- req->addr = sp->value;
- req->flags |= GNU_FUNCTION_ONLY;
- req->command = GNU_RESOLVE_TEXT_ADDR;
- gdb_interface(req);
- req->flags &= ~GNU_COMMAND_FAILED;
- savename = sp->name;
- if ((sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else {
- close_tmpfile();
- error(FATAL, "unable to determine symbol after %s\n",
savename);
- }
-
- do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
-#ifdef OLDWAY
- req->command = GNU_DISASSEMBLE;
- req->fp = pc->tmpfile;
- gdb_interface(req);
-#else
- sprintf(buf5, "x/%ldi 0x%lx",
- (revtarget - req->addr) ? revtarget - req->addr : 1,
- req->addr);
- gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
-#endif
- if (req->flags & GNU_COMMAND_FAILED) {
- close_tmpfile();
- error(FATAL, dis_err, req->addr);
- }
-
- rewind(pc->tmpfile);
- while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
- if (STRNEQ(buf2, "Dump of") || STRNEQ(buf2, "End
of"))
- continue;
+ }
- strip_beginning_whitespace(buf2);
+ print_verbatim(pc->saved_fp, buf2);
+ if (reverse) {
+ if (STRNEQ(buf2, buf1)) {
+ if (LASTCHAR(clean_line(buf2)) != ':')
+ break;
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
+ ret = fgets(buf2, BUFSIZE, pc->tmpfile);
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ if (do_load_module_filter)
+ load_module_filter(buf2, LM_DIS_FILTER);
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
+ if (do_machdep_filter)
+ machdep->dis_filter(curaddr, buf2, radix);
- if (req->flags & GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3, FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(pc->saved_fp, buf3);
- print_verbatim(pc->saved_fp, "\n");
- strcpy(buf4, buf3);
+ print_verbatim(pc->saved_fp, buf2);
+ break;
+ }
}
- }
- print_verbatim(pc->saved_fp, buf2);
- if (STRNEQ(buf2, buf1)) {
- if (LASTCHAR(clean_line(buf2)) != ':')
- break;
-
- ret = fgets(buf2, BUFSIZE, pc->tmpfile);
-
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
-
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
-
- print_verbatim(pc->saved_fp, buf2);
- break;
+ if (count_entered && LASTCHAR(clean_line(buf2)) != ':')
+ if (++count == req->count)
+ break;
}
+ close_tmpfile();
}
+ else if (bug_bytes_entered)
+ return;
+ else cmd_usage(pc->curcmd, SYNOPSIS);
- close_tmpfile();
FREEBUF(req->buf);
FREEBUF(req);
+ return;
}
/*
--
2.4.3
1:20 p.m.
----- Original Message -----
Hi Dave,
No functionality change (hopefully). This patch is essentially a clean up
in preparation to introduce the "-f" option.
Please let me know your thoughts.
Honestly, I've looked at the patch both in text mode and in a graphical diff, and I
have
to say, it's really difficult to understand exactly what the patch accomplishes.
Things
seem to work as expected AFAICT.
I don't understand why it requires so much churn? It would seemingly be a simple
enough task to capture the -f and starting address, and then simply do a disassembly
of the entire function, but only display the output when it arrives at the target
address.
Dave
Signed-off-by: Aaron Tomlin <atomlin(a)redhat.com>
---
kernel.c | 265
++++++++++++++++++++++-----------------------------------------
1 file changed, 91 insertions(+), 174 deletions(-)
diff --git a/kernel.c b/kernel.c
index 73d1983..3107014 100644
--- a/kernel.c
+++ b/kernel.c
@@ -1589,212 +1589,129 @@ cmd_dis(void)
return;
}
- do_load_module_filter = module_symbol(req->addr, NULL, NULL,
- NULL, *gdb_output_radix);
+ req->command = GNU_RESOLVE_TEXT_ADDR;
+ gdb_interface(req);
+ req->flags &= ~GNU_COMMAND_FAILED;
+ if (reverse || req->flags & GNU_FUNCTION_ONLY) {
+ if (sp) {
+ savename = sp->name;
+ if ((sp = next_symbol(NULL, sp)))
+ req->addr2 = sp->value;
+ else
+ error(FATAL,
+ "unable to determine symbol after %s\n",
+ savename);
+ } else {
+ if ((sp = value_search(req->addr, NULL))
+ && (sp = next_symbol(NULL, sp)))
+ req->addr2 = sp->value;
+ else
+ error(FATAL, dis_err, req->addr);
+ }
+ }
- if (!reverse) {
- req->command = GNU_RESOLVE_TEXT_ADDR;
- gdb_interface(req);
- if ((req->flags & GNU_COMMAND_FAILED) ||
- do_load_module_filter ||
- (req->flags & GNU_FUNCTION_ONLY)) {
- req->flags &= ~GNU_COMMAND_FAILED;
- if (sp) {
- savename = sp->name;
- if ((sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else
- error(FATAL,
- "unable to determine symbol after %s\n",
- savename);
- } else {
- if ((sp = value_search(req->addr, NULL))
- && (sp = next_symbol(NULL,
sp)))
- req->addr2 = sp->value;
- else
- error(FATAL, dis_err, req->addr);
- }
- }
+ if (reverse) {
+ revtarget = req->addr;
+ if ((sp = value_search(revtarget, NULL)) == NULL)
+ error(FATAL, "cannot resolve address: %lx\n", revtarget);
- do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
+ sprintf(buf1, "0x%lx", revtarget);
+ req->addr = sp->value;
+ } else
count = 0;
- open_tmpfile();
+ do_load_module_filter = module_symbol(req->addr, NULL, NULL,
+ NULL, *gdb_output_radix);
+
+ do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
+ open_tmpfile();
#ifdef OLDWAY
- req->command = GNU_DISASSEMBLE;
- req->fp = pc->tmpfile;
- gdb_interface(req);
+ req->command = GNU_DISASSEMBLE;
+ req->fp = pc->tmpfile;
+ gdb_interface(req);
#else
- sprintf(buf1, "x/%ldi 0x%lx",
- count_entered && req->count ? req->count
:
+ if (reverse)
+ sprintf(buf5, "x/%ldi 0x%lx",
+ (revtarget - req->addr) ? revtarget - req->addr : 1,
+ req->addr);
+ else
+ sprintf(buf5, "x/%ldi 0x%lx",
+ count_entered && req->count ? req->count :
req->flags & GNU_FUNCTION_ONLY ?
req->addr2 - req->addr : 1,
req->addr);
- gdb_pass_through(buf1, NULL, GNU_RETURN_ON_ERROR);
+ gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
#endif
- if (req->flags & GNU_COMMAND_FAILED) {
- close_tmpfile();
- error(FATAL, dis_err, req->addr);
- }
+ if (req->flags & GNU_COMMAND_FAILED) {
+ close_tmpfile();
+ error(FATAL, dis_err, req->addr);
+ }
- rewind(pc->tmpfile);
- while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
- if (STRNEQ(buf2, "Dump of") ||
- STRNEQ(buf2, "End of"))
- continue;
+ rewind(pc->tmpfile);
+ while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
+ if (STRNEQ(buf2, "Dump of") ||
+ STRNEQ(buf2, "End of"))
+ continue;
- strip_beginning_whitespace(buf2);
+ strip_beginning_whitespace(buf2);
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
+ if (do_load_module_filter)
+ load_module_filter(buf2, LM_DIS_FILTER);
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ if (STRNEQ(buf2, "0x"))
+ extract_hex(buf2, &curaddr, ':', TRUE);
+ if (!reverse)
if ((req->flags & GNU_FUNCTION_ONLY) &&
(curaddr >= req->addr2))
break;
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
-
- if (req->flags & GNU_FUNCTION_ONLY) {
- if (req->flags &
- GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr,
buf3,
- FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(
- pc->saved_fp,
buf3);
- print_verbatim(
- pc->saved_fp,
"\n");
- strcpy(buf4, buf3);
- }
- }
-
- print_verbatim(pc->saved_fp, buf2);
- continue;
- } else {
- if (curaddr < req->addr)
- continue;
+ if (do_machdep_filter)
+ machdep->dis_filter(curaddr, buf2, radix);
- if (req->flags &
- GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3,
- FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(
- pc->saved_fp, buf3);
- print_verbatim(
- pc->saved_fp, "\n");
- strcpy(buf4, buf3);
- }
- }
-
- print_verbatim(pc->saved_fp, buf2);
-
- if (LASTCHAR(clean_line(buf2))
- != ':') {
- if (++count == req->count)
- break;
- }
+ if (req->flags & GNU_PRINT_LINE_NUMBERS) {
+ get_line_number(curaddr, buf3,
+ FALSE);
+ if (!STREQ(buf3, buf4)) {
+ print_verbatim(
+ pc->saved_fp, buf3);
+ print_verbatim(
+ pc->saved_fp, "\n");
+ strcpy(buf4, buf3);
}
- }
- close_tmpfile();
- }
- }
- else if (bug_bytes_entered)
- return;
- else cmd_usage(pc->curcmd, SYNOPSIS);
-
- if (!reverse) {
- FREEBUF(req->buf);
- FREEBUF(req);
- return;
- }
-
- revtarget = req->addr;
- if ((sp = value_search(revtarget, NULL)) == NULL)
- error(FATAL, "cannot resolve address: %lx\n", revtarget);
-
- sprintf(buf1, "0x%lx", revtarget);
-
- open_tmpfile();
-
- req->addr = sp->value;
- req->flags |= GNU_FUNCTION_ONLY;
- req->command = GNU_RESOLVE_TEXT_ADDR;
- gdb_interface(req);
- req->flags &= ~GNU_COMMAND_FAILED;
- savename = sp->name;
- if ((sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else {
- close_tmpfile();
- error(FATAL, "unable to determine symbol after %s\n",
savename);
- }
-
- do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
-#ifdef OLDWAY
- req->command = GNU_DISASSEMBLE;
- req->fp = pc->tmpfile;
- gdb_interface(req);
-#else
- sprintf(buf5, "x/%ldi 0x%lx",
- (revtarget - req->addr) ? revtarget - req->addr : 1,
- req->addr);
- gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
-#endif
- if (req->flags & GNU_COMMAND_FAILED) {
- close_tmpfile();
- error(FATAL, dis_err, req->addr);
- }
-
- rewind(pc->tmpfile);
- while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
- if (STRNEQ(buf2, "Dump of") || STRNEQ(buf2, "End
of"))
- continue;
+ }
- strip_beginning_whitespace(buf2);
+ print_verbatim(pc->saved_fp, buf2);
+ if (reverse) {
+ if (STRNEQ(buf2, buf1)) {
+ if (LASTCHAR(clean_line(buf2)) != ':')
+ break;
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
+ ret = fgets(buf2, BUFSIZE, pc->tmpfile);
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ if (do_load_module_filter)
+ load_module_filter(buf2, LM_DIS_FILTER);
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
+ if (do_machdep_filter)
+ machdep->dis_filter(curaddr, buf2, radix);
- if (req->flags & GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3, FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(pc->saved_fp, buf3);
- print_verbatim(pc->saved_fp, "\n");
- strcpy(buf4, buf3);
+ print_verbatim(pc->saved_fp, buf2);
+ break;
+ }
}
- }
- print_verbatim(pc->saved_fp, buf2);
- if (STRNEQ(buf2, buf1)) {
- if (LASTCHAR(clean_line(buf2)) != ':')
- break;
-
- ret = fgets(buf2, BUFSIZE, pc->tmpfile);
-
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
-
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
-
- print_verbatim(pc->saved_fp, buf2);
- break;
+ if (count_entered && LASTCHAR(clean_line(buf2)) != ':')
+ if (++count == req->count)
+ break;
}
+ close_tmpfile();
}
+ else if (bug_bytes_entered)
+ return;
+ else cmd_usage(pc->curcmd, SYNOPSIS);
- close_tmpfile();
FREEBUF(req->buf);
FREEBUF(req);
+ return;
}
/*
--
2.4.3
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
7:45 a.m.
On Tue 2015-08-04 14:20 -0400, Dave Anderson wrote:
> Hi Dave,
>
> No functionality change (hopefully). This patch is essentially a clean up
> in preparation to introduce the "-f" option.
>
> Please let me know your thoughts.
Honestly, I've looked at the patch both in text mode and in a graphical
diff, and I have to say, it's really difficult to understand exactly what
the patch accomplishes. Things seem to work as expected AFAICT.
I wanted to remove most of code duplication and somewhat reduce the
complexity of the function. This is the sole purpose of this patch.
With regards to the "-f" option feature, I would prefer to build on this
version.
--
Aaron Tomlin
8:10 a.m.
----- Original Message -----
On Tue 2015-08-04 14:20 -0400, Dave Anderson wrote:
> > Hi Dave,
> >
> > No functionality change (hopefully). This patch is essentially a clean up
> > in preparation to introduce the "-f" option.
> >
> > Please let me know your thoughts.
>
> Honestly, I've looked at the patch both in text mode and in a graphical
> diff, and I have to say, it's really difficult to understand exactly what
> the patch accomplishes. Things seem to work as expected AFAICT.
I wanted to remove most of code duplication and somewhat reduce the
complexity of the function. This is the sole purpose of this patch.
With regards to the "-f" option feature, I would prefer to build on this
version.
--
Aaron Tomlin
OK. I did notice that it managed to cut ~70 lines. I'll do a side-by-side
comparison sanity check and continue testing it.
Anyway, the -f implementation on top of it should be fairly trivial, correct?
It should essentially be the same as GNU_FUNCTION_ONLY, but with an additional
"continue" in the lower while loop until curaddr reaches the starting address,
right?
Dave
9:22 a.m.
On Wed 2015-08-05 09:10 -0400, Dave Anderson wrote:
----- Original Message -----
> On Tue 2015-08-04 14:20 -0400, Dave Anderson wrote:
> > > Hi Dave,
> > >
> > > No functionality change (hopefully). This patch is essentially a clean up
> > > in preparation to introduce the "-f" option.
> > >
> > > Please let me know your thoughts.
> >
> > Honestly, I've looked at the patch both in text mode and in a graphical
> > diff, and I have to say, it's really difficult to understand exactly what
> > the patch accomplishes. Things seem to work as expected AFAICT.
>
> I wanted to remove most of code duplication and somewhat reduce the
> complexity of the function. This is the sole purpose of this patch.
> With regards to the "-f" option feature, I would prefer to build on this
> version.
>
> --
> Aaron Tomlin
OK. I did notice that it managed to cut ~70 lines. I'll do a side-by-side
comparison sanity check and continue testing it.
Thanks Dave.
Anyway, the -f implementation on top of it should be fairly trivial, correct?
It should essentially be the same as GNU_FUNCTION_ONLY, but with an additional
"continue" in the lower while loop until curaddr reaches the starting address,
right?
Yes -- until curaddr matches the given target address then proceed.
--
Aaron Tomlin
4:04 p.m.
----- Original Message -----
> On Tue 2015-08-04 14:20 -0400, Dave Anderson wrote:
> > > > Hi Dave,
> > > >
> > > > No functionality change (hopefully). This patch is essentially a
clean up
> > > > in preparation to introduce the "-f" option.
> > > >
> > > > Please let me know your thoughts.
> > >
> > > Honestly, I've looked at the patch both in text mode and in a
graphical
> > > diff, and I have to say, it's really difficult to understand exactly
what
> > > the patch accomplishes. Things seem to work as expected AFAICT.
> >
> > I wanted to remove most of code duplication and somewhat reduce the
> > complexity of the function. This is the sole purpose of this patch.
> > With regards to the "-f" option feature, I would prefer to build on
this
> > version.
> >
> > --
> > Aaron Tomlin
>
> OK. I did notice that it managed to cut ~70 lines. I'll do a side-by-side
> comparison sanity check and continue testing it.
Thanks Dave.
>
> Anyway, the -f implementation on top of it should be fairly trivial, correct?
> It should essentially be the same as GNU_FUNCTION_ONLY, but with an additional
> "continue" in the lower while loop until curaddr reaches the starting
address,
> right?
Yes -- until curaddr matches the given target address then proceed.
--
Aaron Tomlin
OK, I see what you've done, and "Consolidate" makes perfect sense to me now.
While you're at it, the crufty "#ifdef OLDWAY" section should be removed as
well,
since it's just a waste of space.
Since the -f implementation should be simple, do you have a problem with
posting a single patchset that does both the consolidation and the new
functionality (and thereby would have a single changelog entry)?
Thanks,
Dave
2:47 a.m.
On Wed 2015-08-05 17:04 -0400, Dave Anderson wrote:
[ ... ]
OK, I see what you've done, and "Consolidate" makes
perfect sense to me now.
While you're at it, the crufty "#ifdef OLDWAY" section should be removed as
well,
since it's just a waste of space.
OK.
Since the -f implementation should be simple, do you have a problem
with
posting a single patchset that does both the consolidation and the new
functionality (and thereby would have a single changelog entry)?
That's no problem at all.
--
Aaron Tomlin
6:30 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
Dave,
Since our last discussion [1], a further change to the "consolidation"
patch has been made. The assumption now is that the text address generated
by gdb is _always_ in base 16 form. Hence the if clause to check buf2 is
prefixed with 0x has been removed as indicated in the hunk below:
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ extract_hex(buf2, &curaddr, ':', TRUE);
Please let me know if this assumption is valid.
With optimisation level 2 (default) the following warning is displayed:
$ make warn
TARGET: X86_64
CRASH: 7.1.2
GDB: 7.6
cc -c -g -DX86_64 -DGDB_7_6 build_data.c -Wall -O2 -Wstrict-prototypes
-Wmissing-prototypes -fstack-protector -Wformat-security
cc -c -g -DX86_64 -DGDB_7_6 kernel.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes
-fstack-protector -Wformat-security
kernel.c: In function ‘cmd_dis’:
kernel.c:1663:8: warning: ‘target’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
if (curaddr != target)
^
Note that this is not seen when optimisation is disabled. To work around
this (what I suspect is a false positive), where target is declared it is
now initialised to zero even though its value is not considered if forward
or reverse is not set.
[1]: https://www.redhat.com/archives/crash-utility/2015-August/msg00025.html
Aaron Tomlin (2):
dis: Consolidate cmd_dis
dis: Introduce the -f option
help.c | 2 +
kernel.c | 287 +++++++++++++++++++++++----------------------------------------
2 files changed, 106 insertions(+), 183 deletions(-)
--
2.4.3
6:30 a.m.
New subject: [PATCH 1/2] dis: Consolidate cmd_dis
No functionality change. The purpose of this patch is to simply remove most
of the code duplication and somewhat reduce the complexity of the function.
This is in preparation for the "-f" option feature.
Signed-off-by: Aaron Tomlin <atomlin(a)redhat.com>
---
kernel.c | 264 +++++++++++++++++++++------------------------------------------
1 file changed, 88 insertions(+), 176 deletions(-)
diff --git a/kernel.c b/kernel.c
index 73d1983..846cf71 100644
--- a/kernel.c
+++ b/kernel.c
@@ -1589,212 +1589,124 @@ cmd_dis(void)
return;
}
+ req->command = GNU_RESOLVE_TEXT_ADDR;
+ gdb_interface(req);
+ req->flags &= ~GNU_COMMAND_FAILED;
+ if (reverse || req->flags & GNU_FUNCTION_ONLY) {
+ if (sp) {
+ savename = sp->name;
+ if ((sp = next_symbol(NULL, sp)))
+ req->addr2 = sp->value;
+ else
+ error(FATAL,
+ "unable to determine symbol after %s\n",
+ savename);
+ } else {
+ if ((sp = value_search(req->addr, NULL))
+ && (sp = next_symbol(NULL, sp)))
+ req->addr2 = sp->value;
+ else
+ error(FATAL, dis_err, req->addr);
+ }
+ }
+
+ if (reverse) {
+ revtarget = req->addr;
+ if ((sp = value_search(revtarget, NULL)) == NULL)
+ error(FATAL, "cannot resolve address: %lx\n", revtarget);
+
+ sprintf(buf1, "0x%lx", revtarget);
+ req->addr = sp->value;
+ } else
+ count = 0;
do_load_module_filter = module_symbol(req->addr, NULL, NULL,
NULL, *gdb_output_radix);
- if (!reverse) {
- req->command = GNU_RESOLVE_TEXT_ADDR;
- gdb_interface(req);
- if ((req->flags & GNU_COMMAND_FAILED) ||
- do_load_module_filter ||
- (req->flags & GNU_FUNCTION_ONLY)) {
- req->flags &= ~GNU_COMMAND_FAILED;
- if (sp) {
- savename = sp->name;
- if ((sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else
- error(FATAL,
- "unable to determine symbol after %s\n",
- savename);
- } else {
- if ((sp = value_search(req->addr, NULL))
- && (sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else
- error(FATAL, dis_err, req->addr);
- }
- }
+ do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
+ open_tmpfile();
- do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
- count = 0;
- open_tmpfile();
-#ifdef OLDWAY
- req->command = GNU_DISASSEMBLE;
- req->fp = pc->tmpfile;
- gdb_interface(req);
-#else
- sprintf(buf1, "x/%ldi 0x%lx",
- count_entered && req->count ? req->count :
+ if (reverse)
+ sprintf(buf5, "x/%ldi 0x%lx",
+ (revtarget - req->addr) ? revtarget - req->addr : 1,
+ req->addr);
+ else
+ sprintf(buf5, "x/%ldi 0x%lx",
+ count_entered && req->count ? req->count :
req->flags & GNU_FUNCTION_ONLY ?
req->addr2 - req->addr : 1,
req->addr);
- gdb_pass_through(buf1, NULL, GNU_RETURN_ON_ERROR);
-#endif
- if (req->flags & GNU_COMMAND_FAILED) {
- close_tmpfile();
- error(FATAL, dis_err, req->addr);
- }
+ gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
- rewind(pc->tmpfile);
- while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
- if (STRNEQ(buf2, "Dump of") ||
- STRNEQ(buf2, "End of"))
- continue;
+ if (req->flags & GNU_COMMAND_FAILED) {
+ close_tmpfile();
+ error(FATAL, dis_err, req->addr);
+ }
+
+ rewind(pc->tmpfile);
+ while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
+ if (STRNEQ(buf2, "Dump of") ||
+ STRNEQ(buf2, "End of"))
+ continue;
- strip_beginning_whitespace(buf2);
+ strip_beginning_whitespace(buf2);
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
+ if (do_load_module_filter)
+ load_module_filter(buf2, LM_DIS_FILTER);
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ extract_hex(buf2, &curaddr, ':', TRUE);
+ if (!reverse)
if ((req->flags & GNU_FUNCTION_ONLY) &&
(curaddr >= req->addr2))
break;
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
-
- if (req->flags & GNU_FUNCTION_ONLY) {
- if (req->flags &
- GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3,
- FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(
- pc->saved_fp, buf3);
- print_verbatim(
- pc->saved_fp,
"\n");
- strcpy(buf4, buf3);
- }
- }
-
- print_verbatim(pc->saved_fp, buf2);
- continue;
- } else {
- if (curaddr < req->addr)
- continue;
+ if (do_machdep_filter)
+ machdep->dis_filter(curaddr, buf2, radix);
- if (req->flags &
- GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3,
- FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(
- pc->saved_fp, buf3);
- print_verbatim(
- pc->saved_fp, "\n");
- strcpy(buf4, buf3);
- }
- }
-
- print_verbatim(pc->saved_fp, buf2);
-
- if (LASTCHAR(clean_line(buf2))
- != ':') {
- if (++count == req->count)
- break;
- }
+ if (req->flags & GNU_PRINT_LINE_NUMBERS) {
+ get_line_number(curaddr, buf3,
+ FALSE);
+ if (!STREQ(buf3, buf4)) {
+ print_verbatim(
+ pc->saved_fp, buf3);
+ print_verbatim(
+ pc->saved_fp, "\n");
+ strcpy(buf4, buf3);
}
- }
- close_tmpfile();
- }
- }
- else if (bug_bytes_entered)
- return;
- else cmd_usage(pc->curcmd, SYNOPSIS);
-
- if (!reverse) {
- FREEBUF(req->buf);
- FREEBUF(req);
- return;
- }
-
- revtarget = req->addr;
- if ((sp = value_search(revtarget, NULL)) == NULL)
- error(FATAL, "cannot resolve address: %lx\n", revtarget);
-
- sprintf(buf1, "0x%lx", revtarget);
-
- open_tmpfile();
-
- req->addr = sp->value;
- req->flags |= GNU_FUNCTION_ONLY;
- req->command = GNU_RESOLVE_TEXT_ADDR;
- gdb_interface(req);
- req->flags &= ~GNU_COMMAND_FAILED;
- savename = sp->name;
- if ((sp = next_symbol(NULL, sp)))
- req->addr2 = sp->value;
- else {
- close_tmpfile();
- error(FATAL, "unable to determine symbol after %s\n",
savename);
- }
-
- do_machdep_filter = machdep->dis_filter(req->addr, NULL, radix);
-#ifdef OLDWAY
- req->command = GNU_DISASSEMBLE;
- req->fp = pc->tmpfile;
- gdb_interface(req);
-#else
- sprintf(buf5, "x/%ldi 0x%lx",
- (revtarget - req->addr) ? revtarget - req->addr : 1,
- req->addr);
- gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
-#endif
- if (req->flags & GNU_COMMAND_FAILED) {
- close_tmpfile();
- error(FATAL, dis_err, req->addr);
- }
-
- rewind(pc->tmpfile);
- while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
- if (STRNEQ(buf2, "Dump of") || STRNEQ(buf2, "End
of"))
- continue;
+ }
- strip_beginning_whitespace(buf2);
+ print_verbatim(pc->saved_fp, buf2);
+ if (reverse) {
+ if (STRNEQ(buf2, buf1)) {
+ if (LASTCHAR(clean_line(buf2)) != ':')
+ break;
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
+ ret = fgets(buf2, BUFSIZE, pc->tmpfile);
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ if (do_load_module_filter)
+ load_module_filter(buf2, LM_DIS_FILTER);
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
+ if (do_machdep_filter)
+ machdep->dis_filter(curaddr, buf2, radix);
- if (req->flags & GNU_PRINT_LINE_NUMBERS) {
- get_line_number(curaddr, buf3, FALSE);
- if (!STREQ(buf3, buf4)) {
- print_verbatim(pc->saved_fp, buf3);
- print_verbatim(pc->saved_fp, "\n");
- strcpy(buf4, buf3);
+ print_verbatim(pc->saved_fp, buf2);
+ break;
+ }
}
- }
-
- print_verbatim(pc->saved_fp, buf2);
- if (STRNEQ(buf2, buf1)) {
- if (LASTCHAR(clean_line(buf2)) != ':')
- break;
-
- ret = fgets(buf2, BUFSIZE, pc->tmpfile);
-
- if (do_load_module_filter)
- load_module_filter(buf2, LM_DIS_FILTER);
- if (do_machdep_filter)
- machdep->dis_filter(curaddr, buf2, radix);
-
- print_verbatim(pc->saved_fp, buf2);
- break;
+ if (count_entered && LASTCHAR(clean_line(buf2)) != ':')
+ if (++count == req->count)
+ break;
}
+ close_tmpfile();
}
+ else if (bug_bytes_entered)
+ return;
+ else cmd_usage(pc->curcmd, SYNOPSIS);
- close_tmpfile();
FREEBUF(req->buf);
FREEBUF(req);
+ return;
}
/*
--
2.4.3
6:30 a.m.
New subject: [PATCH 2/2] dis: Introduce the -f option
The "dis" command offers a well known "-r" option, which allows
the user to display all instructions from the start of the routine
up to and including the designated address. Currently there is no option to
go in the opposite direction.
This patch introduces the "-f" (forward) option. As the option suggests, it
is to be used to displays all instructions from the given text address to
the end of the routine. The given address may be expressed by value, symbol
or expression. For example:
crash> dis -f do_fork+430
0xffffffff8013804b <do_fork+430>: callq 0xffffffff80143aac <ptrace_notify>
0xffffffff80138050 <do_fork+435>: jmp 0xffffffff8013805d <do_fork+448>
0xffffffff80138052 <do_fork+437>: mov %r13d,%edi
0xffffffff80138055 <do_fork+440>: mov %rbx,%r13
0xffffffff80138058 <do_fork+443>: callq 0xffffffff80149060 <free_pidmap>
0xffffffff8013805d <do_fork+448>: mov %r13,%rax
0xffffffff80138060 <do_fork+451>: add $0x38,%rsp
0xffffffff80138064 <do_fork+455>: pop %rbx
0xffffffff80138065 <do_fork+456>: pop %rbp
0xffffffff80138066 <do_fork+457>: pop %r12
0xffffffff80138068 <do_fork+459>: pop %r13
0xffffffff8013806a <do_fork+461>: pop %r14
0xffffffff8013806c <do_fork+463>: pop %r15
0xffffffff8013806e <do_fork+465>: retq
0xffffffff8013806f <do_fork+466>: nop
Signed-off-by: Aaron Tomlin <atomlin(a)redhat.com>
---
help.c | 2 ++
kernel.c | 44 ++++++++++++++++++++++++++++----------------
2 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/help.c b/help.c
index 2848ead..6f3965b 100644
--- a/help.c
+++ b/help.c
@@ -6152,6 +6152,8 @@ char *help_dis[] = {
" a text address that may be expressed by value, symbol or expression:\n",
" -r (reverse) displays all instructions from the start of the ",
" routine up to and including the designated address.",
+" -f (forward) displays all instructions from the given address ",
+" to the end of the routine.",
" -l displays source code line number data in addition to the ",
" disassembly output.",
" -u address is a user virtual address in the current context;",
diff --git a/kernel.c b/kernel.c
index 846cf71..77ca271 100644
--- a/kernel.c
+++ b/kernel.c
@@ -1427,11 +1427,11 @@ void
cmd_dis(void)
{
int c;
- int do_load_module_filter, do_machdep_filter, reverse;
+ int do_load_module_filter, do_machdep_filter, reverse, forward;
int unfiltered, user_mode, count_entered, bug_bytes_entered;
unsigned int radix;
ulong curaddr;
- ulong revtarget;
+ ulong target;
ulong count;
ulong offset;
struct syment *sp;
@@ -1453,17 +1453,18 @@ cmd_dis(void)
return;
}
- reverse = count_entered = bug_bytes_entered = FALSE;
+ reverse = forward = count_entered = bug_bytes_entered = FALSE;
sp = NULL;
unfiltered = user_mode = do_machdep_filter = do_load_module_filter = 0;
radix = 0;
+ target = 0;
req = (struct gnu_request *)GETBUF(sizeof(struct gnu_request));
req->buf = GETBUF(BUFSIZE);
req->flags |= GNU_FROM_TTY_OFF|GNU_RETURN_ON_ERROR;
req->count = 1;
- while ((c = getopt(argcnt, args, "dxhulrUb:B:")) != EOF) {
+ while ((c = getopt(argcnt, args, "dxhulrfUb:B:")) != EOF) {
switch(c)
{
case 'd':
@@ -1493,6 +1494,10 @@ cmd_dis(void)
reverse = TRUE;
break;
+ case 'f':
+ forward = TRUE;
+ break;
+
case 'l':
if (NO_LINE_NUMBERS())
error(INFO, "line numbers are not available\n");
@@ -1559,9 +1564,10 @@ cmd_dis(void)
}
if (args[++optind]) {
- if (reverse) {
+ if (reverse || forward) {
error(INFO,
- "count argument ignored with -r option\n");
+ "count argument ignored with -%s option\n",
+ reverse ? "r" : "f");
} else {
req->count = stol(args[optind],
FAULT_ON_ERROR, NULL);
@@ -1592,7 +1598,7 @@ cmd_dis(void)
req->command = GNU_RESOLVE_TEXT_ADDR;
gdb_interface(req);
req->flags &= ~GNU_COMMAND_FAILED;
- if (reverse || req->flags & GNU_FUNCTION_ONLY) {
+ if (reverse || forward || req->flags & GNU_FUNCTION_ONLY) {
if (sp) {
savename = sp->name;
if ((sp = next_symbol(NULL, sp)))
@@ -1610,12 +1616,11 @@ cmd_dis(void)
}
}
- if (reverse) {
- revtarget = req->addr;
- if ((sp = value_search(revtarget, NULL)) == NULL)
- error(FATAL, "cannot resolve address: %lx\n", revtarget);
+ if (reverse || forward) {
+ target = req->addr;
+ if ((sp = value_search(target, NULL)) == NULL)
+ error(FATAL, "cannot resolve address: %lx\n", target);
- sprintf(buf1, "0x%lx", revtarget);
req->addr = sp->value;
} else
count = 0;
@@ -1627,12 +1632,12 @@ cmd_dis(void)
if (reverse)
sprintf(buf5, "x/%ldi 0x%lx",
- (revtarget - req->addr) ? revtarget - req->addr : 1,
+ (target - req->addr) ? target - req->addr : 1,
req->addr);
else
sprintf(buf5, "x/%ldi 0x%lx",
count_entered && req->count ? req->count :
- req->flags & GNU_FUNCTION_ONLY ?
+ forward || req->flags & GNU_FUNCTION_ONLY ?
req->addr2 - req->addr : 1,
req->addr);
gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
@@ -1655,8 +1660,15 @@ cmd_dis(void)
extract_hex(buf2, &curaddr, ':', TRUE);
+ if (forward) {
+ if (curaddr != target)
+ continue;
+ else
+ forward = FALSE;
+ }
+
if (!reverse)
- if ((req->flags & GNU_FUNCTION_ONLY) &&
+ if (!count_entered &&
(curaddr >= req->addr2))
break;
@@ -1677,7 +1689,7 @@ cmd_dis(void)
print_verbatim(pc->saved_fp, buf2);
if (reverse) {
- if (STRNEQ(buf2, buf1)) {
+ if (curaddr == target) {
if (LASTCHAR(clean_line(buf2)) != ':')
break;
--
2.4.3
8:03 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
----- Original Message -----
Dave,
Since our last discussion [1], a further change to the "consolidation"
patch has been made. The assumption now is that the text address generated
by gdb is _always_ in base 16 form. Hence the if clause to check buf2 is
prefixed with 0x has been removed as indicated in the hunk below:
- if (STRNEQ(buf2, "0x"))
- extract_hex(buf2, &curaddr, ':', TRUE);
+ extract_hex(buf2, &curaddr, ':', TRUE);
Please let me know if this assumption is valid.
I don't think it is.
The reason for the "0x" check is that gdb may split a single line of
disassembly
into two lines, or at least it used to. As I recall, I think I later added a gdb
option initialization to prevent that from happening (?, but nonetheless, it could
still be undone by the user. So it's best to leave the check in place.
Dave
With optimisation level 2 (default) the following warning is displayed:
$ make warn
TARGET: X86_64
CRASH: 7.1.2
GDB: 7.6
cc -c -g -DX86_64 -DGDB_7_6 build_data.c -Wall -O2 -Wstrict-prototypes
-Wmissing-prototypes -fstack-protector -Wformat-security
cc -c -g -DX86_64 -DGDB_7_6 kernel.c -Wall -O2 -Wstrict-prototypes
-Wmissing-prototypes -fstack-protector -Wformat-security
kernel.c: In function ‘cmd_dis’:
kernel.c:1663:8: warning: ‘target’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
if (curaddr != target)
^
Note that this is not seen when optimisation is disabled. To work around
this (what I suspect is a false positive), where target is declared it is
now initialised to zero even though its value is not considered if forward
or reverse is not set.
That's not a problem -- there are many place where that's done to work around
that kind of warning.
Thanks,
Dave
[1]:
https://www.redhat.com/archives/crash-utility/2015-August/msg00025.html
Aaron Tomlin (2):
dis: Consolidate cmd_dis
dis: Introduce the -f option
help.c | 2 +
kernel.c | 287
+++++++++++++++++++++++----------------------------------------
2 files changed, 106 insertions(+), 183 deletions(-)
--
2.4.3
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
8:13 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
----- Original Message -----
----- Original Message -----
> Dave,
>
> Since our last discussion [1], a further change to the "consolidation"
> patch has been made. The assumption now is that the text address generated
> by gdb is _always_ in base 16 form. Hence the if clause to check buf2 is
> prefixed with 0x has been removed as indicated in the hunk below:
>
> - if (STRNEQ(buf2, "0x"))
> - extract_hex(buf2, &curaddr, ':', TRUE);
> + extract_hex(buf2, &curaddr, ':', TRUE);
>
> Please let me know if this assumption is valid.
I don't think it is.
The reason for the "0x" check is that gdb may split a single line of
disassembly
into two lines, or at least it used to. As I recall, I think I later added a gdb
option initialization to prevent that from happening (?, but nonetheless, it could
still be undone by the user. So it's best to leave the check in place.
BTW, that's what all the ":" checks are there for, both in cmd_dis() and in
the
per-arch dis_filter functions. When gdb would split the line, it would end the
first line with the colon following the text address and offset.
Dave
Dave
>
> With optimisation level 2 (default) the following warning is displayed:
>
> $ make warn
> TARGET: X86_64
> CRASH: 7.1.2
> GDB: 7.6
>
> cc -c -g -DX86_64 -DGDB_7_6 build_data.c -Wall -O2 -Wstrict-prototypes
> -Wmissing-prototypes -fstack-protector -Wformat-security
> cc -c -g -DX86_64 -DGDB_7_6 kernel.c -Wall -O2 -Wstrict-prototypes
> -Wmissing-prototypes -fstack-protector -Wformat-security
> kernel.c: In function ‘cmd_dis’:
> kernel.c:1663:8: warning: ‘target’ may be used uninitialized in this
> function
> [-Wmaybe-uninitialized]
> if (curaddr != target)
> ^
> Note that this is not seen when optimisation is disabled. To work around
> this (what I suspect is a false positive), where target is declared it is
> now initialised to zero even though its value is not considered if forward
> or reverse is not set.
>
That's not a problem -- there are many place where that's done to work around
that kind of warning.
Thanks,
Dave
> [1]:
> https://www.redhat.com/archives/crash-utility/2015-August/msg00025.html
>
> Aaron Tomlin (2):
> dis: Consolidate cmd_dis
> dis: Introduce the -f option
>
> help.c | 2 +
> kernel.c | 287
> +++++++++++++++++++++++----------------------------------------
> 2 files changed, 106 insertions(+), 183 deletions(-)
>
> --
> 2.4.3
>
> --
> Crash-utility mailing list
> Crash-utility(a)redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
8:41 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
On Mon 2015-08-10 09:13 -0400, Dave Anderson wrote:
----- Original Message -----
>
>
> ----- Original Message -----
> > Dave,
> >
> > Since our last discussion [1], a further change to the
"consolidation"
> > patch has been made. The assumption now is that the text address generated
> > by gdb is _always_ in base 16 form. Hence the if clause to check buf2 is
> > prefixed with 0x has been removed as indicated in the hunk below:
> >
> > - if (STRNEQ(buf2, "0x"))
> > - extract_hex(buf2, &curaddr, ':', TRUE);
> > + extract_hex(buf2, &curaddr, ':', TRUE);
> >
> > Please let me know if this assumption is valid.
>
> I don't think it is.
>
> The reason for the "0x" check is that gdb may split a single line of
disassembly
> into two lines, or at least it used to. As I recall, I think I later added a gdb
> option initialization to prevent that from happening (?, but nonetheless, it could
> still be undone by the user. So it's best to leave the check in place.
BTW, that's what all the ":" checks are there for, both in cmd_dis() and in
the
per-arch dis_filter functions. When gdb would split the line, it would end the
first line with the colon following the text address and offset.
OK shall I post a v2 to put the if clause back?
--
Aaron Tomlin
8:58 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
----- Original Message -----
On Mon 2015-08-10 09:13 -0400, Dave Anderson wrote:
>
>
> ----- Original Message -----
> >
> >
> > ----- Original Message -----
> > > Dave,
> > >
> > > Since our last discussion [1], a further change to the
"consolidation"
> > > patch has been made. The assumption now is that the text address
generated
> > > by gdb is _always_ in base 16 form. Hence the if clause to check buf2 is
> > > prefixed with 0x has been removed as indicated in the hunk below:
> > >
> > > - if (STRNEQ(buf2, "0x"))
> > > - extract_hex(buf2, &curaddr, ':', TRUE);
> > > + extract_hex(buf2, &curaddr, ':', TRUE);
> > >
> > > Please let me know if this assumption is valid.
> >
> > I don't think it is.
> >
> > The reason for the "0x" check is that gdb may split a single line of
disassembly
> > into two lines, or at least it used to. As I recall, I think I later added a
gdb
> > option initialization to prevent that from happening (?, but nonetheless, it
could
> > still be undone by the user. So it's best to leave the check in place.
>
> BTW, that's what all the ":" checks are there for, both in cmd_dis()
and in the
> per-arch dis_filter functions. When gdb would split the line, it would end the
> first line with the colon following the text address and offset.
OK shall I post a v2 to put the if clause back?
As long as your patch doesn't make other presumptions that the gdb output is
necessarily a one-liner, then I can add it back for testing.
Dave
9:26 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
On Mon 2015-08-10 09:58 -0400, Dave Anderson wrote:
[ ... ]
> > BTW, that's what all the ":" checks are there
for, both in cmd_dis() and in the
> > per-arch dis_filter functions. When gdb would split the line, it would end
the
> > first line with the colon following the text address and offset.
>
> OK shall I post a v2 to put the if clause back?
As long as your patch doesn't make other presumptions that the gdb output is
necessarily a one-liner, then I can add it back for testing.
That's the only one. I'll wait for your feedback.
Thanks,
--
Aaron Tomlin
10:27 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
----- Original Message -----
On Mon 2015-08-10 09:58 -0400, Dave Anderson wrote:
[ ... ]
> > > BTW, that's what all the ":" checks are there for, both in
cmd_dis() and in the
> > > per-arch dis_filter functions. When gdb would split the line, it would
end the
> > > first line with the colon following the text address and offset.
> >
> > OK shall I post a v2 to put the if clause back?
>
> As long as your patch doesn't make other presumptions that the gdb output is
> necessarily a one-liner, then I can add it back for testing.
That's the only one. I'll wait for your feedback.
Hi Aaron,
I haven't investigated why, but I immediately bumped into a bug
in your patch that needs to be addressed. It will not disassemble
a single instruction (i.e. "dis <address>") unless:
(1) it is the first instruction in a function -- in which case
disassembles the whole function as expected, or
(2) unless a count argument is applied.
Here's an example with crash-7.1.2:
crash> dis vfs_read+32
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash> dis 0xffffffff8119d3c0
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash>
With your patch applied:
crash> dis vfs_read+32
crash> dis 0xffffffff8119d3c0
crash>
Although it does work with a count argument:
crash> dis vfs_read+32 1
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash> dis 0xffffffff8119d3c0 1
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash>
Dave
10:47 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
----- Original Message -----
Hi Aaron,
I haven't investigated why, but I immediately bumped into a bug
in your patch that needs to be addressed. It will not disassemble
a single instruction (i.e. "dis <address>") unless:
The bug is caused by this part of the 2/2 patch:
@@ -1655,8 +1660,15 @@ cmd_dis(void)
extract_hex(buf2, &curaddr, ':', TRUE);
+ if (forward) {
+ if (curaddr != target)
+ continue;
+ else
+ forward = FALSE;
+ }
+
if (!reverse)
- if ((req->flags & GNU_FUNCTION_ONLY) &&
+ if (!count_entered &&
(curaddr >= req->addr2))
break;
req->addr2 is 0 in this case, so it always breaks/fails.
Dave
(1) it is the first instruction in a function -- in which case
disassembles the whole function as expected, or
(2) unless a count argument is applied.
Here's an example with crash-7.1.2:
crash> dis vfs_read+32
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash> dis 0xffffffff8119d3c0
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash>
With your patch applied:
crash> dis vfs_read+32
crash> dis 0xffffffff8119d3c0
crash>
Although it does work with a count argument:
crash> dis vfs_read+32 1
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash> dis 0xffffffff8119d3c0 1
0xffffffff8119d3c0 <vfs_read+32>: testb $0x1,0x44(%rdi)
crash>
Dave
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
5:03 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
On Tue 2015-08-11 11:47 -0400, Dave Anderson wrote:
[ ... ]
The bug is caused by this part of the 2/2 patch:
@@ -1655,8 +1660,15 @@ cmd_dis(void)
extract_hex(buf2, &curaddr, ':', TRUE);
+ if (forward) {
+ if (curaddr != target)
+ continue;
+ else
+ forward = FALSE;
+ }
+
if (!reverse)
- if ((req->flags & GNU_FUNCTION_ONLY) &&
+ if (!count_entered &&
(curaddr >= req->addr2))
break;
req->addr2 is 0 in this case, so it always breaks/fails.
Indeed; req->addr2 is only relevant in the case of: reverse, forward
or GNU_FUNCTION_ONLY. Where a single instruction is specified, we should
not evaluate here. As such the following change should resolve this bug:
--- a/kernel.c
+++ b/kernel.c
@@ -1669,7 +1669,7 @@ cmd_dis(void)
}
if (!reverse)
- if (!count_entered &&
+ if (!count_entered && req->addr2 &&
(curaddr >= req->addr2))
break;
--
Aaron Tomlin
8:55 a.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
----- Original Message -----
On Tue 2015-08-11 11:47 -0400, Dave Anderson wrote:
[ ... ]
> The bug is caused by this part of the 2/2 patch:
>
> @@ -1655,8 +1660,15 @@ cmd_dis(void)
>
> extract_hex(buf2, &curaddr, ':', TRUE);
>
> + if (forward) {
> + if (curaddr != target)
> + continue;
> + else
> + forward = FALSE;
> + }
> +
> if (!reverse)
> - if ((req->flags & GNU_FUNCTION_ONLY)
&&
> + if (!count_entered &&
> (curaddr >= req->addr2))
> break;
>
> req->addr2 is 0 in this case, so it always breaks/fails.
Indeed; req->addr2 is only relevant in the case of: reverse, forward
or GNU_FUNCTION_ONLY. Where a single instruction is specified, we should
not evaluate here. As such the following change should resolve this bug:
--- a/kernel.c
+++ b/kernel.c
@@ -1669,7 +1669,7 @@ cmd_dis(void)
}
if (!reverse)
- if (!count_entered &&
+ if (!count_entered && req->addr2 &&
(curaddr >= req->addr2))
break;
--
OK good. I'm going to run some sanity-checks on most of the other architectures.
I don't foresee any problems, and hopefully I should get this checked in today.
Thanks,
Dave
12:57 p.m.
New subject: [PATCH 0/2] dis: Introduce the -f option
----- Original Message -----
----- Original Message -----
> On Tue 2015-08-11 11:47 -0400, Dave Anderson wrote:
> [ ... ]
> > The bug is caused by this part of the 2/2 patch:
> >
> > @@ -1655,8 +1660,15 @@ cmd_dis(void)
> >
> > extract_hex(buf2, &curaddr, ':', TRUE);
> >
> > + if (forward) {
> > + if (curaddr != target)
> > + continue;
> > + else
> > + forward = FALSE;
> > + }
> > +
> > if (!reverse)
> > - if ((req->flags & GNU_FUNCTION_ONLY)
&&
> > + if (!count_entered &&
> > (curaddr >= req->addr2))
> > break;
> >
> > req->addr2 is 0 in this case, so it always breaks/fails.
>
> Indeed; req->addr2 is only relevant in the case of: reverse, forward
> or GNU_FUNCTION_ONLY. Where a single instruction is specified, we should
> not evaluate here. As such the following change should resolve this bug:
>
> --- a/kernel.c
> +++ b/kernel.c
> @@ -1669,7 +1669,7 @@ cmd_dis(void)
> }
>
> if (!reverse)
> - if (!count_entered &&
> + if (!count_entered && req->addr2
&&
> (curaddr >= req->addr2))
> break;
>
>
> --
OK good. I'm going to run some sanity-checks on most of the other architectures.
I don't foresee any problems, and hopefully I should get this checked in today.
Thanks,
Dave
Hi Aaron,
The testing went OK as expected, although I did make a couple minor additions.
In help.c, I added "f" to the SYNOPSIS line, and added an example to the help
page. And in kernel.c, I made the -f and -r options mutually exclusive.
Queued for crash-7.1.3:
https://github.com/crash-utility/crash/commit/4935c333a6cd1e7a8949eda5331...
Nice cleanup of cmd_dis() and a nifty addition to the toolbox!
Thanks,
Dave
3244
days inactive
3260
days old
devel@lists.crash-utility.osci.io
19 comments
2 participants
participants (2)
-
Aaron Tomlin -
Dave Anderson