From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96676 invoked by alias); 23 Nov 2017 17:21:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 96665 invoked by uid 89); 23 Nov 2017 17:21:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,KAM_STOCKGEN,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Nov 2017 17:21:34 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0603B624C9; Thu, 23 Nov 2017 17:21:33 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A0EC318245; Thu, 23 Nov 2017 17:21:32 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: Yao Qi , Joel Brobecker , "gdb-patches\@sourceware.org" Subject: Re: [PATCH v2] Add support for the --readnever command-line option (DWARF only) References: <1467838463-15786-1-git-send-email-brobecker@adacore.com> <87o9ntddb6.fsf_-_@redhat.com> <2cb6d01b-b40b-0a73-2df4-65f4e2094731@redhat.com> Date: Thu, 23 Nov 2017 17:21:00 -0000 In-Reply-To: <2cb6d01b-b40b-0a73-2df4-65f4e2094731@redhat.com> (Pedro Alves's message of "Thu, 23 Nov 2017 12:09:11 +0000") Message-ID: <87efoodi79.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg00564.txt.bz2 On Thursday, November 23 2017, Pedro Alves wrote: > On 11/23/2017 12:54 AM, Sergio Durigan Junior wrote: >> [ Reviving the thread. ] >> >> Hey there, >> >> So, since I'm working on upstreaming most of the patches we carry on >> Fedora GDB, this one caught my attention (thanks to Pedro for bringing >> this to me). >> >> I applied it to a local tree, did some tests and adjustments (see >> below), and now I'm resubmitting it for another set of reviews. >> >> I hope we can get it pushed this time :-). >> > > Thanks for doing this. > >> Please see comments below. > > See my comments inline below. > >> On Tuesday, October 04 2016, Pedro Alves wrote: > >>> This predates my gdb involvement, I don't really know the history. >>> Maybe Jan knows. >>> >>> In any case, I don't object to the approach. >>> >>> Is this skipping _unwind_ info as well though? I think the >>> documentation should be clear on that, because if it does >>> skip dwarf info for unwinding as well, then you >>> may get a faster, but incorrect backtrace. >> >> So, I looked a bit into this, and it seems that unwind information is >> also skipped, which is unfortunate. I am not an expert in this area of >> GDB so by all means please comment if you have more details, but here's >> the simple test I did. >> >> I modified gdb.dwarf2/dw2-dup-frame.exp to use --readnever, and ran it. >> The tests failed, and from what I checked this is because GDB was unable >> to tell that the frame stack is corrupted (because there are two >> identical frames in it). By doing some debugging, I noticed that >> gdb/dwarf2-frame.c:dwarf2_frame_sniffer is always returning 0 because it >> can't find any FDE's, which are found by using DWARF information that >> GDB hasn't read. > > I think we should document this. Yeah. I will write something about it in the documentation. WDYT of this? Due to the fact that @value{GDBN} uses DWARF information to perform frame unwinding, an unfortunate side effect of @code{--readnever} is to make backtrace information unreliable. ... after reading the rest of the e-mail... Ops, I see you already proposed a text below. I used your version, then. > [...] >>>> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { >>>> + untested "Couldn't compile ${srcfile}" >>>> + return -1 >>>> +} >>> >>> Maybe use build_executable. >> >> Done. >> >>>> +set saved_gdbflags $GDBFLAGS >>>> +set GDBFLAGS "$GDBFLAGS --readnever" >>>> +clean_restart ${binfile} >>>> +set GDBFLAGS $saved_gdbflags >>> >>> Nowadays we have save_vars: >>> >>> save_vars { GDBFLAGS } { >>> append GDBFLAGS " --readnever" >>> clean_restart ${binfile} >>> } >> >> Done. >> >> Here's the updated patch. I've made a few cosmetic modifications >> (s/RedHat/Red Hat/, for example) to the commit message, BTW. >> > > IMO, that commit message could/should be simplified further to > get it more to the point; there's text in there that is more > appropriate for a cover letter than for the commit log itself. > For example, the log of changes. Also, certainly we don't > expect "git log" readers to be able to answer RFC/questions > in git logs. :-) Sure thing. I was going to remove the parts about RFC (and also the part about the "customer request") later, but thanks for pointing it out. > > On 11/23/2017 12:54 AM, Sergio Durigan Junior wrote: >> From 858798fa4ce56e05c24e86098072518950135b4f Mon Sep 17 00:00:00 2001 >> From: Joel Brobecker >> Date: Wed, 6 Jul 2016 13:54:23 -0700 >> Subject: [PATCH] Add support for the --readnever command-line option (DWARF >> only) >> >> Hello, >> >> One of our customers asked us about this option, which they could see >> as being available in the version of GDB shipped by Red Hat but not in >> the version that AdaCore supports. >> >> The purpose of this option is to turn the load of debugging >> information off. The implementation proposed here is mostly a copy of >> the patch distributed with Fedora, and looking at the patch itself and >> the history, I can see some reasons why it was never submitted: >> >> - The patch appears to have been introduced as a workaround, at >> least initially; >> - The patch is far from perfect, as it simply shunts the load of >> DWARF debugging information, without really worrying about the >> other debug format. >> - Who really does non-symbolic debugging anyways? >> >> One use of this is when a user simply wants to do the following >> sequence: attach, dump core, detach. Loading the debugging information >> in this case is an unnecessary cause of delay. > > I think this sentence about the use case could migrate to > the documentation. Done. > BTW, this is missing a NEWS entry. Done. >> I started looking at a more general way of implementing this feature. >> For instance, I was hoping for a patch maybe at the objfile reader >> level, or even better at the generic part of the objfile reader. >> But it turns out this is not trivial at all. The loading of debugging >> information appears to be done as part of either the sym_fns->sym_read >> (most cases), or during the sym_fns->sym_read_psymbols phase ("lazy >> loading", ELF). Reading the code there, it wasn't obvious to me >> what the consequence would be to add some code to ignore debugging >> information, and since I would not be able to test those changes, >> I opted towards touching only the targets that use DWARF. >> >> This is why I ended up choosing the same approach as Red Hat. It's >> far from perfect, but has the benefit of working for what I hope is >> the vast majority of people, and being fairly unintrusive. I thought, >> since it's useful to AdaCore, and it's been useful to Red Hat, maybe >> others might find it useful, so here it is. >> >> The changes I made to the patch from Fedora are: >> >> - dwarf2_has_info: Return immediately if readnever_symbol_files >> is set - faster return, and easier to read, IMO; >> - main.c: Update the --help output to mention that this feature >> only supports DWARF; >> - gdb.texinfo: Add a paragraph to explain that this option only >> supports DWARF. >> >> If you guys don't think it's a good idea, then I'll understand >> (hence the RFC). At least we'll have it in the archives, in case >> someone else wants it. >> >> gdb/ChangeLog: >> >> Andrew Cagney >> Joel Brobecker >> Sergio Durigan Junior >> >> * symfile.c (readnever_symbol_files): New global. >> * top.h (readnever_symbol_files): New extern global. >> * main.c (captured_main): Add support for --readnever. >> (print_gdb_help): Document --readnever. >> * dwarf2read.c: #include "top.h". >> (dwarf2_has_info): Return 0 if READNEVER_SYMBOL_FILES is set. >> >> gdb/doc/ChangeLog: >> >> Andrew Cagney >> Joel Brobecker >> >> * gdb.texinfo (File Options): Document --readnever. >> >> gdb/testsuite/ChangeLog: >> >> Joel Brobecker >> Sergio Durigan Junior >> >> * gdb.base/readnever.c, gdb.base/readnever.exp: New files. >> >> Tested on x86_64-linux. >> --- >> gdb/doc/gdb.texinfo | 8 ++++++ >> gdb/dwarf2read.c | 4 +++ >> gdb/main.c | 32 +++++++++++++++++++++--- >> gdb/symfile.c | 1 + >> gdb/testsuite/gdb.base/readnever.c | 39 ++++++++++++++++++++++++++++++ >> gdb/testsuite/gdb.base/readnever.exp | 47 ++++++++++++++++++++++++++++++++++++ >> gdb/top.h | 1 + >> 7 files changed, 129 insertions(+), 3 deletions(-) >> create mode 100644 gdb/testsuite/gdb.base/readnever.c >> create mode 100644 gdb/testsuite/gdb.base/readnever.exp >> >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index ab05a3718d..7d3d651185 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -1037,6 +1037,14 @@ Read each symbol file's entire symbol table immediately, rather than >> the default, which is to read it incrementally as it is needed. >> This makes startup slower, but makes future operations faster. >> >> +@item --readnever >> +@cindex @code{--readnever} >> +Do not read each symbol file's symbolic debug information. This makes >> +startup faster but at the expense of not being able to perform >> +symbolic debugging. > > Here I think it'd be good to say something about unwind info, mention > that backtraces may be inaccurate, or something. For example, Fedora's > gstack used to use --readnever, but it no longer does; it relies > on .gdb_index for speed instead. > > The sentence about the "attach + core" use case in the commit log > could go here, since it's the main use case -- the point being > to help users answer Joel's question "but why would I want that; who > wants non-symbolic debugging anyway?". > > So all in all, I'd suggest: > > Do not read each symbol file's symbolic debug information. This makes > startup faster but at the expense of not being able to perform > symbolic debugging. DWARF unwind information is also not read, meaning > backtraces may become incomplete or inaccurate. > One use of this is when a user simply wants to do the following > sequence: attach, dump core, detach. Loading the debugging information > in this case is an unnecessary cause of delay. Cool, I used this version, thanks. >> + >> +This option is currently limited to debug information in DWARF format. >> +For all other format, this option has no effect. > > How hard would it be to just make it work? There's only stabs and mdebug > left, I think? There should be a single a function somewhere that we can > add an early return. And then we don't need to document this limitation... > > For example, in elf_symfile_read, we could just skip the elf_locate_sections > call. In coffread.c we could skip reading stabs right after > bfd_map_over_sections (abfd, coff_locate_sections....); > > Looking for: > > $ grep -h "^[a-z]*_build_psymtabs" gdb/ > coffstab_build_psymtabs (struct objfile *objfile, > elfstab_build_psymtabs (struct objfile *objfile, asection *stabsect, > stabsect_build_psymtabs (struct objfile *objfile, char *stab_name, > mdebug_build_psymtabs (minimal_symbol_reader &reader, > elfmdebug_build_psymtabs (struct objfile *objfile, > > finds all the relevant places. > > Maybe it wouldn't be that hard to make this be an objfile flag > afterall (like OBJF_READNOW is). That'd make it possible > to add the location "-readnever" counterpart switch to add-symbol-file > too, BTW: > > symfile.c: if (strcmp (arg, "-readnow") == 0) > symfile.c: else if (strcmp (arg, "-readnow") == 0) Hm, I'll look into this. Just to make it clear: the idea is to have both a --readnever global option and also a OBJF_READNEVER specific to each objfile? >> @end table >> >> @node Mode Options >> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c >> index 334d8c2e05..686fa10148 100644 >> --- a/gdb/dwarf2read.c >> +++ b/gdb/dwarf2read.c >> @@ -82,6 +82,7 @@ >> #include >> #include >> #include "selftest.h" >> +#include "top.h" >> >> /* When == 1, print basic high level tracing messages. >> When > 1, be more verbose. >> @@ -2319,6 +2320,9 @@ int >> dwarf2_has_info (struct objfile *objfile, >> const struct dwarf2_debug_sections *names) >> { >> + if (readnever_symbol_files) >> + return 0; >> + >> dwarf2_per_objfile = ((struct dwarf2_per_objfile *) >> objfile_data (objfile, dwarf2_objfile_data_key)); >> if (!dwarf2_per_objfile) >> diff --git a/gdb/main.c b/gdb/main.c >> index 61168faf50..3ca64f48ef 100644 >> --- a/gdb/main.c >> +++ b/gdb/main.c >> @@ -579,14 +579,17 @@ captured_main_1 (struct captured_main_args *context) >> OPT_NOWINDOWS, >> OPT_WINDOWS, >> OPT_IX, >> - OPT_IEX >> + OPT_IEX, >> + OPT_READNOW, >> + OPT_READNEVER >> }; >> static struct option long_options[] = >> { >> {"tui", no_argument, 0, OPT_TUI}, >> {"dbx", no_argument, &dbx_commands, 1}, >> - {"readnow", no_argument, &readnow_symbol_files, 1}, >> - {"r", no_argument, &readnow_symbol_files, 1}, >> + {"readnow", no_argument, NULL, OPT_READNOW}, >> + {"readnever", no_argument, NULL, OPT_READNEVER}, >> + {"r", no_argument, NULL, OPT_READNOW}, >> {"quiet", no_argument, &quiet, 1}, >> {"q", no_argument, &quiet, 1}, >> {"silent", no_argument, &quiet, 1}, >> @@ -809,6 +812,26 @@ captured_main_1 (struct captured_main_args *context) >> } >> break; >> >> + case OPT_READNOW: >> + { >> + if (readnever_symbol_files) >> + error (_("%s: '--readnow' and '--readnever' cannot be " >> + "specified simultaneously"), >> + gdb_program_name); >> + readnow_symbol_files = 1; >> + } >> + break; >> + >> + case OPT_READNEVER: >> + { >> + if (readnow_symbol_files) >> + error (_("%s: '--readnow' and '--readnever' cannot be " >> + "specified simultaneously"), >> + gdb_program_name); >> + readnever_symbol_files = 1; > > maybe move the error call to a shared function, like > > static void > validate_readnow_readnever () > { > if (readnever_symbol_files && readnow_symbol_files) > { > error (_("%s: '--readnow' and '--readnever' cannot be " > "specified simultaneously"), > gdb_program_name); > } > } > > and then: > > case OPT_READNOW: > readnow_symbol_files = 1; > validate_readnow_readnever (); > break; > case OPT_READNEVER: > readnever_symbol_files = 1; > validate_readnow_readnever (); > break; I had a previous version of the patch that did a similar thing ;-). Anyway, consider it done. > >> diff --git a/gdb/testsuite/gdb.base/readnever.c b/gdb/testsuite/gdb.base/readnever.c >> new file mode 100644 >> index 0000000000..803a5542f9 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.base/readnever.c >> @@ -0,0 +1,39 @@ >> +/* Copyright 2016 Free Software Foundation, Inc. > > 2016-2017 Fixed. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +void >> +fun_three (void) >> +{ >> + /* Do nothing. */ >> +} >> + >> +void >> +fun_two (void) >> +{ >> + fun_three (); >> +} >> + >> +void >> +fun_one (void) >> +{ >> + fun_two (); >> +} >> + >> +int >> +main (void) >> +{ >> + fun_one (); >> + return 0; >> +} >> diff --git a/gdb/testsuite/gdb.base/readnever.exp b/gdb/testsuite/gdb.base/readnever.exp >> new file mode 100644 >> index 0000000000..24220f85c6 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.base/readnever.exp >> @@ -0,0 +1,47 @@ >> +# Copyright 2016 Free Software Foundation, Inc. > > Ditto. Done. >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see . >> + >> +# This file is part of the gdb testsuite. It is intended to test that >> +# gdb can correctly print arrays with indexes for each element of the >> +# array. > > No it isn't. Removed. >> + >> +standard_testfile .c >> + >> +if { [build_executable "failed to build" $testfile $srcfile { debug }] == -1 } { >> + untested "Couldn't compile ${srcfile}" >> + return -1 >> +} >> + >> +save_vars { GDBFLAGS } { >> + append GDBFLAGS " --readnever" >> + clean_restart ${binfile} >> +} > > I wonder, can we add tests ensuring that --readnever --readnow > errors out? I think you can use gdb_test_multiple, expect the > error output, and then expect eof {}, meaning GDB exited. Not > sure we have any testcase that tests invalid command line > options... (we should!) I'll give it a try. >> + >> +if ![runto_main] then { >> + perror "couldn't run to breakpoint" >> + continue >> +} >> + >> +gdb_test "break fun_three" \ >> + "Breakpoint $decimal at $hex" >> + >> +gdb_test "continue" \ >> + "Breakpoint $decimal, $hex in fun_three \\(\\)" >> + >> +gdb_test "backtrace" \ >> + [multi_line "#0 $hex in fun_three \\(\\)" \ >> + "#1 $hex in fun_two \\(\\)" \ >> + "#2 $hex in fun_one \\(\\)" \ >> + "#3 $hex in main \\(\\)" ] > > It doesn't look like this testcase is actually testing > that --readnever actually worked as intended? If GDB loads > debug info, then GDB shows the arguments. Otherwise it > shows "()" like above. But it also shows "()" if the function > is "(void)". How about adding some non-void parameters to > the functions? Hm, I guess you're right. It didn't catch my attention at first, but now it seems strange that we're testing things with functions that don't receive arguments. I'll improve it. > Could also try printing some local variable and expect > an error. > > And also: > > gdb_test_no_output "maint info symtabs" > gdb_test_no_output "maint info psymtabs" Will do. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/