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

  [ 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 :-).

Please see comments below.

On Tuesday, October 04 2016, Pedro Alves wrote:

> On 07/12/2016 03:27 PM, Yao Qi wrote:
>> Hi Joel,
>> 
>> On Wed, Jul 6, 2016 at 9:54 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>>> Hello,
>>>
>>> One of our customers asked us about this option, which they could
>>> see as being available in the version of GDB shipped by RedHat 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 RedHat Fedora, and looking at the patch itself and
>>> the history, I can see some reasons why it was never submitted:
>> 
>> Andrew Cagney posted the patch
>> https://www.sourceware.org/ml/gdb/2004-11/msg00216.html
>> 
>>>   - 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?
>> 
>> The reason, IMO, it was posted is that people want GDB avoid reading
>> debug info and efficiently dump stack backtrace.  I think Red Hat people
>> must know why Fedora is shipping this patch.
>> 
>> I don't object to this approach.
>> 
>
> 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.

On Tuesday, October 04 2016, Pedro Alves wrote:

> On 07/06/2016 09:54 PM, Joel Brobecker wrote:
>
>>  #include <fcntl.h>
>>  #include <sys/types.h>
>> @@ -2062,6 +2063,9 @@ int
>>  dwarf2_has_info (struct objfile *objfile,
>>                   const struct dwarf2_debug_sections *names)
>>  {
>> +  if (readnever_symbol_files)
>> +    return 0;
>
> Guess that means '--readnever --readnow' is the same as
> --readnever in practice?

I've modified our main.c to detect whether --readnow and --readnever are
given at the same time, and print an error in this case.

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

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.

> 
> On Tuesday, October 04 2016, Pedro Alves wrote:
> 
>> On 07/06/2016 09:54 PM, Joel Brobecker wrote:
>>
>>>  #include <fcntl.h>
>>>  #include <sys/types.h>
>>> @@ -2062,6 +2063,9 @@ int
>>>  dwarf2_has_info (struct objfile *objfile,
>>>                   const struct dwarf2_debug_sections *names)
>>>  {
>>> +  if (readnever_symbol_files)
>>> +    return 0;
>>
>> Guess that means '--readnever --readnow' is the same as
>> --readnever in practice?
> 
> I've modified our main.c to detect whether --readnow and --readnever are
> given at the same time, and print an error in this case.

Sounds fine to me.

> 
>>
>>> +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.  :-)

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.

BTW, this is missing a NEWS entry.

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


> +
> +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)

>  @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;


> 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

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

> +# 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.

> +
> +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!)

> +
> +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?  

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"

Thanks,
Pedro Alves
  

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Yao Qi <qiyaoltc@gmail.com>,  Joel Brobecker <brobecker@adacore.com>,  "gdb-patches\@sourceware.org" <gdb-patches@sourceware.org>
> Date: Wed, 22 Nov 2017 19:54:53 -0500
> 
>   - 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.

This use case should be mentioned in the manual.  And I think if we
want to accept a patch that is DWARF specific, the name of the option
should reflect that; --readnever sounds misleading to me.

