[PATCH] use error, not internal_error, in dwarf2-frame.c

[PATCH] use error, not internal_error, in dwarf2-frame.c

[Tom Tromey]

While writing a test case, I happened to introduce a bit of invalid
CFI.  This caused gdb to call internal_error.

I think that gdb should not call internal_error for bad DWARF.
Instead, it should simply throw an ordinary exception.

This patch changes dwarf2-frame.c to follow this rule.

No test case; but I could write one for one of the errors if you want.

2013-11-13  Tom Tromey  <tromey@redhat.com>

	* dwarf2-frame.c (execute_cfa_program, dwarf2_compile_cfa_to_ax)
	(dwarf2_frame_cache, dwarf2_frame_prev_register)
	(encoding_for_size, read_encoded_value): Use error, not
	internal_error.
---
 gdb/ChangeLog      |  7 +++++++
 gdb/dwarf2-frame.c | 20 ++++++++------------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index e05236f..8f55e9f 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -680,8 +680,7 @@ bad CFI data; mismatched DW_CFA_restore_state at %s"),
 	      break;
 
 	    default:
-	      internal_error (__FILE__, __LINE__,
-			      _("Unknown CFI encountered."));
+	      error (_("Unknown CFI encountered."));
 	    }
 	}
     }
@@ -954,7 +953,7 @@ dwarf2_compile_cfa_to_ax (struct agent_expr *expr, struct axs_value *loc,
       break;
 
     default:
-      internal_error (__FILE__, __LINE__, _("Unknown CFA rule."));
+      error (_("Unknown CFA rule."));
     }
 }
 
@@ -1112,7 +1111,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 	  break;
 
 	default:
-	  internal_error (__FILE__, __LINE__, _("Unknown CFA rule."));
+	  error (_("Unknown CFA rule."));
 	}
     }
   if (ex.reason < 0)
@@ -1364,7 +1363,7 @@ dwarf2_frame_prev_register (struct frame_info *this_frame, void **this_cache,
       return cache->reg[regnum].loc.fn (this_frame, this_cache, regnum);
 
     default:
-      internal_error (__FILE__, __LINE__, _("Unknown register rule."));
+      error (_("Unknown register rule."));
     }
 }
 
@@ -1577,7 +1576,7 @@ encoding_for_size (unsigned int size)
     case 8:
       return DW_EH_PE_udata8;
     default:
-      internal_error (__FILE__, __LINE__, _("Unsupported address size"));
+      error (_("Unsupported address size"));
     }
 }
 
@@ -1593,8 +1592,7 @@ read_encoded_value (struct comp_unit *unit, gdb_byte encoding,
   /* GCC currently doesn't generate DW_EH_PE_indirect encodings for
      FDE's.  */
   if (encoding & DW_EH_PE_indirect)
-    internal_error (__FILE__, __LINE__, 
-		    _("Unsupported encoding: DW_EH_PE_indirect"));
+    error (_("Unsupported encoding: DW_EH_PE_indirect"));
 
   *bytes_read_ptr = 0;
 
@@ -1626,8 +1624,7 @@ read_encoded_value (struct comp_unit *unit, gdb_byte encoding,
 	}
       break;
     default:
-      internal_error (__FILE__, __LINE__,
-		      _("Invalid or unsupported encoding"));
+      error (_("Invalid or unsupported encoding"));
     }
 
   if ((encoding & 0x07) == 0x00)
@@ -1674,8 +1671,7 @@ read_encoded_value (struct comp_unit *unit, gdb_byte encoding,
       *bytes_read_ptr += 8;
       return (base + bfd_get_signed_64 (unit->abfd, (bfd_byte *) buf));
     default:
-      internal_error (__FILE__, __LINE__,
-		      _("Invalid or unsupported encoding"));
+      error (_("Invalid or unsupported encoding"));
     }
 }
 \f
-- 
1.8.1.4

Re: [PATCH] use error, not internal_error, in dwarf2-frame.c

[Joel Brobecker]

> I think that gdb should not call internal_error for bad DWARF.
> Instead, it should simply throw an ordinary exception.

FWIW, I agree with the reasoning.

> 2013-11-13  Tom Tromey  <tromey@redhat.com>
> 
> 	* dwarf2-frame.c (execute_cfa_program, dwarf2_compile_cfa_to_ax)
> 	(dwarf2_frame_cache, dwarf2_frame_prev_register)
> 	(encoding_for_size, read_encoded_value): Use error, not
> 	internal_error.

-- 
Joel

Re: [PATCH] use error, not internal_error, in dwarf2-frame.c

[Andrew Burgess]

On 13/11/2013 8:43 PM, Tom Tromey wrote:
> 
> I think that gdb should not call internal_error for bad DWARF.
> Instead, it should simply throw an ordinary exception.

Sounds reasonable.

> 
> diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
> index e05236f..8f55e9f 100644
> --- a/gdb/dwarf2-frame.c
> +++ b/gdb/dwarf2-frame.c
> @@ -680,8 +680,7 @@ bad CFI data; mismatched DW_CFA_restore_state at %s"),
>  	      break;
>  
>  	    default:
> -	      internal_error (__FILE__, __LINE__,
> -			      _("Unknown CFI encountered."));
> +	      error (_("Unknown CFI encountered."));

I wonder if we could make this error message clearer?  Will an "average"
user understand what a CFI is?  How about something like:

  "Invalid DWARF debugging information: Unknown CFI encountered"

This might give a better idea what is going on.  If you agree then the
same applies to the rest of these errors too.

Just a thought.

Thanks,
Andrew

Re: [PATCH] use error, not internal_error, in dwarf2-frame.c

[Doug Evans]

On Thu, Nov 14, 2013 at 3:23 AM, Andrew Burgess <aburgess@broadcom.com> wrote:
> On 13/11/2013 8:43 PM, Tom Tromey wrote:
>>
>> I think that gdb should not call internal_error for bad DWARF.
>> Instead, it should simply throw an ordinary exception.
>
> Sounds reasonable.
>
>>
>> diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
>> index e05236f..8f55e9f 100644
>> --- a/gdb/dwarf2-frame.c
>> +++ b/gdb/dwarf2-frame.c
>> @@ -680,8 +680,7 @@ bad CFI data; mismatched DW_CFA_restore_state at %s"),
>>             break;
>>
>>           default:
>> -           internal_error (__FILE__, __LINE__,
>> -                           _("Unknown CFI encountered."));
>> +           error (_("Unknown CFI encountered."));
>
> I wonder if we could make this error message clearer?  Will an "average"
> user understand what a CFI is?  How about something like:
>
>   "Invalid DWARF debugging information: Unknown CFI encountered"
>
> This might give a better idea what is going on.  If you agree then the
> same applies to the rest of these errors too.
>
> Just a thought.

There's a convention for the style of these kinds of messages in dwarf2read.c.
IWBN to use the same convention here.

"Dwarf Error: blah [in module %s]"

E.g.,

  if (header->version != 2 && header->version != 3 && header->version != 4)
    error (_("Dwarf Error: wrong version in compilation unit header "
           "(is %d, should be 2, 3, or 4) [in module %s]"), header->version,
           filename);

I didn't check to see whether the file name is available in any of the
instances in dwarf2-frame.c, but if available IWBN to print it.