Re: [PATCH v2] Add support for the --readnever command-line option (DWARF only)

Sergio Durigan Junior
2017-11-23T17:21:37Z

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 <brobecker@adacore.com>
>> 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 <cagney@redhat.com>
>> Joel Brobecker <brobecker@adacore.com>
>> Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * 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 <cagney@redhat.com>
>> Joel Brobecker <brobecker@adacore.com>
>>
>> * gdb.texinfo (File Options): Document --readnever.
>>
>> gdb/testsuite/ChangeLog:
>>
>> Joel Brobecker <brobecker@adacore.com>
>> Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * 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 <unordered_set>
>> #include <unordered_map>
>> #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 <http://www.gnu.org/licenses/>. */
>> +
>> +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 <http://www.gnu.org/licenses/>.
>> +
>> +# 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