[v3] Add support for the readnever concept

  Changes from v2:

- Fixed a few nits pointed by Eli in the docs.

- Implemented Pedro's suggestion and moved the logic of readnever out
  of the "*_build_psymtabs" and into the elf_symfile_read function.

- Handle the propagation of the OBJF_READNEVER flag to the separate
  debuginfo file.

- Reword commit title, fix a few typos in the commit message.


The purpose of this concept is to turn the load of debugging
information off, either globally (via the '--readnever' option), or
objfile-specific.  The implementation proposed here is an extension of
the patch distributed with Fedora GDB; looking at the Fedora patch
itself and the history, one can see some reasons why it was never
resubmitted:

  - 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 feature 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 patch expands the version shipped with Fedora GDB in order to
make the feature available for all the debuginfo backends, not only
for DWARF.  It also implements a per-objfile flag which can be
activated by using the "-readnever" command when using the
'add-symbol-file' or 'symbol-file' commands.

While implementing the code for the 'symbol-file' command, I noticed a
bug in 'symbol_file_command': GDB adds the symbol file before
finishing parsing all the options, which means that the position of an
option in the command impacts whether it will be considered or not.  I
changed the code there in order to only add the symbol file after all
options have been parsed.

It's also worth mentioning that this patch tests whether GDB correctly
fails to initialize if both '--readnow' and '--readnever' options are
passed.  We didn't have any infrastructure to test that, so I added a
new version of 'gdb_spawn', called 'gdb_spawn_ignore_error', which can
be used to start a GDB that will fail without compromising the entire
testcase.

Tested on the BuildBot.

gdb/ChangeLog:

