[binutils-gdb] gdb: Don't fault for 'maint print psymbols' when using an index

[binutils-gdb] gdb: Don't fault for 'maint print psymbols' when using an index

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=3dd9bb462012df685d6d41300dacedae1c81e28a

commit 3dd9bb462012df685d6d41300dacedae1c81e28a
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Sat Aug 31 21:46:27 2019 +0100

    gdb: Don't fault for 'maint print psymbols' when using an index
    
    I found that these tests:
    
      make check-gdb RUNTESTFLAGS="--target_board=cc-with-gdb-index gdb.base/maint.exp"
      make check-gdb RUNTESTFLAGS="--target_board=cc-with-debug-names gdb.base/maint.exp"
    
    were causing GDB to segfault.  It turns out that this test runs this
    command:
    
      maint print psymbols -pc main /path/to/some/file
    
    which tries to lookup the partial_symtab for 'main'.  The problem is
    that there is no partial_symtab for 'main' as we are using the
    .gdb_index or .debug_names instead of partial_symtabs.
    
    What happens is that maintenance_print_symbols calls
    find_pc_sect_psymtab, which looks for the partial_symtab in the
    objfile's objfile->partial_symtabs->psymtabs_addrmap.
    
    This is a problem because when we are using the indexes
    psymtabs_addrmap is reused to hold things other than partial_symtabs,
    this can be seen in dwarf2read.c in create_addrmap_from_index and
    create_addrmap_from_aranges.  If we then lookup in psymtabs_addrmap we
    end up returning a pointer to something that isn't really a
    partial_symtab, after which everything goes wrong.
    
    Initially I simply added a check at the start of find_pc_sect_psymtab
    that the objfile had some partial_symtabs, like:
    
      if (objfile->partial_symtabs->psymtabs == NULL)
        return NULL;
    
    Figuring that if there were no partial_symtabs at all then this
    function should always return NULL, however, this caused a failure in
    the test gdb.python/py-event.exp which I didn't dig into too deeply,
    but seems to be that in this tests there are initially no psymtabs,
    but the second part of find_pc_sect_psymtab does manage to read some
    in from somewhere, with the check I added the test fails as we
    returned NULL here and this caused GDB to load in the full symtabs
    earlier than was expected.
    
    Instead I chose to guard only the access to psymtabs_addrmap with a
    check that the function has some psymtabs.  This allows my original
    tests to pass, and the py-event.exp test to pass too.
    
    Now, a good argument can be made that we simply should never call
    find_pc_sect_psymtab on an objfile that is using indexes instead of
    partial_symtabs.  I did consider this approach, we could easily add an
    assert into find_pc_sect_psymtab that if we find a partial_symtab in
    psymtabs_addrmap then the psymtabs pointer must be non-null.  The
    responsibility would then be on the user of find_pc_sect_psymtab to
    ensure that the objfile being checked is suitable.  In the end I
    didn't take this approach as the check in find_pc_sect_psymtab is
    cheap and this ensures that any future miss-uses of the function will
    not cause problems.
    
    I also extended the comment on psymtabs_addrmap to indicate that it
    holds more than just partial_symtabs as this was not at all clear from
    the original comment, and caused me some confusion when I was
    initially debugging this problem.
    
    gdb/ChangeLog:
    
    	* psymtab.c (find_pc_sect_psymtab): Move baseaddr local into more
    	inner scope, add check that the objfile has psymtabs before
    	checking psymtabs_addrmap.
    	* psymtab.h (psymtab_storage) <psymtabs_addrmap>: Extend comment.

Diff:
---
 gdb/ChangeLog |  7 +++++++
 gdb/psymtab.c | 24 +++++++++++++++++-------
 gdb/psymtab.h |  6 +++++-
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 986a701..9540c4f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-09-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* psymtab.c (find_pc_sect_psymtab): Move baseaddr local into more
+	inner scope, add check that the objfile has psymtabs before
+	checking psymtabs_addrmap.
+	* psymtab.h (psymtab_storage) <psymtabs_addrmap>: Extend comment.
+
 2019-09-12  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
 
 	* NEWS: Announce that Ada task names are now shown at more places,
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index e9cc8c3..031dbd9 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -301,14 +301,24 @@ find_pc_sect_psymtab (struct objfile *objfile, CORE_ADDR pc,
 		      struct obj_section *section,
 		      struct bound_minimal_symbol msymbol)
 {
-  CORE_ADDR baseaddr = ANOFFSET (objfile->section_offsets,
-				 SECT_OFF_TEXT (objfile));
-
-  /* Try just the PSYMTABS_ADDRMAP mapping first as it has better granularity
-     than the later used TEXTLOW/TEXTHIGH one.  */
-
-  if (objfile->partial_symtabs->psymtabs_addrmap != NULL)
+  /* Try just the PSYMTABS_ADDRMAP mapping first as it has better
+     granularity than the later used TEXTLOW/TEXTHIGH one.  However, we need
+     to take care as the PSYMTABS_ADDRMAP can hold things other than partial
+     symtabs in some cases.
+
+     This function should only be called for objfiles that are using partial
+     symtabs, not for objfiles that are using indexes (.gdb_index or
+     .debug_names), however 'maintenance print psymbols' calls this function
+     directly for all objfiles.  If we assume that PSYMTABS_ADDRMAP contains
+     partial symtabs then we will end up returning a pointer to an object
+     that is not a partial_symtab, which doesn't end well.  */
+
+  if (objfile->partial_symtabs->psymtabs != NULL
+      && objfile->partial_symtabs->psymtabs_addrmap != NULL)
     {
+      CORE_ADDR baseaddr = ANOFFSET (objfile->section_offsets,
+				     SECT_OFF_TEXT (objfile));
+
       struct partial_symtab *pst
 	= ((struct partial_symtab *)
 	   addrmap_find (objfile->partial_symtabs->psymtabs_addrmap,
diff --git a/gdb/psymtab.h b/gdb/psymtab.h
index aed6862..0ad2b49 100644
--- a/gdb/psymtab.h
+++ b/gdb/psymtab.h
@@ -109,7 +109,11 @@ public:
   /* Map addresses to the entries of PSYMTABS.  It would be more efficient to
      have a map per the whole process but ADDRMAP cannot selectively remove
      its items during FREE_OBJFILE.  This mapping is already present even for
-     PARTIAL_SYMTABs which still have no corresponding full SYMTABs read.  */
+     PARTIAL_SYMTABs which still have no corresponding full SYMTABs read.
+
+     The DWARF parser reuses this addrmap to store things other than
+     psymtabs in the cases where debug information is being read from, for
+     example, the .debug-names section.  */
 
   struct addrmap *psymtabs_addrmap = nullptr;