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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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
http://sergiodj.net/