Hi Alexey,
sorry for the late reply, I needed to learn the crash-gdb interaction and
how to review such a huge patch as this. We appreciate your patience and
understanding.
I've almost read the patch, on the whole, it is absolutely excellent!
I think it's going a right way.
There are several comments including slight things:
- Is it possible to separate the fixes only in crash (outside gdb-10.1.patch)
and old patches removal from this 1/2 patch? i.e.
- fix for "'bt' command often emits reduced output"
- fix for "lack of information about struct members" and
"unionprint"
- removal of gdb-7.6.patch and gdb-7.6-proc_service.h.patch
This would help us understand what issue a bunch of changes fixes, and
we can read it easier. I know it's better not to split the gdb-10.1.patch.
- Build error on architectures except for x86_64:
/usr/bin/ld: ../../crashlib.a(gdb_interface.o): in function `crash_get_nr_cpus':
/home/travis/build/k-hagio/crash/gdb_interface.c:1074: undefined reference to
`sadump_get_nr_cpus'
/usr/bin/ld: /home/travis/build/k-hagio/crash/gdb_interface.c:1076: undefined reference to
`diskdump_get_nr_cpus'
/usr/bin/ld: /home/travis/build/k-hagio/crash/gdb_interface.c:1078: undefined reference to
`kdump_get_nr_cpus'
collect2: error: ld returned 1 exit status
make[4]: *** [Makefile:1872: gdb] Error 1
make[3]: *** [Makefile:10072: all-gdb] Error 2
make[2]: *** [Makefile:860: all] Error 2
crash build failed
make[1]: *** [Makefile:239: gdb_merge] Error 1
make: *** [Makefile:314: warn] Error 2
The command "make warn" exited with 2.
ref.
https://travis-ci.org/github/k-hagio/crash/builds/759444845
- "make target=x86" on an x86_64 machine also fails with additional errors:
$ make target=x86
...
ar: creating crashlib.a
CXXLD gdb
/usr/bin/ld: skipping incompatible ./../zlib/libz.a when searching for -lz
/usr/bin/ld: skipping incompatible ./../zlib/libz.a when searching for -lz
/usr/bin/ld: i386 architecture of input file `../../crashlib.a(main.o)' is
incompatible with i386:x86-64 output
/usr/bin/ld: i386 architecture of input file `../../crashlib.a(tools.o)' is
incompatible with i386:x86-64 output
/usr/bin/ld: i386 architecture of input file `../../crashlib.a(global_data.o)' is
incompatible with i386:x86-64 output
...
../../crashlib.a(tools.o): In function `eval_common':
/home/k-hagio/crash/x86-gdb10/tools.c:3012: undefined reference to `__udivdi3'
/home/k-hagio/crash/x86-gdb10/tools.c:3015: undefined reference to `__umoddi3'
...
decNumber.c:(.text+0x79a7): undefined reference to `__udivdi3'
collect2: error: ld returned 1 exit status
make[3]: *** [gdb] Error 1
make[2]: *** [rebuild] Error 2
make[1]: *** [gdb_merge] Error 2
make: *** [all] Error 2
- "p" does not print the address of "linux_banner" for some vmcores
(relatively old kernels? like RHEL7).
--- crash-master.log
+++ crash-gdb10.1.log
crash> p linux_banner
-linux_banner = $1 = 0xffffffff816bc100 <linux_banner> "Linux version ...
+linux_banner = $1 = "Linux version ...
- "whatis" options print reduced/duplicated results:
crash> whatis -r 512
SIZE TYPE
- 512 _legacy_mbr <<-- dropped
512 i387_fxsave_struct
512 netns_ipv4
- 512 sgi_disklabel
512 user_i387_struct
crash> whatis -m mm_struct
SIZE TYPE
16 tlb_state
- 24 flush_tlb_info <<-- dropped
- 24 ftrace_raw_xen_mmu_pgd
24 futex_key
...
216 vm_area_struct
256 linux_binprm
2752 rq
+2752 rq <<-- duplicated
+2752 rq
+2752 rq
+2752 rq
4048 task_struct
-8296 numa_maps_private
+--- gdb-10.1/gdb/main.c.orig
++++ gdb-10.1/gdb/main.c
+@@ -929,8 +944,12 @@ captured_main_1 (struct captured_main_args
+ catch_command_errors returns non-zero on success! */
+ if (catch_command_errors (exec_file_attach, execarg,
+ !batch_flag, RETURN_MASK_ALL))
++#ifdef CRASH_MERGE
++ catch_command_errors (symbol_file_add_main, symarg, 0, RETURN_MASK_ALL);
++#else
+ catch_command_errors (symbol_file_add_main, symarg,
+ !batch_flag, RETURN_MASK_ALL);
++#endif
+ }
+ else
+ {
- This is a dropped hunk, but without this, "crash -s" also prints the
following message:
# crash -s
Reading symbols from /usr/lib/debug/lib/modules/3.10.0-1127.el7.x86_64/vmlinux...
crash>
I'm ok with this message without the "-s" option, but it would be
preferable
to print nothing with the option if there is no warning.
+@@ -992,8 +1011,12 @@ captured_main (void *data)
+ {
+ auto_load_local_gdbinit_loaded = 1;
+
++#ifdef CRASH_MERGE
++ catch_command_errors (source_script, local_gdbinit, -1, RETURN_MASK_ALL);
++#else
+ catch_command_errors (source_script, local_gdbinit, 0,
+ RETURN_MASK_ALL);
++#endif
+ }
+ }
+
+@@ -1039,6 +1062,12 @@ captured_main (void *data)
+ while (1)
+ {
+ catch_errors (captured_command_loop, 0, "", RETURN_MASK_ALL);
++#ifdef CRASH_MERGE
++ {
++ int console(char *, ...);
++ console("<CAPTURED_MAIN WHILE LOOP>\n");
++ }
++#endif
+ }
+ /* No exit -- exit is through quit_command. */
+ }
- Why were the two hunks dropped? Is it possible not to drop?
++static void
++gdb_delete_symbol_file(struct gnu_request *req)
++{
++ for (objfile *objfile : current_program_space->objfiles ()) {
++ if (STREQ(objfile_name(objfile), req->name) ||
++ same_file((char *)objfile_name(objfile), req->name)) {
++ break;
++ }
++ }
- This does not delete the symbol file, so the symbols remain even after
"mod -d" command.
++static void
++dump_enum(struct type *type, struct gnu_request *req)
++{
++ register int i;
++ int len;
++ int lastval;
- The "lastval" variable should be "long long"?
#define DUMP_EMPTY_FILE 0x8
#define DUMP_FILE_NRPAGES 0x10
-#endif /* !GDB_COMMON */
int same_file(char *, char *);
+#endif /* !GDB_COMMON */
#ifndef GDB_COMMON
int cleanup_memory_driver(void);
- We can remove this #endif and #ifndef?
++#ifdef CRASH_MERGE
++extern "C" int gdb_main_entry(int, char **);
++extern void replace_ui_file_FILE(struct ui_file *, FILE *);
- We don't have the replace_ui_file_FILE() any more?
+int crash_get_nr_cpus(void)
+{
+ if (SADUMP_DUMPFILE())
+ return sadump_get_nr_cpus();
+ else if (DISKDUMP_DUMPFILE())
+ return diskdump_get_nr_cpus();
+ else if (KDUMP_DUMPFILE())
+ return kdump_get_nr_cpus();
+ else if (VMSS_DUMPFILE())
+ return vmware_vmss_get_nr_cpus();
- Seems diskdump_get_nr_cpus() and kdump_get_nr_cpus() works only with
QEMU memory dumps and return 0 for normal vmcores. This causes crash
to fail with the 2/2 patch?
- The gdb-10.1.patch does not have a shell script at the head of it, once
it's modified, "make" prints "gdb-10.1.patch: line 2: ---: command not
found"
and so on.
$ make warn
TARGET: X86_64
CRASH: 7.2.9++
GDB: 10.1
+ diff -aurp -X diff_exclude gdb-10.1.orig/gdb/cli/cli-cmds.c gdb-10.1/gdb/cli/cli-cmds.c
diff: diff_exclude: No such file or directory
+ --- gdb-10.1.orig/gdb/cli/cli-cmds.c 2020-10-23 21:23:02.000000000 -0700
gdb-10.1.patch: line 2: ---: command not found
+ +++ gdb-10.1/gdb/cli/cli-cmds.c 2020-11-10 13:06:56.423569114 -0800
gdb-10.1.patch: line 3: +++: command not found
gdb-10.1.patch: line 4: syntax error near unexpected token `('
gdb-10.1.patch: line 4: `@@ -435,6 +435,11 @@ complete_command (const char *arg, int
f'
patching file gdb-10.1/gdb/cli/cli-cmds.c
Reversed (or previously applied) patch detected! Skipping patch.
4 out of 4 hunks ignored
...
- Compilation warnings.
symtab.c: In function ‘void gdb_get_line_number(gnu_request*)’:
symtab.c:7073:17: warning: variable ‘sym’ set but not used [-Wunused-but-set-variable]
struct symbol *sym;
^
CXX gcore.o
symtab.c: In function ‘void gdb_get_datatype(gnu_request*)’:
symtab.c:7137:51: warning: deprecated conversion from string constant to ‘char*’
[-Wwrite-strings]
console("gdb_get_datatype [%s] (a)\n", req->name);
^
CXX gdb-demangle.o
symtab.c:7163:51: warning: deprecated conversion from string constant to ‘char*’
[-Wwrite-strings]
console("gdb_get_datatype [%s] (b)\n", req->name);
^
symtab.c:7172:57: warning: deprecated conversion from string constant to ‘char*’
[-Wwrite-strings]
console("expr->elts[0].opcode: OP_VAR_VALUE\n");
^
CXX gdb_bfd.o
symtab.c:7191:52: warning: deprecated conversion from string constant to ‘char*’
[-Wwrite-strings]
console("expr->elts[0].opcode: OP_TYPE\n");
^
symtab.c:7224:31: warning: deprecated conversion from string constant to ‘char*’
[-Wwrite-strings]
expr.get()->elts[0].opcode);
^
symtab.c: In function ‘void eval_enum(type*, gnu_request*)’:
symtab.c:7285:17: warning: deprecated conversion from string constant to ‘char*’
[-Wwrite-strings]
req->tagname = "(unknown)";
^
symtab.c: In function ‘void get_member_data(gnu_request*, type*, long int, int)’:
symtab.c:7315:42: warning: deprecated conversion from string constant to ‘char*’
[-Wwrite-strings]
req->name, req->member, type, newtype);
^
symtab.c: In function ‘void gdb_command_exists(gnu_request*)’:
symtab.c:7355:43: warning: variable ‘c’ set but not used [-Wunused-but-set-variable]
register struct cmd_list_element *c;
Thanks,
Kazu
-----Original Message-----
Fully redone gdb-7.6.patch to gdb-10.1.patch to keep all
functionality. Changes which were dropped are saved in
dropped-gdb-7.6-to-10.1.patch
Main difference between gdb-7.6 and gdb-10.1 is the last
one was rewritten in C++.
I continue to keep crash code in C. Mark transition
functions as extern "C" to resolve linking issues.
Eliminated error_hook() and SJLJ while running in C++ code
(after gdb_command_funnel()) use try-catch mechanism instead.
request_types() was redone to do not call
GNU_GET_NEXT_DATATYPE multiple times but single usage of
GNU_ITERATE_DATATYPES with proper callback instead.
Complete iteration happens on C++ side now.
Removed "struct global_iterator" from request structure,
but added several fields (including callback pointer) to
be able to perform iteration on C++ side.
Type of "linux_banner" symbol is reported as 'D' by new
gdb as its section ".rodata" marked as writable in vmlinux.
BFD API has changed.
deprecated_command_loop_hook got deprecated. So, call crash
main_loop() directly from gdb captured_main().
Added symbol file (vmlinux) rebase in gdb by kaslr_offset.
by using new function: objfile_rebase().
As result, we do not need kernel symbol patching as well as
bait_and_switch hook anymore.
Added crash_target for gdb to provide target operations
such as xfer_partial to read and write crash dump memory.
Removed previously used hooks for that in target.c.
Keep crash_target.c as a file in crash folder instead of
in gdb-10.1.patch for easier development and history
tracking.
crash_target can be enhanced in future to provide access
to CPU registers, so backtrace and frame related commands
from gdb can be used.
Removed gdb-7.6-proc_service.h.patch is not required as
gdb-10.1 already has this change.
Extra: add VMware copyright to the version info.
TODO:
1) gdb-10.1-ppc64le-support.patch has to be updated with
following commits.
2) deprecate #if defined(GDB_X_Y) code as crash really
supports only the latest gdb (only one patch).
3) move gdb_funnel_command() and subfunctions to separate
file, similar to crash_target.c
4) remove legacy kernel patching and bait_and_switch hook.
Signed-off-by: Alexey Makhalov <amakhalov(a)vmware.com>
---
Makefile | 11 +-
configure.c | 20 +-
crash_target.c | 104 +
defs.h | 35 +-
dropped-gdb-7.6-to-10.1.patch | 303 +++
...support.patch => gdb-10.1-ppc64le-support.patch | 0
gdb-10.1.patch | 1577 ++++++++++++
gdb-7.6-proc_service.h.patch | 67 -
gdb-7.6.patch | 2503 --------------------
gdb_interface.c | 85 +-
help.c | 1 +
kernel.c | 2 +-
main.c | 1 -
symbols.c | 125 +-
x86_64.c | 14 +-
15 files changed, 2141 insertions(+), 2707 deletions(-)
create mode 100644 crash_target.c
create mode 100644 dropped-gdb-7.6-to-10.1.patch
rename gdb-7.6-ppc64le-support.patch => gdb-10.1-ppc64le-support.patch (100%)
create mode 100644 gdb-10.1.patch
delete mode 100644 gdb-7.6-proc_service.h.patch
delete mode 100644 gdb-7.6.patch