public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: Simon Marchi <simon.marchi@polymtl.ca>,
	Simon Marchi <simon.marchi@efficios.com>,
	gdb-patches@sourceware.org
Subject: Re: [PATCH v2] gdb/dwarf: dump cooked index contents in cooked_index_functions::dump
Date: Tue, 31 Jan 2023 09:21:54 +0000	[thread overview]
Message-ID: <877cx3uki5.fsf@redhat.com> (raw)
In-Reply-To: <03ff1644-df3d-2e6b-f2aa-49767abc580b@polymtl.ca>

Simon Marchi <simon.marchi@polymtl.ca> writes:

>> With this patch I'm seeing this failure:
>> 
>> 
>> (gdb) PASS: gdb.dwarf2/dw2-error.exp: file dw2-error
>> maint print objfiles /tmp/build/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error
>> 
>> Object file /tmp/build/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error:  Objfile at 0x40dada0, bfd at 0x426bf70, 50 minsyms
>> 
>> Cooked index in use:
>> 
>> ../../src/gdb/../gdbsupport/gdb-checked-static-cast.h:58: internal-error: checked_static_cast: Assertion `result != nullptr' failed.
>
> Ah, sorry about that.
>
> This is because dwarf2_build_psymtabs_hard throws, because of a bad
> DWARF header (this is the point of the test), so no index ever gets
> installed in index_table.
>
> The obvious fix, to fix the test quickly, would be to add a nullptr
> check in cooked_index_functions::dump.  However, the result is weird:
>
>     (gdb) maintenance print objfiles
>
>     Object file /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error:  Objfile at 0x614000007240, bfd at 0x6120000dbdc0, 24 minsyms
>
>     Cooked index in use:
>
>     (gdb)
>
> It says the cooked index is in use, but there's nothing, as we never
> actually managed to build a cooked index.  Maybe we should consider that
> an invalid state, and remove the cooked_index_functions instance from
> the objfile's quick symbol functions, if failing to read the psymtabs.
>
> Of course, this state (cooked_index_functions in the objfile's qf +
> per_objfile->index_table == nullptr) exists before
> objfile::require_partial_symbols has been called on the objfile, for
> quick symbol functions that can read partial symbols lazily (i.e. just
> the DWARF cooked index at this time), as we haven't tried to read
> partial symbols yet.  But after we called
> objfile::require_partial_symbols, I don't think it's a state that makes
> sense.
>
> See the patch below for a quick prototype.
> quick_symbol_functions::read_partial_symbols now returns a bool, and if
> it returns false (failure), objfile::require_partial_symbols removes
> that quick symbol instance from the qf list.
>
> The result is:
>
>     (gdb) file testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error
>     Reading symbols from testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error...
>     Dwarf Error: wrong version in compilation unit header (is 153, should be 2, 3, 4 or 5) [in module /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error]
>     (No debugging symbols found in testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error)
>
> The "No debugging symbols" message now appears.  It wouldn't appear,
> because objfile_has_symbols would return true, which is kind of a lie.
> When the cooked index qf is removed, it returns false.
>
> And the problematic command that crashes the test now shows:
>
>     (gdb) maintenance print objfiles
>
>     Object file /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error:  Objfile at 0x614000007240, bfd at 0x6120000dbdc0, 24 minsyms
>
>     (gdb)
>
> Which is the same that would be shown if the objfile didn't have DWARF
> info at all in the first place.
>
> Please let me know if you think this approach makes sense, and if I'll
> finish and polish the patch.

I think this would be a good improvement.  Especially the "No debugging
symbols found..." message now being printed, is, I think, a good thing.

Thanks,
Andrew


>
> Simon
>
> From ef55dbecacc7ec5f5e1cea82fc731b21e078783e Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Mon, 30 Jan 2023 21:38:28 -0500
> Subject: [PATCH] fix
>
> Change-Id: Ic6b741324cca13239ac84c5dbef474534ad08870
> ---
>  gdb/dwarf2/read.c   | 14 ++++++++++----
>  gdb/quick-symbol.h  |  8 ++++++--
>  gdb/symfile-debug.c | 17 +++++++++++++----
>  3 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index e8a3e359adaa..118cbd450668 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -5422,13 +5422,13 @@ dwarf2_initialize_objfile (struct objfile *objfile)
>  
>  /* Build a partial symbol table.  */
>  
> -static void
> +static bool
>  dwarf2_build_psymtabs (struct objfile *objfile)
>  {
>    dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
>  
>    if (per_objfile->per_bfd->index_table != nullptr)
> -    return;
> +    return true;
>  
>    try
>      {
> @@ -5436,10 +5436,14 @@ dwarf2_build_psymtabs (struct objfile *objfile)
>  
>        /* (maybe) store an index in the cache.  */
>        global_index_cache.store (per_objfile);
> +
> +      return true;
>      }
>    catch (const gdb_exception_error &except)
>      {
>        exception_print (gdb_stderr, except);
> +
> +      return false;
>      }
>  }
>  
> @@ -18622,10 +18626,12 @@ struct cooked_index_functions : public dwarf2_base_index_functions
>      return true;
>    }
>  
> -  void read_partial_symbols (struct objfile *objfile) override
> +  bool read_partial_symbols (struct objfile *objfile) override
>    {
>      if (dwarf2_has_info (objfile, nullptr))
> -      dwarf2_build_psymtabs (objfile);
> +      return dwarf2_build_psymtabs (objfile);
> +
> +    return true;
>    }
>  };
>  
> diff --git a/gdb/quick-symbol.h b/gdb/quick-symbol.h
> index a7fea2ccb494..41a34ec6e200 100644
> --- a/gdb/quick-symbol.h
> +++ b/gdb/quick-symbol.h
> @@ -221,9 +221,13 @@ struct quick_symbol_functions
>    }
>  
>    /* Read the partial symbols for OBJFILE.  This will only ever be
> -     called if can_lazily_read_symbols returns true.  */
> -  virtual void read_partial_symbols (struct objfile *objfile)
> +     called if can_lazily_read_symbols returns true.
> +
> +     Return true on success.  Return false on failure, in which case the
> +     quick_symbol_functions instance is removed from the objfile's QF list.  */
> +  virtual bool read_partial_symbols (struct objfile *objfile)
>    {
> +    return true;
>    }
>  };
>  
> diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
> index efc6bcdf2bdf..f244ec0d5d6e 100644
> --- a/gdb/symfile-debug.c
> +++ b/gdb/symfile-debug.c
> @@ -313,7 +313,7 @@ objfile::dump ()
>      gdb_printf (gdb_stdlog, "qf->dump (%s)\n",
>  		objfile_debug_name (this));
>  
> -  for (const auto &iter : qf)
> +  for (const auto &iter : this->qf_require_partial_symbols ())
>      iter->dump (this);
>  }
>  
> @@ -528,9 +528,10 @@ objfile::require_partial_symbols (bool verbose)
>        flags |= OBJF_PSYMTABS_READ;
>  
>        bool printed = false;
> -      for (const auto &iter : qf)
> +      for (auto iter = qf.begin (), prev = qf.before_begin ();
> +	   iter != qf.end (); )
>  	{
> -	  if (iter->can_lazily_read_symbols ())
> +	  if ((*iter)->can_lazily_read_symbols ())
>  	    {
>  	      if (verbose && !printed)
>  		{
> @@ -538,7 +539,15 @@ objfile::require_partial_symbols (bool verbose)
>  			      objfile_name (this));
>  		  printed = true;
>  		}
> -	      iter->read_partial_symbols (this);
> +
> +	      bool success = (*iter)->read_partial_symbols (this);
> +	      if (success)
> +		{
> +		  ++iter;
> +		  ++prev;
> +		}
> +	      else
> +		iter = qf.erase_after (prev);
>  	    }
>  	}
>        if (printed && !objfile_has_symbols (this))
>
> base-commit: 9c6e6c8f4b07a51a47c10a84e10a938b64b65fd5
> -- 
> 2.39.0


  reply	other threads:[~2023-01-31  9:22 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-30 16:03 Simon Marchi
2023-01-30 16:08 ` Simon Marchi
2023-01-30 19:07 ` Tom Tromey
2023-01-30 20:08   ` Simon Marchi
2023-01-30 21:35 ` Andrew Burgess
2023-01-31  3:06   ` Simon Marchi
2023-01-31  9:21     ` Andrew Burgess [this message]
2023-01-31 15:49       ` Simon Marchi
2023-01-31 17:27         ` Tom Tromey
2023-01-31 14:19     ` Tom Tromey
2023-01-31 15:35       ` Simon Marchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877cx3uki5.fsf@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=simon.marchi@efficios.com \
    --cc=simon.marchi@polymtl.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).