Re: [PATCH v3] Add support for the readnever concept

[Sergio Durigan Junior <sergiodj@redhat.com>]

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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/