(Another possibility would be to have a "maint dwarf" command to do
the same; maybe it's better.)

> 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.
> +
> +This option is currently limited to debug information in DWARF format.
> +For all other format, this option has no effect.
   ^^^^^^^^^^^^^^^^^^^^
"For the others formats"

And I think we need a NEWS entry for this new feature.

Thanks.
  

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,
  

On 11/23/2017 05:21 PM, Sergio Durigan Junior wrote:

>>> +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:

I meant "logical" instead of "location".  I was staring at
gdb/location.c at that time.  :-P

>>
>>  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?

Sure, the idea is to do something similar to what's done for --readnow.

Thanks,
Pedro Alves
  

On Thursday, November 23 2017, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Yao Qi <qiyaoltc@gmail.com>,  Joel Brobecker <brobecker@adacore.com>,  "gdb-patches\@sourceware.org" <gdb-patches@sourceware.org>
>> Date: Wed, 22 Nov 2017 19:54:53 -0500
>> 
>>   - 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.
>
> This use case should be mentioned in the manual.  And I think if we
> want to accept a patch that is DWARF specific, the name of the option
> should reflect that; --readnever sounds misleading to me.
>
> (Another possibility would be to have a "maint dwarf" command to do
> the same; maybe it's better.)

Thanks for the review, Eli.

According to Pedro's comments, I am working on the patch to make the
feature available to other backends as well, so parts of the text will
be changed.

As for the use case, I've included it in the manual, as per Pedro's
suggestion.

>> 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.
>> +
>> +This option is currently limited to debug information in DWARF format.
>> +For all other format, this option has no effect.
>    ^^^^^^^^^^^^^^^^^^^^
> "For the others formats"

Thanks; this part will most likely change due to the extension of the
feature to the other backends.

> And I think we need a NEWS entry for this new feature.

Already added it.

Thanks,
  

On Thursday, November 23 2017, Pedro Alves wrote:

> On 11/23/2017 05:21 PM, Sergio Durigan Junior wrote:
>
>>>> +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:
>
> I meant "logical" instead of "location".  I was staring at
> gdb/location.c at that time.  :-P
>
>>>
>>>  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?
>
> Sure, the idea is to do something similar to what's done for --readnow.

Sorry, but I guess I need a few more details on this.

The way I understand the code at elf_symfile_read, the very first thing
to do would be to check if OBJF_READNEVER is set and return early if it
is.  But it seems that you're proposing something a bit different when
you say that we should "... just skip the elf_locate_sections call."  It
doesn't seem to me that is worth continuing on that function if
OBJF_READNEVER is present.

As for the *_build_psymtabs functions, I am doing exactly that: if
objfile->flags contains OBJF_READNEVER, then just return and do nothing.

The patch is almost ready for resubmission (well, I still need to figure
out how to test the --readnow && --readnever scenario), but I want to
make sure I got this part right.

Thanks,
  

On 11/24/2017 04:54 AM, Sergio Durigan Junior wrote:
> On Thursday, November 23 2017, Pedro Alves wrote:
> 
>> On 11/23/2017 05:21 PM, Sergio Durigan Junior wrote:
>>
>>>>> +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:
>>
>> I meant "logical" instead of "location".  I was staring at
>> gdb/location.c at that time.  :-P
>>
>>>>
>>>>  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?
>>
>> Sure, the idea is to do something similar to what's done for --readnow.
> 
> Sorry, but I guess I need a few more details on this.
> 
> The way I understand the code at elf_symfile_read, the very first thing
> to do would be to check if OBJF_READNEVER is set and return early if it
> is.  But it seems that you're proposing something a bit different when
> you say that we should "... just skip the elf_locate_sections call."  It
> doesn't seem to me that is worth continuing on that function if
> OBJF_READNEVER is present.

No, you can't return early the very first thing, because
 --readnever is supposed to skip _debug_ info, not ELF/minimal symbols...
So the "return early" would have to be _after_ the
elf_read_minimal_symbols call:

 static void
 elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 {
   bfd *abfd = objfile->obfd;
   struct elfinfo ei;

   memset ((char *) &ei, 0, sizeof (ei));
   bfd_map_over_sections (abfd, elf_locate_sections, (void *) & ei);
 
   elf_read_minimal_symbols (objfile, symfile_flags, &ei);

I don't know whether we can reorder that.  Maybe we can.

When I looked at this quickly yesterday, I saw that elf_location_sections
is what finds the stabs and mdebug sections:

static void
elf_locate_sections (bfd *ignore_abfd, asection *sectp, void *eip)
{
  struct elfinfo *ei;

  ei = (struct elfinfo *) eip;
  if (strcmp (sectp->name, ".stab") == 0)
    {
      ei->stabsect = sectp;
    }
  else if (strcmp (sectp->name, ".mdebug") == 0)
    {
      ei->mdebugsect = sectp;
    }
}

and it seemed to be that skipping the section location would make
the parts of  elf_symfile_read that actually read the symbols
be no-ops, because the stabsect/mdebusect pointers would be NULL.

But if returning early or something else works, that's fine.

> 
> As for the *_build_psymtabs functions, I am doing exactly that: if
> objfile->flags contains OBJF_READNEVER, then just return and do nothing.

Sure, that should work too.  It's just the difference between
skipping checking whether debug info is available (skipping before
calling into those), vs letting gdb do the work to figure out whether
debug info is available, but then ignore it.
The grep for "*_build_psymtabs" was intended as a pointer to find
what the relevant code is, including to look at the code that
is calling those functions, see if there's something to be done there.

> 
> The patch is almost ready for resubmission (well, I still need to figure
> out how to test the --readnow && --readnever scenario), but I want to
> make sure I got this part right.
> 

Thanks,
Pedro Alves
  

On Friday, November 24 2017, Pedro Alves wrote:

> On 11/24/2017 04:54 AM, Sergio Durigan Junior wrote:
>> On Thursday, November 23 2017, Pedro Alves wrote:
>> 
>>> On 11/23/2017 05:21 PM, Sergio Durigan Junior wrote:
>>>
>>>>>> +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:
>>>
>>> I meant "logical" instead of "location".  I was staring at
>>> gdb/location.c at that time.  :-P
>>>
>>>>>
>>>>>  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?
>>>
>>> Sure, the idea is to do something similar to what's done for --readnow.
>> 
>> Sorry, but I guess I need a few more details on this.
>> 
>> The way I understand the code at elf_symfile_read, the very first thing
>> to do would be to check if OBJF_READNEVER is set and return early if it
>> is.  But it seems that you're proposing something a bit different when
>> you say that we should "... just skip the elf_locate_sections call."  It
>> doesn't seem to me that is worth continuing on that function if
>> OBJF_READNEVER is present.
>
> No, you can't return early the very first thing, because
>  --readnever is supposed to skip _debug_ info, not ELF/minimal symbols...
> So the "return early" would have to be _after_ the
> elf_read_minimal_symbols call:

Hm, OK, it makes sense and I confess I thought "why is Pedro mentioning
elf_symfile_read if the feature is about skipping DWARF, not ELF?".

>  static void
>  elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>  {
>    bfd *abfd = objfile->obfd;
>    struct elfinfo ei;
>
>    memset ((char *) &ei, 0, sizeof (ei));
>    bfd_map_over_sections (abfd, elf_locate_sections, (void *) & ei);
>  
>    elf_read_minimal_symbols (objfile, symfile_flags, &ei);
>
> I don't know whether we can reorder that.  Maybe we can.
>
> When I looked at this quickly yesterday, I saw that elf_location_sections
> is what finds the stabs and mdebug sections:
>
> static void
> elf_locate_sections (bfd *ignore_abfd, asection *sectp, void *eip)
> {
>   struct elfinfo *ei;
>
>   ei = (struct elfinfo *) eip;
>   if (strcmp (sectp->name, ".stab") == 0)
>     {
>       ei->stabsect = sectp;
>     }
>   else if (strcmp (sectp->name, ".mdebug") == 0)
>     {
>       ei->mdebugsect = sectp;
>     }
> }
>
> and it seemed to be that skipping the section location would make
> the parts of  elf_symfile_read that actually read the symbols
> be no-ops, because the stabsect/mdebusect pointers would be NULL.
>
> But if returning early or something else works, that's fine.

OK, thanks for clarifying.

>> As for the *_build_psymtabs functions, I am doing exactly that: if
>> objfile->flags contains OBJF_READNEVER, then just return and do nothing.
>
> Sure, that should work too.  It's just the difference between
> skipping checking whether debug info is available (skipping before
> calling into those), vs letting gdb do the work to figure out whether
> debug info is available, but then ignore it.
> The grep for "*_build_psymtabs" was intended as a pointer to find
> what the relevant code is, including to look at the code that
> is calling those functions, see if there's something to be done there.

I think it makes more sense, logically speaking, to not mess with
elf_symfile_read and instead modify the *_build_psymtabs functions.

Thanks,
  

On 11/24/2017 08:26 PM, Sergio Durigan Junior wrote:
> On Friday, November 24 2017, Pedro Alves wrote:
> 
>> On 11/24/2017 04:54 AM, Sergio Durigan Junior wrote:
>>> On Thursday, November 23 2017, Pedro Alves wrote:
>>>
>>>> On 11/23/2017 05:21 PM, Sergio Durigan Junior wrote:
>>>>
>>>>>>> +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:
>>>>
>>>> I meant "logical" instead of "location".  I was staring at
>>>> gdb/location.c at that time.  :-P
>>>>
>>>>>>
>>>>>>  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?
>>>>
>>>> Sure, the idea is to do something similar to what's done for --readnow.
>>>
>>> Sorry, but I guess I need a few more details on this.
>>>
>>> The way I understand the code at elf_symfile_read, the very first thing
>>> to do would be to check if OBJF_READNEVER is set and return early if it
>>> is.  But it seems that you're proposing something a bit different when
>>> you say that we should "... just skip the elf_locate_sections call."  It
>>> doesn't seem to me that is worth continuing on that function if
>>> OBJF_READNEVER is present.
>>
>> No, you can't return early the very first thing, because
>>  --readnever is supposed to skip _debug_ info, not ELF/minimal symbols...
>> So the "return early" would have to be _after_ the
>> elf_read_minimal_symbols call:
> 
> Hm, OK, it makes sense and I confess I thought "why is Pedro mentioning
> elf_symfile_read if the feature is about skipping DWARF, not ELF?".
> 
>>  static void
>>  elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>>  {
>>    bfd *abfd = objfile->obfd;
>>    struct elfinfo ei;
>>
>>    memset ((char *) &ei, 0, sizeof (ei));
>>    bfd_map_over_sections (abfd, elf_locate_sections, (void *) & ei);
>>  
>>    elf_read_minimal_symbols (objfile, symfile_flags, &ei);
>>
>> I don't know whether we can reorder that.  Maybe we can.
>>
>> When I looked at this quickly yesterday, I saw that elf_location_sections
>> is what finds the stabs and mdebug sections:
>>
>> static void
>> elf_locate_sections (bfd *ignore_abfd, asection *sectp, void *eip)
>> {
>>   struct elfinfo *ei;
>>
>>   ei = (struct elfinfo *) eip;
>>   if (strcmp (sectp->name, ".stab") == 0)
>>     {
>>       ei->stabsect = sectp;
>>     }
>>   else if (strcmp (sectp->name, ".mdebug") == 0)
>>     {
>>       ei->mdebugsect = sectp;
>>     }
>> }
>>
>> and it seemed to be that skipping the section location would make
>> the parts of  elf_symfile_read that actually read the symbols
>> be no-ops, because the stabsect/mdebusect pointers would be NULL.
>>
>> But if returning early or something else works, that's fine.
> 
> OK, thanks for clarifying.
> 
>>> As for the *_build_psymtabs functions, I am doing exactly that: if
>>> objfile->flags contains OBJF_READNEVER, then just return and do nothing.
>>
>> Sure, that should work too.  It's just the difference between
>> skipping checking whether debug info is available (skipping before
>> calling into those), vs letting gdb do the work to figure out whether
>> debug info is available, but then ignore it.
>> The grep for "*_build_psymtabs" was intended as a pointer to find
>> what the relevant code is, including to look at the code that
>> is calling those functions, see if there's something to be done there.
> 
> I think it makes more sense, logically speaking, to not mess with
> elf_symfile_read and instead modify the *_build_psymtabs functions.

Yet, somehow that logic didn't carry to the DWARF reader
changes?  ;-)  dwarf2_has_info is the place that checks
whether the objfile has DWARF sections, 
see dwarf2_per_objfile::dwarf2_per_objfile.
So for DWARF, the patch is checking for readnever _before_
dwarf2_build_psymtabs is reached, by essentially skipping
the "locate_sections" call.  

A small advantage of checking before is that you can skip a little bit
more work.  See the comment in elf_read_minimal_symbols about skipping
work unless stabs, or this bit here:

  if (info->stabsects)
    {
      if (!info->stabstrsect)
	{
	  error (_("The debugging information in `%s' is corrupted.\nThe "
		   "file has a `.stabs' section, but no `.stabstr' section."),
		 name);
	}
      ...

It's really not a bit deal, but to me it'd be more
consistent have all readers do the same logically.

On another note: there's a symbol_file_add_separate call at the tail
end of elf_symfile_read, where we seem to read separate info.  It seems
like that'd end up reading debug info, with "add-symbol-file -readnever"?
Same in read_symbols, I guess.  Or is OBJF_READNEVER etc. somehow
propagated to the separate objfile?

Thanks,
Pedro Alves
  

On Monday, November 27 2017, Pedro Alves wrote:

> On 11/24/2017 08:26 PM, Sergio Durigan Junior wrote:
>> On Friday, November 24 2017, Pedro Alves wrote:
>> 
>>> On 11/24/2017 04:54 AM, Sergio Durigan Junior wrote:
>>>> On Thursday, November 23 2017, Pedro Alves wrote:
>>>>
>>>>> On 11/23/2017 05:21 PM, Sergio Durigan Junior wrote:
>>>>>
>>>>>>>> +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:
>>>>>
>>>>> I meant "logical" instead of "location".  I was staring at
>>>>> gdb/location.c at that time.  :-P
>>>>>
>>>>>>>
>>>>>>>  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?
>>>>>
>>>>> Sure, the idea is to do something similar to what's done for --readnow.
>>>>
>>>> Sorry, but I guess I need a few more details on this.
>>>>
>>>> The way I understand the code at elf_symfile_read, the very first thing
>>>> to do would be to check if OBJF_READNEVER is set and return early if it
>>>> is.  But it seems that you're proposing something a bit different when
>>>> you say that we should "... just skip the elf_locate_sections call."  It
>>>> doesn't seem to me that is worth continuing on that function if
>>>> OBJF_READNEVER is present.
>>>
>>> No, you can't return early the very first thing, because
>>>  --readnever is supposed to skip _debug_ info, not ELF/minimal symbols...
>>> So the "return early" would have to be _after_ the
>>> elf_read_minimal_symbols call:
>> 
>> Hm, OK, it makes sense and I confess I thought "why is Pedro mentioning
>> elf_symfile_read if the feature is about skipping DWARF, not ELF?".
>> 
>>>  static void
>>>  elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>>>  {
>>>    bfd *abfd = objfile->obfd;
>>>    struct elfinfo ei;
>>>
>>>    memset ((char *) &ei, 0, sizeof (ei));
>>>    bfd_map_over_sections (abfd, elf_locate_sections, (void *) & ei);
>>>  
>>>    elf_read_minimal_symbols (objfile, symfile_flags, &ei);
>>>
>>> I don't know whether we can reorder that.  Maybe we can.
>>>
>>> When I looked at this quickly yesterday, I saw that elf_location_sections
>>> is what finds the stabs and mdebug sections:
>>>
>>> static void
>>> elf_locate_sections (bfd *ignore_abfd, asection *sectp, void *eip)
>>> {
>>>   struct elfinfo *ei;
>>>
>>>   ei = (struct elfinfo *) eip;
>>>   if (strcmp (sectp->name, ".stab") == 0)
>>>     {
>>>       ei->stabsect = sectp;
>>>     }
>>>   else if (strcmp (sectp->name, ".mdebug") == 0)
>>>     {
>>>       ei->mdebugsect = sectp;
>>>     }
>>> }
>>>
>>> and it seemed to be that skipping the section location would make
>>> the parts of  elf_symfile_read that actually read the symbols
>>> be no-ops, because the stabsect/mdebusect pointers would be NULL.
>>>
>>> But if returning early or something else works, that's fine.
>> 
>> OK, thanks for clarifying.
>> 
>>>> As for the *_build_psymtabs functions, I am doing exactly that: if
>>>> objfile->flags contains OBJF_READNEVER, then just return and do nothing.
>>>
>>> Sure, that should work too.  It's just the difference between
>>> skipping checking whether debug info is available (skipping before
>>> calling into those), vs letting gdb do the work to figure out whether
>>> debug info is available, but then ignore it.
>>> The grep for "*_build_psymtabs" was intended as a pointer to find
>>> what the relevant code is, including to look at the code that
>>> is calling those functions, see if there's something to be done there.
>> 
>> I think it makes more sense, logically speaking, to not mess with
>> elf_symfile_read and instead modify the *_build_psymtabs functions.
>
> Yet, somehow that logic didn't carry to the DWARF reader
> changes?  ;-)  dwarf2_has_info is the place that checks
> whether the objfile has DWARF sections, 
> see dwarf2_per_objfile::dwarf2_per_objfile.
> So for DWARF, the patch is checking for readnever _before_
> dwarf2_build_psymtabs is reached, by essentially skipping
> the "locate_sections" call.  

You're right, I never thought about making the changes for DWARF.

> A small advantage of checking before is that you can skip a little bit
> more work.  See the comment in elf_read_minimal_symbols about skipping
> work unless stabs, or this bit here:
>
>   if (info->stabsects)
>     {
>       if (!info->stabstrsect)
> 	{
> 	  error (_("The debugging information in `%s' is corrupted.\nThe "
> 		   "file has a `.stabs' section, but no `.stabstr' section."),
> 		 name);
> 	}
>       ...
>
> It's really not a bit deal, but to me it'd be more
> consistent have all readers do the same logically.

Understood.  I switched the logic and now I'm doing all the work on
elf_symfile_read (and consequently dwarf2_has_info, which now is the
right place to put the checks on).

I wasn't sure if I should modify other *_symfile_read functions to obey
the setting, so I left them alone.

> On another note: there's a symbol_file_add_separate call at the tail
> end of elf_symfile_read, where we seem to read separate info.  It seems
> like that'd end up reading debug info, with "add-symbol-file -readnever"?
> Same in read_symbols, I guess.  Or is OBJF_READNEVER etc. somehow
> propagated to the separate objfile?

"add-symbol-file -readnever" doesn't get propagated, the only thing that
does is the "--readnever" global option.  symbol_file_add_separate calls
symbol_file_add_with_addrs (responsible for created the objfile), so I
now propagate the OBJF_READNEVER flag if the main objfile has it.

I'll submit a new version soon.  Thanks,
  

On 11/29/2017 12:59 AM, Sergio Durigan Junior wrote:
> On Monday, November 27 2017, Pedro Alves wrote:

>> It's really not a bit deal, but to me it'd be more
>> consistent have all readers do the same logically.
> 
> Understood.  I switched the logic and now I'm doing all the work on
> elf_symfile_read (and consequently dwarf2_has_info, which now is the
> right place to put the checks on).
> 
> I wasn't sure if I should modify other *_symfile_read functions to obey
> the setting, so I left them alone.

Surely you should?  You'll need to modify coffread.c and xcoffread.c
accordingly.  In the earlier iteration you were changing coffstab_build_psymtabs,
for example.  Basically you're moving checks to the callers instead, so
you should end up with roughly the same number of checks as before, not fewer.
At least logically.  They'll just be in different places.

Thanks,
Pedro Alves