[01/13] gdb/dwarf: change read_loclist_index complaints into errors

Message ID 20210120053925.142862-2-simon.marchi@polymtl.ca
State New
Headers show
Series
  • DWARF 5 rnglists & loclists fixes (PR 26813)
Related show

Commit Message

Mike Frysinger via Gdb-patches Jan. 20, 2021, 5:39 a.m.
From: Simon Marchi <simon.marchi@efficios.com>


Unlike read_rnglists_index, read_loclist_index uses complaints when it
detects an inconsistency (a DW_FORM_loclistx value without a
.debug_loclists section or an offset outside of the section).  I really
think they should be errors, since there's no point in continuing if
this situation happens, we will likely segfault or read garbage.

gdb/ChangeLog:

	* dwarf2/read.c (read_loclist_index): Change complaints into
	errors.

Change-Id: Ic3a1cf6e682d47cb6e739dd76fd7ca5be2637e10
---
 gdb/dwarf2/read.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.30.0

Comments

Mike Frysinger via Gdb-patches Jan. 28, 2021, 3:17 p.m. | #1
> From: Simon Marchi <simon.marchi@efficios.com>

> 

> Unlike read_rnglists_index, read_loclist_index uses complaints when it

> detects an inconsistency (a DW_FORM_loclistx value without a

> .debug_loclists section or an offset outside of the section).  I really

> think they should be errors, since there's no point in continuing if

> this situation happens, we will likely segfault or read garbage.

> 


Makes sense.

Although unless I misunderstand something, isn't the difference here 
that the error will actually throw an exception and therefore stop 
reading of the compilation unit if not the whole object file debug 
information?

If this is the case, then considering the difference in usage between 
the two, having a wrong loclist information can still provide a correct 
line table information, but having a wrong rnglist information in my 
mind creates a more serious issue.

On the other hand, how much can one trust in the information correctness 
if either of those are wrong.
Mike Frysinger via Gdb-patches Jan. 28, 2021, 3:42 p.m. | #2
On 2021-01-28 10:17 a.m., Zoran Zaric wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>

>>

>> Unlike read_rnglists_index, read_loclist_index uses complaints when it

>> detects an inconsistency (a DW_FORM_loclistx value without a

>> .debug_loclists section or an offset outside of the section).  I really

>> think they should be errors, since there's no point in continuing if

>> this situation happens, we will likely segfault or read garbage.

>>

> 

> Makes sense.

> 

> Although unless I misunderstand something, isn't the difference here that the error will actually throw an exception and therefore stop reading of the compilation unit if not the whole object file debug information?

> 

> If this is the case, then considering the difference in usage between the two, having a wrong loclist information can still provide a correct line table information, but having a wrong rnglist information in my mind creates a more serious issue.

> 

> On the other hand, how much can one trust in the information correctness if either of those are wrong.


Indeed, `error` throws an exception that gets handled... I don't know
where.  If you are reading partial symtabs, it probably goes up to
dwarf2_build_psymtabs, so it stops the processing for the whole object
file.

I think it's correct from the read_rnglists_index and read_loclist_index
functions point of view to throw if they encounter invalid data and
can't return something meaningful.  If we want to make error handling a
bit more granular, we could catch the error in the caller
(read_attribute_reprocess), make it display a complaint, and continue as
if the attribute wasn't present.

We won't have location or range information for this entity (or perhaps
for all entities, if we fail to read all of them), but perhaps other
parts of the debug info will be read fine.  And those other parts could
maybe be useful to some user, versus aborting completely and having no
debug info at all.

However, I don't intend to do this at the moment, because it would be
quite a lot of work to do and test properly, in the end to accomodate an
hypothetical buggy compiler.  Maybe if/when we have a concrete instance
of a widely available compiler producing such buggy debug info, we can
revisit this idea.

Thanks for the review!

Simon
Tom Tromey Feb. 25, 2021, 7:20 p.m. | #3
>> If this is the case, then considering the difference in usage between

>> the two, having a wrong loclist information can still provide a

>> correct line table information, but having a wrong rnglist

>> information in my mind creates a more serious issue.


>> On the other hand, how much can one trust in the information

>> correctness if either of those are wrong.


Simon> Indeed, `error` throws an exception that gets handled... I don't know
Simon> where.  If you are reading partial symtabs, it probably goes up to
Simon> dwarf2_build_psymtabs, so it stops the processing for the whole object
Simon> file.

FWIW there is at least one bug open about this behavior.

It's not completely clear what would be best.  One idea I consider
sometimes is to throw out just the known-bad CU, but try to read the
remaining ones.  This might not be too difficult to implement.

Simon> I think it's correct from the read_rnglists_index and read_loclist_index
Simon> functions point of view to throw if they encounter invalid data and
Simon> can't return something meaningful.  If we want to make error handling a
Simon> bit more granular, we could catch the error in the caller
Simon> (read_attribute_reprocess), make it display a complaint, and continue as
Simon> if the attribute wasn't present.

Things like this could be considered on a case-by-case basis as well.

Simon> However, I don't intend to do this at the moment, because it would be
Simon> quite a lot of work to do and test properly, in the end to accomodate an
Simon> hypothetical buggy compiler.  Maybe if/when we have a concrete instance
Simon> of a widely available compiler producing such buggy debug info, we can
Simon> revisit this idea.

For the plan of just rejecting an individual bad CU, the problem isn't
so much consistently bad compilers as that, when one does trip across
bad DWARF, it makes the entire result unusable -- even if the bug is
localized somehow.

Tom

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 9032186ef89e..2b76ed001616 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -20190,19 +20190,22 @@  read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
 
   section->read (objfile);
   if (section->buffer == NULL)
-    complaint (_("DW_FORM_loclistx used without .debug_loclists "
-		"section [in module %s]"), objfile_name (objfile));
+    error (_("DW_FORM_loclistx used without .debug_loclists "
+	     "section [in module %s]"), objfile_name (objfile));
+
   struct loclists_rnglists_header header;
   read_loclists_rnglists_header (&header, section);
   if (loclist_index >= header.offset_entry_count)
-    complaint (_("DW_FORM_loclistx pointing outside of "
-		".debug_loclists offset array [in module %s]"),
-		objfile_name (objfile));
+    error (_("DW_FORM_loclistx pointing outside of "
+	     ".debug_loclists offset array [in module %s]"),
+	   objfile_name (objfile));
+
   if (loclist_base + loclist_index * cu->header.offset_size
 	>= section->size)
-    complaint (_("DW_FORM_loclistx pointing outside of "
-		".debug_loclists section [in module %s]"),
-		objfile_name (objfile));
+    error (_("DW_FORM_loclistx pointing outside of "
+	     ".debug_loclists section [in module %s]"),
+	   objfile_name (objfile));
+
   const gdb_byte *info_ptr
     = section->buffer + loclist_base + loclist_index * cu->header.offset_size;