2017-11-28  Andrew Cagney  <cagney@redhat.com>
	    Joel Brobecker  <brobecker@adacore.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>

	* NEWS (Changes since GDB 8.0: Mention new '--readnever'
	feature.
	* dwarf2read.c: Include "top.h".
	(dwarf2_has_info): Return 0 if READNEVER_SYMBOL_FILES is set.
	* elfread.c: Include "top.h".
	(elf_symfile_read): Do not map over sections with
	'elf_locate_sections' if readnever is on.
	* main.c (validate_readnow_readnever): New function.
	(captured_main_1): Add support for --readnever.
	(print_gdb_help): Document --readnever.
	* objfile-flags.h (enum objfile_flag) <OBJF_READNEVER>: New
	flag.
	* symfile.c (readnever_symbol_files): New global.
	(symbol_file_add_with_addrs): Set 'OBJF_READNEVER' when
	'READNEVER_SYMBOL_FILES' is set.
	(validate_readnow_readnever): New function.
	(symbol_file_command): Handle '-readnever' option.  Move call
	to 'symbol_file_add_main_1' out of the argument parsing loop.
	Call 'validate_readnow_readnever'.
	(add_symbol_file_command): Handle '-readnever' option.
	Document it.  Call 'validate_readnow_readnever'.
	* top.h (readnever_symbol_files): New extern global.

gdb/doc/ChangeLog:

2017-11-28  Andrew Cagney  <cagney@redhat.com>
	    Joel Brobecker  <brobecker@adacore.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.texinfo (File Options): Document --readnever.
	(Commands to Specify Files): Likewise, for 'symbol-file'.

gdb/testsuite/ChangeLog:

2017-11-28  Joel Brobecker  <brobecker@adacore.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* gdb.base/readnever.c, gdb.base/readnever.exp: New files.
	* lib/gdb.exp (default_gdb_spawn): Add 'ignore_error'
	parameter.  Handle case when 'ignore_error' is set.
	(gdb_spawn_ignore_error): New function.
---
 gdb/NEWS                             |  6 +++
 gdb/doc/gdb.texinfo                  | 17 ++++++++
 gdb/dwarf2read.c                     |  4 ++
 gdb/elfread.c                        |  3 +-
 gdb/main.c                           | 37 +++++++++++++++--
 gdb/objfile-flags.h                  |  4 ++
 gdb/symfile.c                        | 33 +++++++++++++---
 gdb/testsuite/gdb.base/readnever.c   | 43 ++++++++++++++++++++
 gdb/testsuite/gdb.base/readnever.exp | 77 ++++++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp            | 18 ++++++---
 gdb/top.h                            |  1 +
 11 files changed, 229 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/readnever.c
 create mode 100644 gdb/testsuite/gdb.base/readnever.exp
  

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Joel Brobecker <brobecker@adacore.com>,
> 	Yao Qi <qiyaoltc@gmail.com>,
> 	Pedro Alves <palves@redhat.com>,
> 	Eli Zaretskii <eliz@gnu.org>,
> 	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 28 Nov 2017 20:21:39 -0500
> 
> Changes from v2:
> 
> - Fixed a few nits pointed by Eli in the docs.
> 
> - Implemented Pedro's suggestion and moved the logic of readnever out
>   of the "*_build_psymtabs" and into the elf_symfile_read function.
> 
> - Handle the propagation of the OBJF_READNEVER flag to the separate
>   debuginfo file.
> 
> - Reword commit title, fix a few typos in the commit message.

OK for the docs.

Thanks.
  

On 11/29/2017 01:21 AM, Sergio Durigan Junior wrote:
> Changes from v2:
> 
> - Fixed a few nits pointed by Eli in the docs.
> 
> - Implemented Pedro's suggestion and moved the logic of readnever out
>   of the "*_build_psymtabs" and into the elf_symfile_read function.

See my comments here:
  https://sourceware.org/ml/gdb-patches/2017-11/msg00766.html

> While implementing the code for the 'symbol-file' command, I noticed a
> bug in 'symbol_file_command': GDB adds the symbol file before
> finishing parsing all the options, which means that the position of an
> option in the command impacts whether it will be considered or not.  I
> changed the code there in order to only add the symbol file after all
> options have been parsed.

Is this the same as:

 https://sourceware.org/ml/gdb-patches/2017-11/msg00656.html

?

Sounds like something that would be better split off to
a precursor patch.

> +++ b/gdb/testsuite/gdb.base/readnever.c
> @@ -0,0 +1,43 @@
> +/* Copyright 2016-2017 Free Software Foundation, Inc.
> +
> +   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/>.  */
> +
> +#include <stdio.h>

This include isn't necessary.

> +
> +static void
> +fun_three (int a, char b, void *c)
> +{
> +  /* Do nothing.  */
> +}
> +
> +static void
> +fun_two (unsigned int p, const char *y)
> +{
> +  fun_three ((int) p, '1', (void *) y);
> +}
> +
> +static void
> +fun_one (int *x)
> +{
> +  fun_two (10, (const char *) x);
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 10;
> +
> +  fun_one (&a);
> +  return 0;
> +}

> +# Test invalid combination of flags.
> +save_vars { GDBFLAGS } {
> +    append GDBFLAGS " --readnever --readnow"
> +    gdb_exit
> +    gdb_spawn_ignore_error
> +
> +    set test "test readnow and readnever at the same time"
> +    gdb_test_multiple "" $test {
> +	eof {
> +	    pass $test
> +	}
> +    }

I had suggested to match GDB's error output before the eof,
so I'm surprised to see only eof expected?

> +}
> +

>  # Default gdb_spawn procedure.
>  
> -proc default_gdb_spawn { } {
> +proc default_gdb_spawn { { ignore_error 0 } } {
>      global use_gdb_stub
>      global GDB
>      global INTERNAL_GDBFLAGS GDBFLAGS
> @@ -1610,11 +1610,12 @@ proc default_gdb_spawn { } {
>  	}
>      }
>      set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]
> -    if { $res < 0 || $res == "" } {
> -	perror "Spawning $GDB failed."
> -	return 1
> +    if { !$ignore_error } {
> +	if { $res < 0 || $res == "" } {
> +	    perror "Spawning $GDB failed."
> +	    return 1
> +	}
>      }

Hmm, I think I'm now confused on why this is needed in the first
place?  GDB-the-executable should be starting successfully, 
print some error output, and exit.  What error do you see
here, exactly?

> -
>      set gdb_spawn_id $res
>      return 0
>  }

Thanks,
Pedro Alves
  

On Wednesday, November 29 2017, Pedro Alves wrote:

> On 11/29/2017 01:21 AM, Sergio Durigan Junior wrote:
>> Changes from v2:
>> 
>> - Fixed a few nits pointed by Eli in the docs.
>> 
>> - Implemented Pedro's suggestion and moved the logic of readnever out
>>   of the "*_build_psymtabs" and into the elf_symfile_read function.
>
> See my comments here:
>   https://sourceware.org/ml/gdb-patches/2017-11/msg00766.html

I'll address them, thanks.

>> While implementing the code for the 'symbol-file' command, I noticed a
>> bug in 'symbol_file_command': GDB adds the symbol file before
>> finishing parsing all the options, which means that the position of an
>> option in the command impacts whether it will be considered or not.  I
>> changed the code there in order to only add the symbol file after all
>> options have been parsed.
>
> Is this the same as:
>
>  https://sourceware.org/ml/gdb-patches/2017-11/msg00656.html
>
> ?

No, it seems like a different issue, even though it's in the same part
of the code.

> Sounds like something that would be better split off to
> a precursor patch.

Sure, I can do that.

>> +++ b/gdb/testsuite/gdb.base/readnever.c
>> @@ -0,0 +1,43 @@
>> +/* Copyright 2016-2017 Free Software Foundation, Inc.
>> +
>> +   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/>.  */
>> +
>> +#include <stdio.h>
>
> This include isn't necessary.

Removed.

>> +
>> +static void
>> +fun_three (int a, char b, void *c)
>> +{
>> +  /* Do nothing.  */
>> +}
>> +
>> +static void
>> +fun_two (unsigned int p, const char *y)
>> +{
>> +  fun_three ((int) p, '1', (void *) y);
>> +}
>> +
>> +static void
>> +fun_one (int *x)
>> +{
>> +  fun_two (10, (const char *) x);
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> +  int a = 10;
>> +
>> +  fun_one (&a);
>> +  return 0;
>> +}
>
>> +# Test invalid combination of flags.
>> +save_vars { GDBFLAGS } {
>> +    append GDBFLAGS " --readnever --readnow"
>> +    gdb_exit
>> +    gdb_spawn_ignore_error
>> +
>> +    set test "test readnow and readnever at the same time"
>> +    gdb_test_multiple "" $test {
>> +	eof {
>> +	    pass $test
>> +	}
>> +    }
>
> I had suggested to match GDB's error output before the eof,
> so I'm surprised to see only eof expected?

Wow, I spent a lot of time trying to make this work before and didn't
succeed, but now I think I managed to do it.  See below.

>> +}
>> +
>
>>  # Default gdb_spawn procedure.
>>  
>> -proc default_gdb_spawn { } {
>> +proc default_gdb_spawn { { ignore_error 0 } } {
>>      global use_gdb_stub
>>      global GDB
>>      global INTERNAL_GDBFLAGS GDBFLAGS
>> @@ -1610,11 +1610,12 @@ proc default_gdb_spawn { } {
>>  	}
>>      }
>>      set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]
>> -    if { $res < 0 || $res == "" } {
>> -	perror "Spawning $GDB failed."
>> -	return 1
>> +    if { !$ignore_error } {
>> +	if { $res < 0 || $res == "" } {
>> +	    perror "Spawning $GDB failed."
>> +	    return 1
>> +	}
>>      }
>
> Hmm, I think I'm now confused on why this is needed in the first
> place?  GDB-the-executable should be starting successfully, 
> print some error output, and exit.  What error do you see
> here, exactly?

Here's what I see when I try to use gdb_start:

  spawn /home/sergio/work/src/git/binutils-gdb/readnever/build-64/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /home/sergio/work/src/git/binutils-gdb/readnever/build-64/gdb/testsuite/../data-directory --readnever --readnow
  /home/sergio/work/src/git/binutils-gdb/readnever/build-64/gdb/testsuite/../../gdb/gdb: '--readnow' and '--readnever' cannot be specified simultaneously
  ERROR: : spawn id exp8 not open
      while executing
  "expect {
  -i exp8 -timeout 10 
          -re "$gdb_prompt $" { 
              verbose "Setting height to 0." 2
          }
          timeout {
              warning "Couldn't set the height to 0"
  ..."
      ("uplevel" body line 1)
      invoked from within
  "uplevel $body" NONE : spawn id exp8 not open
  WARNING: Couldn't set the height to 0
  ERROR: : spawn id exp8 not open
      while executing
  "expect {
  -i exp8 -timeout 10 
          -re "$gdb_prompt $" {
              verbose "Setting width to 0." 2
          }
          timeout {
              warning "Couldn't set the width to 0."
          }..."
      ("uplevel" body line 1)
      invoked from within
  "uplevel $body" NONE : spawn id exp8 not open
  WARNING: Couldn't set the width to 0.
  ERROR: : spawn id exp8 not open
      while executing
  "expect {
  -i exp8 -timeout 10 
          -re ".*A problem internal to GDB has been detected" {
              fail "$message (GDB internal error)"
              gdb_internal_error..."
      ("uplevel" body line 1)
      invoked from within
  "uplevel $body" NONE : spawn id exp8 not open
  UNRESOLVED: gdb.base/readnever.exp: test readnow and readnever at the same time

It doesn't work because gdb_start does more than spawn GDB; it also sets
height/width and other things.  So I tried invoking gdb_spawn directly,
and it worked fine!  Thanks for poking me a bit more.

I'll send a v4 with the necessary adjustments.

Thanks,