Re: [PATCH v3 1/2] Update gdb to 10.1

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



-----Original Message-----
> 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

sorry, this is wrong URL, the correct one is:
https://travis-ci.org/github/k-hagio/crash/builds/759421444

The former is the results with the 2/2 patch.

> 
> 
> - "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 ...

hmm, this looks gdb's behavior.  Maybe no need to match it with the current
output by force.

Thanks,
Kazu

> 
> 
> - "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@xxxxxxxxxx>
> > ---
> >  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
> >
> 
> 
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://listman.redhat.com/mailman/listinfo/crash-utility


--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux