fhpd vs RuntimeExceptions

fhpd vs RuntimeExceptions

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

Hi,

While investigating some bugs I noticed that the fhpd sometimes swallows
the RuntimeExceptions from the core (and there I thought all was
fine...). This makes debugging the fhpd itself a little hard. I added a
possible exception cause to the Message class and while I was at it
added checks to make sure we don't present the user with an empty or
null message (which is very uninformative). Currently we always print
these possible exception causes when they are attached to a Message in
CLI.flushMessages(). But we could add a fhpd 'frysk_debug mode' so they
are only printed if an 'expert' is running frysk. For now I assume all
users are experts and want to see them, but maybe it is annoying.
Opinions?

2007-11-14  Mark Wielaard  <mwielaard@redhat.com>
    
     * CLI.java (addMessage): New variant that takes a possible
     exception cause.
     (doAttach): Use new addMessage().
     (execCommand): Likewise.
     (flushMessages): Add possible exception cause if present.
     Actually flush outWriter.
     * EvalCommands.java (eval): Add possible RuntimeException cause.
     * Message.java (Message): Add constructor that takes a possible
     exception cause.
     (getException): New method.
     * PlocationCommand.java (interpret): Add possible RuntimeException
     cause.

All tests pass before and after this patch (go hackers - all PASS!)

Cheers,

Mark

[-- Attachment #2: message_exception_cause.patch --]
[-- Type: text/x-patch, Size: 5430 bytes --]

diff --git a/frysk-core/frysk/hpd/CLI.java b/frysk-core/frysk/hpd/CLI.java
index 3c14b2e..d79e557 100644
--- a/frysk-core/frysk/hpd/CLI.java
+++ b/frysk-core/frysk/hpd/CLI.java
@@ -123,6 +123,7 @@ public class CLI {
 	    return topLevelCommand.complete(this, new Input(buffer), cursor,
 					    candidates);
 	} catch (RuntimeException e) {
+	    // XXX - FIXME - What if this is something fatal?
 	    return -1;
 	}
     }
@@ -142,7 +143,7 @@ public class CLI {
 	    outWriter.print("Attached to process ");
 	    outWriter.println(attached);
         } catch (InterruptedException ie) {
-            addMessage("Attach interrupted.", Message.TYPE_ERROR);
+            addMessage("Attach interrupted.", Message.TYPE_ERROR, ie);
             return;
         } finally {
             synchronized (this) {
@@ -253,19 +254,14 @@ public class CLI {
 		    topLevelCommand.interpret(this, command);
                 }
             }
-            catch (NullPointerException e) {
-                e.printStackTrace();
-                String msg = "";
-                if (e.getMessage() != null)
-                    msg = e.getMessage();
-
-                addMessage(msg, Message.TYPE_DBG_ERROR);
-            }
+            catch (InvalidCommandException ice) {
+                addMessage(ice.getMessage(), Message.TYPE_ERROR);
+	    }
             catch (RuntimeException e) {
-                String msg = "";
-                if (e.getMessage() != null)
-                    msg = e.getMessage();
-                addMessage(msg, Message.TYPE_ERROR);
+                String msg = e.getMessage();
+                if (msg == null || msg.equals(""))
+                    msg = e.toString();
+                addMessage(msg, Message.TYPE_DBG_ERROR, e);
             }
             flushMessages();
         }
@@ -280,6 +276,10 @@ public class CLI {
         addMessage(new Message(msg, type));
     }
 
+    void addMessage(String msg, int type, Throwable exc) {
+        addMessage(new Message(msg, type, exc));
+    }
+
     private void flushMessages() {
         for (Iterator iter = messages.iterator(); iter.hasNext();) {
             Message tempmsg = (Message) iter.next();
@@ -293,8 +293,12 @@ public class CLI {
             if (prefix != null)
                 outWriter.print(prefix);
             outWriter.println(tempmsg.getMessage());
+	    Throwable exc = tempmsg.getException();
+	    if (exc != null)
+		exc.printStackTrace(outWriter);
             iter.remove();
         }
+	outWriter.flush();
     }
 
     PTSet createSet(String set) {
diff --git a/frysk-core/frysk/hpd/EvalCommands.java b/frysk-core/frysk/hpd/EvalCommands.java
index 5c69032..d9679db 100644
--- a/frysk-core/frysk/hpd/EvalCommands.java
+++ b/frysk-core/frysk/hpd/EvalCommands.java
@@ -96,7 +96,10 @@ abstract class EvalCommands extends ParameterizedCommand {
 	    try {
 		result = cli.parseValue(task, expression, options.dumpTree);
 	    } catch (RuntimeException nnfe) {
-		cli.addMessage(nnfe.getMessage(), Message.TYPE_ERROR);
+		String msg = nnfe.getMessage();
+		if (msg == null || msg.equals(""))
+		    msg = nnfe.toString();
+		cli.addMessage(msg, Message.TYPE_ERROR, nnfe);
 		continue;
 	    }
 
diff --git a/frysk-core/frysk/hpd/Message.java b/frysk-core/frysk/hpd/Message.java
index 933305f..aeb4ed2 100644
--- a/frysk-core/frysk/hpd/Message.java
+++ b/frysk-core/frysk/hpd/Message.java
@@ -49,17 +49,39 @@ class Message
 	public static int TYPE_NORMAL = 3;
 	public static int TYPE_VERBOSE = 4;
 
-	String msg = null;
-	int type = 0;
+	private final String msg;
+	private final int type;
+        private final Throwable exc;
 
-	public Message (String msg, int type)
+        /**
+	 * Creates a new Message with the given message and type
+	 * and no exception.
+	 */
+        public Message (String msg, int type)
+        {
+	    this(msg, type, null);
+	}  
+        /**
+	 * Creates a new Message with the given message and type.
+	 * The message cannot be null or empty. The exception is optional
+	 * and may be null.
+	 */
+        public Message (String msg, int type, Throwable exc)
 	{
+	        if (msg == null)
+		    throw new NullPointerException("null msg");
+
+	        if (msg.equals(""))
+		    throw new IllegalArgumentException("empty msg");
+
 		this.msg = msg;
 
 		if (type < TYPE_DBG_ERROR || type > TYPE_VERBOSE)
 			throw new IllegalArgumentException("Debugger message created with illegal type.");
 		else
 			this.type = type;
+
+		this.exc = exc;
 	}
 
 	public String getMessage()
@@ -71,4 +93,9 @@ class Message
 	{
 		return type;
 	}
+
+        public Throwable getException()
+        {
+	        return exc;
+        }
 }
diff --git a/frysk-core/frysk/hpd/PlocationCommand.java b/frysk-core/frysk/hpd/PlocationCommand.java
index 225cc09..61ced31 100644
--- a/frysk-core/frysk/hpd/PlocationCommand.java
+++ b/frysk-core/frysk/hpd/PlocationCommand.java
@@ -77,7 +77,10 @@ class PlocationCommand extends ParameterizedCommand {
             try {
                 result = cli.parseValue(task, sInput);	  
             } catch (RuntimeException nnfe) {
-                cli.addMessage(nnfe.getMessage(), Message.TYPE_ERROR);
+		String msg = nnfe.getMessage();
+		if (msg == null || msg.equals(""))
+		    msg = nnfe.toString();
+                cli.addMessage(msg, Message.TYPE_ERROR, nnfe);
                 continue;
             }
 	    result.getLocation().toPrint(cli.outWriter);

Re: fhpd vs RuntimeExceptions

[Andrew Cagney]

Mark Wielaard wrote:
> Hi,
>
> While investigating some bugs I noticed that the fhpd sometimes swallows
> the RuntimeExceptions from the core (and there I thought all was
> fine...).
Just fyi, broadly the message stuff, at least for normal output, is 
going away.

The reason is that the cli alternates between using addMessage and 
PrintWriter.print(...) for normal out; so I've been migrating stuff to 
just do PrintWriter.print.  But this leaves us still needing a way to 
consistently report errors.
>  This makes debugging the fhpd itself a little hard. I added a
> possible exception cause to the Message class and while I was at it
> added checks to make sure we don't present the user with an empty or
> null message (which is very uninformative). Currently we always print
> these possible exception causes when they are attached to a Message in
> CLI.flushMessages().

We were printing both the error and the stack, that looked terrible (the 
number of times an exception occurs for legitimated reasons is 
surprising); so they were turned off.  Did this turn them back on?

>  But we could add a fhpd 'frysk_debug mode' so they
> are only printed if an 'expert' is running frysk. For now I assume all
> users are experts and want to see them, but maybe it is annoying.
> Opinions?
>
> 2007-11-14  Mark Wielaard  <mwielaard@redhat.com>
>     
>      * CLI.java (addMessage): New variant that takes a possible
>      exception cause.
>      (doAttach): Use new addMessage().
>      (execCommand): Likewise.
>      (flushMessages): Add possible exception cause if present.
>      Actually flush outWriter.
>      * EvalCommands.java (eval): Add possible RuntimeException cause.
>      * Message.java (Message): Add constructor that takes a possible
>      exception cause.
>      (getException): New method.
>      * PlocationCommand.java (interpret): Add possible RuntimeException
>      cause.
>
> All tests pass before and after this patch (go hackers - all PASS!)
>
> Cheers,
>
> Mark
>   
> ------------------------------------------------------------------------
>
> diff --git a/frysk-core/frysk/hpd/CLI.java b/frysk-core/frysk/hpd/CLI.java
> index 3c14b2e..d79e557 100644
> --- a/frysk-core/frysk/hpd/CLI.java
> +++ b/frysk-core/frysk/hpd/CLI.java
> @@ -123,6 +123,7 @@ public class CLI {
>  	    return topLevelCommand.complete(this, new Input(buffer), cursor,
>  					    candidates);
>  	} catch (RuntimeException e) {
> +	    // XXX - FIXME - What if this is something fatal?
>  	    return -1;
>  	}
>      }
> @@ -142,7 +143,7 @@ public class CLI {
>  	    outWriter.print("Attached to process ");
>  	    outWriter.println(attached);
>          } catch (InterruptedException ie) {
> -            addMessage("Attach interrupted.", Message.TYPE_ERROR);
> +            addMessage("Attach interrupted.", Message.TYPE_ERROR, ie);
>              return;
>          } finally {
>              synchronized (this) {
> @@ -253,19 +254,14 @@ public class CLI {
>  		    topLevelCommand.interpret(this, command);
>                  }
>              }
> -            catch (NullPointerException e) {
> -                e.printStackTrace();
> -                String msg = "";
> -                if (e.getMessage() != null)
> -                    msg = e.getMessage();
> -
> -                addMessage(msg, Message.TYPE_DBG_ERROR);
> -            }
> +            catch (InvalidCommandException ice) {
> +                addMessage(ice.getMessage(), Message.TYPE_ERROR);
> +	    }
>              catch (RuntimeException e) {
> -                String msg = "";
> -                if (e.getMessage() != null)
> -                    msg = e.getMessage();
> -                addMessage(msg, Message.TYPE_ERROR);
> +                String msg = e.getMessage();
> +                if (msg == null || msg.equals(""))
> +                    msg = e.toString();
> +                addMessage(msg, Message.TYPE_DBG_ERROR, e);
>              }
>              flushMessages();
>          }
> @@ -280,6 +276,10 @@ public class CLI {
>          addMessage(new Message(msg, type));
>      }
>  
> +    void addMessage(String msg, int type, Throwable exc) {
> +        addMessage(new Message(msg, type, exc));
> +    }
> +
>      private void flushMessages() {
>          for (Iterator iter = messages.iterator(); iter.hasNext();) {
>              Message tempmsg = (Message) iter.next();
> @@ -293,8 +293,12 @@ public class CLI {
>              if (prefix != null)
>                  outWriter.print(prefix);
>              outWriter.println(tempmsg.getMessage());
> +	    Throwable exc = tempmsg.getException();
> +	    if (exc != null)
> +		exc.printStackTrace(outWriter);
>              iter.remove();
>          }
> +	outWriter.flush();
>      }
>  
>      PTSet createSet(String set) {
> diff --git a/frysk-core/frysk/hpd/EvalCommands.java b/frysk-core/frysk/hpd/EvalCommands.java
> index 5c69032..d9679db 100644
> --- a/frysk-core/frysk/hpd/EvalCommands.java
> +++ b/frysk-core/frysk/hpd/EvalCommands.java
> @@ -96,7 +96,10 @@ abstract class EvalCommands extends ParameterizedCommand {
>  	    try {
>  		result = cli.parseValue(task, expression, options.dumpTree);
>  	    } catch (RuntimeException nnfe) {
> -		cli.addMessage(nnfe.getMessage(), Message.TYPE_ERROR);
> +		String msg = nnfe.getMessage();
> +		if (msg == null || msg.equals(""))
> +		    msg = nnfe.toString();
> +		cli.addMessage(msg, Message.TYPE_ERROR, nnfe);
>  		continue;
>  	    }
>  
> diff --git a/frysk-core/frysk/hpd/Message.java b/frysk-core/frysk/hpd/Message.java
> index 933305f..aeb4ed2 100644
> --- a/frysk-core/frysk/hpd/Message.java
> +++ b/frysk-core/frysk/hpd/Message.java
> @@ -49,17 +49,39 @@ class Message
>  	public static int TYPE_NORMAL = 3;
>  	public static int TYPE_VERBOSE = 4;
>  
> -	String msg = null;
> -	int type = 0;
> +	private final String msg;
> +	private final int type;
> +        private final Throwable exc;
>  
> -	public Message (String msg, int type)
> +        /**
> +	 * Creates a new Message with the given message and type
> +	 * and no exception.
> +	 */
> +        public Message (String msg, int type)
> +        {
> +	    this(msg, type, null);
> +	}  
> +        /**
> +	 * Creates a new Message with the given message and type.
> +	 * The message cannot be null or empty. The exception is optional
> +	 * and may be null.
> +	 */
> +        public Message (String msg, int type, Throwable exc)
>  	{
> +	        if (msg == null)
> +		    throw new NullPointerException("null msg");
> +
> +	        if (msg.equals(""))
> +		    throw new IllegalArgumentException("empty msg");
> +
>  		this.msg = msg;
>  
>  		if (type < TYPE_DBG_ERROR || type > TYPE_VERBOSE)
>  			throw new IllegalArgumentException("Debugger message created with illegal type.");
>  		else
>  			this.type = type;
> +
> +		this.exc = exc;
>  	}
>  
>  	public String getMessage()
> @@ -71,4 +93,9 @@ class Message
>  	{
>  		return type;
>  	}
> +
> +        public Throwable getException()
> +        {
> +	        return exc;
> +        }
>  }
> diff --git a/frysk-core/frysk/hpd/PlocationCommand.java b/frysk-core/frysk/hpd/PlocationCommand.java
> index 225cc09..61ced31 100644
> --- a/frysk-core/frysk/hpd/PlocationCommand.java
> +++ b/frysk-core/frysk/hpd/PlocationCommand.java
> @@ -77,7 +77,10 @@ class PlocationCommand extends ParameterizedCommand {
>              try {
>                  result = cli.parseValue(task, sInput);	  
>              } catch (RuntimeException nnfe) {
> -                cli.addMessage(nnfe.getMessage(), Message.TYPE_ERROR);
> +		String msg = nnfe.getMessage();
> +		if (msg == null || msg.equals(""))
> +		    msg = nnfe.toString();
> +                cli.addMessage(msg, Message.TYPE_ERROR, nnfe);
>                  continue;
>              }
>  	    result.getLocation().toPrint(cli.outWriter);
>   

Re: fhpd vs RuntimeExceptions

[Kris Van Hees]

On Wed, Nov 14, 2007 at 09:44:08AM -0500, Andrew Cagney wrote:
> Mark Wielaard wrote:
>>  This makes debugging the fhpd itself a little hard. I added a
>> possible exception cause to the Message class and while I was at it
>> added checks to make sure we don't present the user with an empty or
>> null message (which is very uninformative). Currently we always print
>> these possible exception causes when they are attached to a Message in
>> CLI.flushMessages().
>
> We were printing both the error and the stack, that looked terrible (the 
> number of times an exception occurs for legitimated reasons is surprising); 
> so they were turned off.  Did this turn them back on?

While exceptions definitely are expected to occur primarily for legitimate
reasons, exceptions should hardly ever be simply printed (along with the stack)
because that is pretty much equivalent to not handling it.  Some might be
very simple cases of needing to write out a message indicating the exception
or error, but that is usually more exception than rule.  Turning it off is
not quite the answer either I think.  Leaving it in the state where it spews
out excessive information might be better along with bugs filed for the pieces
of code that are failing to handle the exceptions appropriately.  Otherwise
we are simply hiding problems, and making debugging fhpd a lot more difficult
as Mark points out.

I've found it quite convenient in various projects that deal with this type of
issues (Java-based and otherwise) to rather ensure this output goes to stderr
so it can be redirected to a file for later examination, as to not interfere
with the interactive session.  And when specifically debugging some of those
issues, not redirecting so they show up inline where they make most sense.  Any
form of stopping them from being printed has always led to somehow delaying the
resolution of the issues that cause the problem in the first place.

	Cheers,
	Kris

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

We quickly went over this on the meeting just now.
Just a summary to see if I got it all.

On Wed, 2007-11-14 at 09:44 -0500, Andrew Cagney wrote:
> Mark Wielaard wrote:
> > While investigating some bugs I noticed that the fhpd sometimes swallows
> > the RuntimeExceptions from the core (and there I thought all was
> > fine...).
> Just fyi, broadly the message stuff, at least for normal output, is 
> going away.
>
> The reason is that the cli alternates between using addMessage and 
> PrintWriter.print(...) for normal out; so I've been migrating stuff to 
> just do PrintWriter.print.  But this leaves us still needing a way to 
> consistently report errors.

OK, good to know, I had only seen the Message variants in the code that
I looked at. The (add)Message stuff had one benefit that it concentrated
the generation of Messages so you can easily capture any exception
causes, which may patch added. When using "raw" Printwriter calls you
would need some way to capture and then report the errors indeed.

> >  This makes debugging the fhpd itself a little hard. I added a
> > possible exception cause to the Message class and while I was at it
> > added checks to make sure we don't present the user with an empty or
> > null message (which is very uninformative). Currently we always print
> > these possible exception causes when they are attached to a Message in
> > CLI.flushMessages().
> 
> We were printing both the error and the stack, that looked terrible (the 
> number of times an exception occurs for legitimated reasons is 
> surprising); so they were turned off.  Did this turn them back on?

The cases I looked at were things like:
http://sourceware.org/bugzilla/show_bug.cgi?id=5286
http://sourceware.org/bugzilla/show_bug.cgi?id=5267
Where there was an internal RuntimeException without a message so you
would just see Error: null or Error: "" without any extra info.

In those cases when you have an internal RuntimeException you now attach
a exception cause to the message (and currently always print it, but
that can certainly be made optional - either with a command line option
to fhpd when started up or by setting some internal variable - see help
set).

The main problem seems to be how to categorize RuntimeExceptions.
Currently we are mixing internal ones, that should never occur and when
they bubble up to the fhpd CLI level should really be reported and
"expected" RuntimeExceptions that "mean" something at the fphd level and
for which only the message might make sense.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Andrew Cagney]

Mark,

from the call; what about:
  CLI.printError(String)
and/or/...
  CLI.printError(Exception)
the "logic" deciding what to do with the exception; for instance if 
Exception.getMessage() is null/empty then things are bad; dump the 
back-trace; but otherwise just print the message (That should cover null 
pointer exceptions).

Andrew

Mark Wielaard wrote:
> We quickly went over this on the meeting just now.
> Just a summary to see if I got it all.
>
> On Wed, 2007-11-14 at 09:44 -0500, Andrew Cagney wrote:
>   
>> Mark Wielaard wrote:
>>     
>>> While investigating some bugs I noticed that the fhpd sometimes swallows
>>> the RuntimeExceptions from the core (and there I thought all was
>>> fine...).
>>>       
>> Just fyi, broadly the message stuff, at least for normal output, is 
>> going away.
>>
>> The reason is that the cli alternates between using addMessage and 
>> PrintWriter.print(...) for normal out; so I've been migrating stuff to 
>> just do PrintWriter.print.  But this leaves us still needing a way to 
>> consistently report errors.
>>     
>
> OK, good to know, I had only seen the Message variants in the code that
> I looked at. The (add)Message stuff had one benefit that it concentrated
> the generation of Messages so you can easily capture any exception
> causes, which may patch added. When using "raw" Printwriter calls you
> would need some way to capture and then report the errors indeed.
>
>   
>>>  This makes debugging the fhpd itself a little hard. I added a
>>> possible exception cause to the Message class and while I was at it
>>> added checks to make sure we don't present the user with an empty or
>>> null message (which is very uninformative). Currently we always print
>>> these possible exception causes when they are attached to a Message in
>>> CLI.flushMessages().
>>>       
>> We were printing both the error and the stack, that looked terrible (the 
>> number of times an exception occurs for legitimated reasons is 
>> surprising); so they were turned off.  Did this turn them back on?
>>     
>
> The cases I looked at were things like:
> http://sourceware.org/bugzilla/show_bug.cgi?id=5286
> http://sourceware.org/bugzilla/show_bug.cgi?id=5267
> Where there was an internal RuntimeException without a message so you
> would just see Error: null or Error: "" without any extra info.
>
> In those cases when you have an internal RuntimeException you now attach
> a exception cause to the message (and currently always print it, but
> that can certainly be made optional - either with a command line option
> to fhpd when started up or by setting some internal variable - see help
> set).
>
> The main problem seems to be how to categorize RuntimeExceptions.
> Currently we are mixing internal ones, that should never occur and when
> they bubble up to the fhpd CLI level should really be reported and
> "expected" RuntimeExceptions that "mean" something at the fphd level and
> for which only the message might make sense.
>
> Cheers,
>
> Mark
>
>   

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Andrew,

On Wed, 2007-11-14 at 12:31 -0500, Andrew Cagney wrote:
> from the call; what about:
>   CLI.printError(String)
> and/or/...
>   CLI.printError(Exception)

You mean to replace CLI.addMessage() and do the user feedback directly
instead of stacking it in a Message queue and then replaying it? Yes,
that makes sense. The current setup does feel a little over-engineered.

[There is the issue that you don't have a strong connection to the
original Command executed by the user for which this is a response, but
we didn't have that in the case of the (add)Message constructs in the
first place, so that might not matter at this point. Since from the user
point of view the fhpd is single threaded and accepts just one command
at a time. It might matter in the future for the gui though since that
could be issuing multiple commands at the same time and then you want to
have a stronger tie between the issued command and the user response or
error message.]

> the "logic" deciding what to do with the exception; for instance if 
> Exception.getMessage() is null/empty then things are bad; dump the 
> back-trace; but otherwise just print the message (That should cover null 
> pointer exceptions).

I don't think that is the right logic. There are also other
RuntimeExceptions like ArrayOutOfBoundsException, ArithmeticException,
IllegalArgumentException or NumberFormatException, etc. that don't have
empty messages, but that are real core bugs if they occur and "bubble
up" to the CLI. Just printing the message isn't enough information for
the user to see what really went wrong.

If the Exception is truly meant to convey information to the end user in
relation to a specific command it should be more explicitly communicated
than just by any generic RuntimeException.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Phil Muldoon]

Mark Wielaard wrote:
> Hi,
>
> While investigating some bugs I noticed that the fhpd sometimes swallows
> the RuntimeExceptions from the core (and there I thought all was
> fine...). This makes debugging the fhpd itself a little hard. I added a
> possible exception cause to the Message class and while I was at it
> added checks to make sure we don't present the user with an empty or
> null message (which is very uninformative). Currently we always print
> these possible exception causes when they are attached to a Message in
> CLI.flushMessages(). But we could add a fhpd 'frysk_debug mode' so they
> are only printed if an 'expert' is running frysk. For now I assume all
> users are experts and want to see them, but maybe it is annoying.
> Opinions?
>   
Pending a the full implementation of this it's a pain to see every 
single exception printed. As talked about on IRC over the corefile 
message design, exceptions can and are used to carry warnings, messages 
and so on. How do you differentiate between a warning and an error in 
this case?

Take this example. I manufactured this warning to happen, but all the 
message is telling the user is that it cannot read at the peek() address 
given. It's not an error, just a cannot do. But the huge ugly backtrace 
that follows is not very useful.

(fhpd) core /home/pmuldoon/core.12463 /bin/bash
Internal debugger error:  peek() at address 6992f8 cannot be found in 
metadata table.
java.lang.RuntimeException: peek() at address 6992f8 cannot be found in 
metadata table.
   at frysk.proc.dead.CorefileByteBuffer.peek(fhpd)
   at inua.eio.ByteBuffer.peek(fhpd)
   at inua.eio.ByteBuffer.peekFully(fhpd)
   at inua.eio.ByteBuffer.peekLittle(fhpd)
   at inua.eio.ByteBuffer.peekLittle(fhpd)
   at inua.eio.ByteOrdered$2.peekULong(fhpd)
   at inua.eio.ByteBuffer.getULong(fhpd)
   at inua.eio.WordSized$3.getUWord(fhpd)
   at inua.eio.ByteBuffer.getUWord(fhpd)
   at frysk.proc.dead.LinuxProc.sendrecMaps(fhpd)
   at frysk.proc.Proc.getMaps(fhpd)
   at frysk.dwfl.DwflFactory.updateDwfl(fhpd)
   at frysk.dwfl.DwflCache.getDwfl(fhpd)
   at frysk.debuginfo.DebugInfoFrame.getScopes(fhpd)
   at frysk.debuginfo.DebugInfoStackFactory.createVirtualStackTrace(fhpd)
   at frysk.hpd.CoreCommand.interpret(fhpd)
   at frysk.hpd.ParameterizedCommand.interpret(fhpd)
   at frysk.hpd.MultiLevelCommand.interpret(fhpd)
   at frysk.hpd.CLI.execCommand(fhpd)
   at frysk.bindir.fhpd.main(fhpd)

If this is the way forward, I'll have to gobble exceptions locally in 
CoreCommand, and just deal with them locally. I kind of liked the way 
that fhpd would deal with them, and rank severity on Runtime, or NPE, or 
whatever. This relieved the subcommands writing their own mandatory 
handlers. (right now you can still do that as an option).

Regards

Phil

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Phil,

On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
> As talked about on IRC over the corefile 
> message design, exceptions can and are used to carry warnings, messages 
> and so on. How do you differentiate between a warning and an error in 
> this case?

By using different exception types, so a higher level can distinquish
between a "recoverable" warning and a "unrecoverable" error.

> Take this example. I manufactured this warning to happen, but all the 
> message is telling the user is that it cannot read at the peek() address 
> given. It's not an error, just a cannot do. But the huge ugly backtrace 
> that follows is not very useful.
> 
> (fhpd) core /home/pmuldoon/core.12463 /bin/bash
> Internal debugger error:  peek() at address 6992f8 cannot be found in 
> metadata table.
> java.lang.RuntimeException: peek() at address 6992f8 cannot be found in 
> metadata table.
>    at frysk.proc.dead.CorefileByteBuffer.peek(fhpd)
>    at inua.eio.ByteBuffer.peek(fhpd)
>    at inua.eio.ByteBuffer.peekFully(fhpd)
>    at inua.eio.ByteBuffer.peekLittle(fhpd)
>    at inua.eio.ByteBuffer.peekLittle(fhpd)
>    at inua.eio.ByteOrdered$2.peekULong(fhpd)
>    at inua.eio.ByteBuffer.getULong(fhpd)
>    at inua.eio.WordSized$3.getUWord(fhpd)
>    at inua.eio.ByteBuffer.getUWord(fhpd)
>    at frysk.proc.dead.LinuxProc.sendrecMaps(fhpd)
>    at frysk.proc.Proc.getMaps(fhpd)
>    at frysk.dwfl.DwflFactory.updateDwfl(fhpd)
>    at frysk.dwfl.DwflCache.getDwfl(fhpd)
>    at frysk.debuginfo.DebugInfoFrame.getScopes(fhpd)
>    at frysk.debuginfo.DebugInfoStackFactory.createVirtualStackTrace(fhpd)
>    at frysk.hpd.CoreCommand.interpret(fhpd)
>    at frysk.hpd.ParameterizedCommand.interpret(fhpd)
>    at frysk.hpd.MultiLevelCommand.interpret(fhpd)
>    at frysk.hpd.CLI.execCommand(fhpd)
>    at frysk.bindir.fhpd.main(fhpd)
> 
> If this is the way forward, I'll have to gobble exceptions locally in 
> CoreCommand, and just deal with them locally.

Yes, I think that is the way forward. Something terribly failed. And
just passing the "address 6992f8 cannot be found in metadata table."
message to the user is clearly not very helpful if the user just wanted
to run a specific command. Only the command knows if this is something
fatal or not and should catch it at the appropriate level and report
what the exact action was that was attempted and which structure
couldn't be created because of the error.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Forgot:

On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
> Pending a the full implementation of this it's a pain to see every 
> single exception printed.

Till there is a difference between fatal and user message "exceptions"
you can easily get the old behavior by tweaking CLI.flushMessages():

--- a/frysk-core/frysk/hpd/CLI.java
+++ b/frysk-core/frysk/hpd/CLI.java
@@ -294,7 +294,7 @@ public class CLI {
                 outWriter.print(prefix);
             outWriter.println(tempmsg.getMessage());
            Throwable exc = tempmsg.getException();
-           if (exc != null)
+           if (exc != null && false)
                exc.printStackTrace(outWriter);
             iter.remove();
         }

Or replace false with the heuristic you find acceptable of course.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Phil Muldoon]

Mark Wielaard wrote:
> Hi Phil,
>
> On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
>   
>> As talked about on IRC over the corefile 
>> message design, exceptions can and are used to carry warnings, messages 
>> and so on. How do you differentiate between a warning and an error in 
>> this case?
>>     
>
> By using different exception types, so a higher level can distinquish
> between a "recoverable" warning and a "unrecoverable" error.
>   

Like I mentioned in reply to Sami's email yesterday, having a napi throw 
several different unchecked exception types places a huge and unfair 
burden on the user to know the code beyond the api. The places "must be 
an expert on Frysk to call Frysk apis" charge at our feet.

> Yes, I think that is the way forward. Something terribly failed. And
> just passing the "address 6992f8 cannot be found in metadata table."
> message to the user is clearly not very helpful if the user just wanted
> to run a specific command. 

What would be helpful here? It's a message, not an error? Basically it 
just means cannot read the address here as we don't know how to read it. 
I can format the  message however I want, but it is still not an error 
message. However it sure looks like one now ;)

> Only the command knows if this is something
> fatal or not and should catch it at the appropriate level and report
> what the exact action was that was attempted and which structure
> couldn't be created because of the error.
>   

No in the my case. Corecommand is just there to bootstrap a corefile 
into fhpd and report back any messages and/or errors it receives.


Regards

Phil

Re: fhpd vs RuntimeExceptions

[Sami Wagiaalla]

Mark Wielaard wrote:
> Hi Phil,
>
> On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
>   
>> As talked about on IRC over the corefile 
>> message design, exceptions can and are used to carry warnings, messages 
>> and so on. How do you differentiate between a warning and an error in 
>> this case?
>>     
>
> By using different exception types, so a higher level can distinquish
> between a "recoverable" warning and a "unrecoverable" error.
>   
My suggestion would be to create a MetaDataTablePeekFailedException. 
Recoverable or unrecoverable is not something that the thrower can 
decide, it is the decision of the catcher. For example 
DwAttributeNotFoundException can be used to conclude something about a 
die's type. But in another case it can mean that your code is looking 
for a bogus attribute or is looking at the wrong die. If your code knows 
why the exception is thrown then by all means handle it, print a clean 
stack trace free error/warning message or nothing at all. Otherwise let 
it float up.

Re: fhpd vs RuntimeExceptions

[Phil Muldoon]

Mark Wielaard wrote:
> Forgot:
>
> On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
>   
>> Pending a the full implementation of this it's a pain to see every 
>> single exception printed.
>>     
>
> Till there is a difference between fatal and user message "exceptions"
> you can easily get the old behavior by tweaking CLI.flushMessages():
>
> --- a/frysk-core/frysk/hpd/CLI.java
> +++ b/frysk-core/frysk/hpd/CLI.java
> @@ -294,7 +294,7 @@ public class CLI {
>                  outWriter.print(prefix);
>              outWriter.println(tempmsg.getMessage());
>             Throwable exc = tempmsg.getException();
> -           if (exc != null)
> +           if (exc != null && false)
>                 exc.printStackTrace(outWriter);
>              iter.remove();
>          }
>
> Or replace false with the heuristic you find acceptable of course.
>   

I'm not advocating any such course. I think we need to let the thread 
move on a bit, have a few more conversations on it, then make a 
decision. It's good to have this conversation, personally, as I learn 
from these debates. I know much more about fhpd now than last week. In 
short, nothing too hasty ;)

Regards

Phil

Re: fhpd vs RuntimeExceptions

[Sami Wagiaalla]

Phil Muldoon wrote:
> Mark Wielaard wrote:
>> Hi Phil,
>>
>> On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
>>  
>>> As talked about on IRC over the corefile message design, exceptions 
>>> can and are used to carry warnings, messages and so on. How do you 
>>> differentiate between a warning and an error in this case?
>>>     
>>
>> By using different exception types, so a higher level can distinquish
>> between a "recoverable" warning and a "unrecoverable" error.
>>   
>
> Like I mentioned in reply to Sami's email yesterday, having a napi 
> throw several different unchecked exception types places a huge and 
> unfair burden on the user to know the code beyond the api. The places 
> "must be an expert on Frysk to call Frysk apis" charge at our feet.
The user doent have to know about any exceptions that are thrown and 
caught within the library (ie exceptions used for comunication). But if 
an exception bubbles up to that user, and we are following the system 
described above then that is infact an error and that user must have 
done something bad and should be notified in very clear manner.

Re: fhpd vs RuntimeExceptions

[Andrew Cagney]

Mark Wielaard wrote:
>
> By using different exception types, so a higher level can distinquish
> between a "recoverable" warning and a "unrecoverable" error.
>
>   
>> Take this example. I manufactured this warning to happen, but all the 
>> message is telling the user is that it cannot read at the peek() address 
>> given. It's not an error, just a cannot do. But the huge ugly backtrace 
>> that follows is not very useful.
>>
>> (fhpd) core /home/pmuldoon/core.12463 /bin/bash
>> Internal debugger error:  peek() at address 6992f8 cannot be found in 
>> metadata table.
>> java.lang.RuntimeException: peek() at address 6992f8 cannot be found in 
>> metadata table.
>>    at frysk.proc.dead.CorefileByteBuffer.peek(fhpd)
>>    at inua.eio.ByteBuffer.peek(fhpd)
>>    at inua.eio.ByteBuffer.peekFully(fhpd)
>>    at inua.eio.ByteBuffer.peekLittle(fhpd)
>>    at inua.eio.ByteBuffer.peekLittle(fhpd)
>>    at inua.eio.ByteOrdered$2.peekULong(fhpd)
>>    at inua.eio.ByteBuffer.getULong(fhpd)
>>    at inua.eio.WordSized$3.getUWord(fhpd)
>>    at inua.eio.ByteBuffer.getUWord(fhpd)
>>    at frysk.proc.dead.LinuxProc.sendrecMaps(fhpd)
>>    at frysk.proc.Proc.getMaps(fhpd)
>>    at frysk.dwfl.DwflFactory.updateDwfl(fhpd)
>>    at frysk.dwfl.DwflCache.getDwfl(fhpd)
>>    at frysk.debuginfo.DebugInfoFrame.getScopes(fhpd)
>>    at frysk.debuginfo.DebugInfoStackFactory.createVirtualStackTrace(fhpd)
>>    at frysk.hpd.CoreCommand.interpret(fhpd)
>>    at frysk.hpd.ParameterizedCommand.interpret(fhpd)
>>    at frysk.hpd.MultiLevelCommand.interpret(fhpd)
>>    at frysk.hpd.CLI.execCommand(fhpd)
>>    at frysk.bindir.fhpd.main(fhpd)
>>
>> If this is the way forward, I'll have to gobble exceptions locally in 
>> CoreCommand, and just deal with them locally.
>>     
>
> Yes, I think that is the way forward.

Er, no.  We're not going to make all commands gobble all exceptions 
locally.  It's a loosing battle.  And we've far more important battles 
to be focused on right now.


>  Something terribly failed. And
> just passing the "address 6992f8 cannot be found in metadata table."
> message to the user is clearly not very helpful if the user just wanted
> to run a specific command. Only the command knows if this is something
> fatal or not and should catch it at the appropriate level and report
> what the exact action was that was attempted and which structure
> couldn't be created because of the error.
>
> Cheers,
>
> Mark
>
>   

Re: fhpd vs RuntimeExceptions

[Andrew Cagney]

Mark Wielaard wrote:
> Forgot:
>
> On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
>   
>> Pending a the full implementation of this it's a pain to see every 
>> single exception printed.
>>     
>
> Till there is a difference between fatal and user message "exceptions"
> you can easily get the old behavior by tweaking CLI.flushMessages():
>
>   
That unfortunatly isn't sufficient; the old code in CLI.java was 
differentiating between and NPE and other exceptions - dumping the stack 
when an NPE occured.  I'll restore this behavior; so we're at least back 
to a usable status quo (and from a HPD user prospective in a better 
position - these back-traces plain suck)

I'll also add a prototype version of CLI.printError() that we discussed; 
and have just EvalCommands use it; we can then refine it as needed.

Andrew

> --- a/frysk-core/frysk/hpd/CLI.java
> +++ b/frysk-core/frysk/hpd/CLI.java
> @@ -294,7 +294,7 @@ public class CLI {
>                  outWriter.print(prefix);
>              outWriter.println(tempmsg.getMessage());
>             Throwable exc = tempmsg.getException();
> -           if (exc != null)
> +           if (exc != null && false)
>                 exc.printStackTrace(outWriter);
>              iter.remove();
>          }
>
> Or replace false with the heuristic you find acceptable of course.
>
> Cheers,
>
> Mark
>
>   

Re: fhpd vs RuntimeExceptions

[Kris Van Hees]

On Thu, Nov 15, 2007 at 01:20:56PM -0500, Sami Wagiaalla wrote:
> Mark Wielaard wrote:
>> By using different exception types, so a higher level can distinquish
>> between a "recoverable" warning and a "unrecoverable" error.
>>   
> My suggestion would be to create a MetaDataTablePeekFailedException. 
> Recoverable or unrecoverable is not something that the thrower can decide, 
> it is the decision of the catcher. For example DwAttributeNotFoundException 
> can be used to conclude something about a die's type. But in another case 
> it can mean that your code is looking for a bogus attribute or is looking 
> at the wrong die. If your code knows why the exception is thrown then by 
> all means handle it, print a clean stack trace free error/warning message 
> or nothing at all. Otherwise let it float up.

Well, yes and no.  From the perspective of the thrower there can definitely be
a determination on whether a problem is persistent or temporary, i.e. whether
the operation can be retried.  Likewise, the thrower may be able to indicate in
the exception whether there may be an alternative resolution (but leaving the
decision on whether that is used to the caller).  That's a less common way to
use exceptions, but it definitely has its uses.

The caller can have its own decision logic on how to handle exceptions.  E.g.
it is very well possible that the thrower considers a problem persistent, while
the caller knows some alternatives to try.  Looking for a file in different
locations is a typical example of that.  The thrower will typically indicate
that the given file (specified with an absolute path) cannot be accessed, while
the caller may have a couple of directories left to try.

One common problem with exceptions that I keep running into with projects is
overuse of letting exceptions float up until someone handles it.  More often
than not, exceptions end up floating across component boundaries.  In those
cases, it might be more appropriate to catch the exception, and throw a new
one (using the caught one as reason) to indicate the exception status on the
level of the current component.

	Cheers,
	Kris

Re: fhpd vs RuntimeExceptions

[Kris Van Hees]

On Thu, Nov 15, 2007 at 06:41:56PM +0100, Mark Wielaard wrote:
> Hi Phil,
> > << Example exception output deleted - see original message for details. >>
> > If this is the way forward, I'll have to gobble exceptions locally in 
> > CoreCommand, and just deal with them locally.
> 
> Yes, I think that is the way forward. Something terribly failed. And
> just passing the "address 6992f8 cannot be found in metadata table."
> message to the user is clearly not very helpful if the user just wanted
> to run a specific command. Only the command knows if this is something
> fatal or not and should catch it at the appropriate level and report
> what the exact action was that was attempted and which structure
> couldn't be created because of the error.

I do not think that an exception like this should ever get handled at the
level of the command module.  In fact, in a case like this it should never
have floated up that high without getting handled.  Clearly, being unable to
peek at the given address was the result of trying to do some action.  The
failure to peek at the address either causes that action to fail, or it can be
ignored (or otherwise recovered from).  Either way, the command module should
not be exposed to this level of detail (though the information may need to be
retained as a nested exception for more verbose output, etc...).

This is similar to the famous NullPointerException.  I'd expect we all agree
that one of those should never ever float up all the way to the fhpd.

	Cheers,
	Kris

Re: fhpd vs RuntimeExceptions

[Phil Muldoon]

Kris Van Hees wrote:
> On Thu, Nov 15, 2007 at 06:41:56PM +0100, Mark Wielaard wrote:
>   
>> Hi Phil,
>>     
>>> << Example exception output deleted - see original message for details. >>
>>> If this is the way forward, I'll have to gobble exceptions locally in 
>>> CoreCommand, and just deal with them locally.
>>>       
>> Yes, I think that is the way forward. Something terribly failed. And
>> just passing the "address 6992f8 cannot be found in metadata table."
>> message to the user is clearly not very helpful if the user just wanted
>> to run a specific command. Only the command knows if this is something
>> fatal or not and should catch it at the appropriate level and report
>> what the exact action was that was attempted and which structure
>> couldn't be created because of the error.
>>     
>
> I do not think that an exception like this should ever get handled at the
> level of the command module.  In fact, in a case like this it should never
> have floated up that high without getting handled.  Clearly, being unable to
> peek at the given address was the result of trying to do some action.  The
> failure to peek at the address either causes that action to fail, or it can be
> ignored (or otherwise recovered from).  Either way, the command module should
> not be exposed to this level of detail (though the information may need to be
> retained as a nested exception for more verbose output, etc...).
>   

Well yes and no. Perhaps I agree with the overall assertions but the 
corefile user should always recognizes that a corefile is created from a 
dead or dying process (or just a snapshot in some cases). Let's assume 
the former for now. The big use-case around core-files is that 
programmers use them to debug a process that died some time ago. That 
process might have been in any kind of corrupted state. Therefore a read 
at an address that is expected to be sane, but is not, should absolutely 
be reported. But is it an error in the core file code itself? No. It 
duly tried to do as asked, but failed.  It cannot diagnose why that 
failure occurred, just that it did. It can mean corrupt segments, 
missing linkmaps table, screwed up dynamic segment and a whole host of 
other issues the user (the programmer) should know about via this feedback.

I keep bringing this example up as this is my pain now; and perhaps it 
is a special case - but normally they are what define the boundaries of 
the rules. As it is right now, this is an exception as in "Cannot read 
where you want me to read", but it is not an error condition simply as 
it is just honestly reporting that it cannot do what one asked. Because 
a data read at 0x12345 address is invalid, it should not print a 
stacktrace to that effect. This is not an error in the corefile code, 
but the corefile itself. That's pretty much the crux of the argument, 
and separating out messaging from true errors.

Anyway, just wanted to give some background to that exception. I realize 
now I should have gone into more detail here earlier.

Regards

Phil

Re: fhpd vs RuntimeExceptions

[Kris Van Hees]

Phil,

Thanks for the more verbose explanation of your specific example.  Obviously,
all special cases will require some exception (no pun intended) to any rule
for handling exceptions.  Your example might end up being even more complex,
because some form of core file corruption could certainly result in a flood
of peek failures, making it impractical to report every one of them to the
user.  That would require more complex logic for dealing with exception
handling, to be smarter about how to report that situation.

My main point was (not knowing the full details of your specific example)
mainly that e.g. a failure to read data at a memory location on behalf of a
some task may very well be better reported as e.g. a failure to complete that
specific task (probably with reporting of the specific lower level problem as
a cause for the task failure).  I'd still argue that even in your specific
example, the peek failure should probably float up as nested exception in a
component specific exception.

If you can stand me just making things up as I go for a minute, maybe the
getMaps() method could throw a CannotLoadMapException, with something like a
CorefilePeekAccessException nested in it to indicate the lower level failure.
And the CannotLoadMapException may need to be nested inside a higher level
exception as well, to appropriately float up across component boundaries.

Beyond this, I actually favour the concept that projects should not use any of
the standard Java exceptions and instead should define its own.  Whenever I
worked on projects that enforced this, I did complain about what a pain it is
to define exception classes all over the place - but in the end it did actually
turn out to be quite worth the effort.

	Cheers,
	Kris

On Thu, Nov 15, 2007 at 10:11:22PM +0000, Phil Muldoon wrote:
> Kris Van Hees wrote:
>> I do not think that an exception like this should ever get handled at the
>> level of the command module.  In fact, in a case like this it should never
>> have floated up that high without getting handled.  Clearly, being unable 
>> to
>> peek at the given address was the result of trying to do some action.  The
>> failure to peek at the address either causes that action to fail, or it 
>> can be
>> ignored (or otherwise recovered from).  Either way, the command module 
>> should
>> not be exposed to this level of detail (though the information may need to 
>> be
>> retained as a nested exception for more verbose output, etc...).
>>   
>
> Well yes and no. Perhaps I agree with the overall assertions but the 
> corefile user should always recognizes that a corefile is created from a 
> dead or dying process (or just a snapshot in some cases). Let's assume the 
> former for now. The big use-case around core-files is that programmers use 
> them to debug a process that died some time ago. That process might have 
> been in any kind of corrupted state. Therefore a read at an address that is 
> expected to be sane, but is not, should absolutely be reported. But is it 
> an error in the core file code itself? No. It duly tried to do as asked, 
> but failed.  It cannot diagnose why that failure occurred, just that it 
> did. It can mean corrupt segments, missing linkmaps table, screwed up 
> dynamic segment and a whole host of other issues the user (the programmer) 
> should know about via this feedback.
>
> I keep bringing this example up as this is my pain now; and perhaps it is a 
> special case - but normally they are what define the boundaries of the 
> rules. As it is right now, this is an exception as in "Cannot read where 
> you want me to read", but it is not an error condition simply as it is just 
> honestly reporting that it cannot do what one asked. Because a data read at 
> 0x12345 address is invalid, it should not print a stacktrace to that 
> effect. This is not an error in the corefile code, but the corefile itself. 
> That's pretty much the crux of the argument, and separating out messaging 
> from true errors.
>
> Anyway, just wanted to give some background to that exception. I realize 
> now I should have gone into more detail here earlier.
>
> Regards
>
> Phil

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Sami,

On Thu, 2007-11-15 at 13:20 -0500, Sami Wagiaalla wrote:
> Mark Wielaard wrote:
> > On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
> >> As talked about on IRC over the corefile 
> >> message design, exceptions can and are used to carry warnings, messages 
> >> and so on. How do you differentiate between a warning and an error in 
> >> this case?
> >
> > By using different exception types, so a higher level can distinquish
> > between a "recoverable" warning and a "unrecoverable" error.
> >   
> My suggestion would be to create a MetaDataTablePeekFailedException. 
> Recoverable or unrecoverable is not something that the thrower can 
> decide, it is the decision of the catcher. For example 
> DwAttributeNotFoundException can be used to conclude something about a 
> die's type. But in another case it can mean that your code is looking 
> for a bogus attribute or is looking at the wrong die. If your code knows 
> why the exception is thrown then by all means handle it, print a clean 
> stack trace free error/warning message or nothing at all. Otherwise let 
> it float up.

Yes I agree, having explicit exception types would give the caller the
opportunity to decide how to handle the exception.

At the lowest level it would be good to communicate the distinction
between "cannot access data" (peek or poke of inferior memory or
register, which probably indicates a real error or "out of bounds"
access) and "invalid data" (or data structure in inferior/core is
inconsistent, so I am unable to reconstruct a specific table or type).

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Andrew,

On Thu, 2007-11-15 at 13:44 -0500, Andrew Cagney wrote:
> Er, no.  We're not going to make all commands gobble all exceptions 
> locally.

I didn't mean locally at the lowest command level. I meant at the
appropriate level in the core. Commands should never have to decide
locally if an exception is fatal or not. And they shouldn't need to
gobble up exceptions of course because they might contain important
information about what happened.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Andrew,

On Thu, 2007-11-15 at 14:05 -0500, Andrew Cagney wrote:
> That unfortunatly isn't sufficient; the old code in CLI.java was 
> differentiating between and NPE and other exceptions - dumping the stack 
> when an NPE occured.  I'll restore this behavior; so we're at least back 
> to a usable status quo (and from a HPD user prospective in a better 
> position - these back-traces plain suck)

OK, but I don't understand this heuristic with NullPointerException. You
introduced a "nasty()" method (cute name) that gobbles up the Exception
except if it is a NullPointerException or has an empty message. But that
doesn't seem to cover other catastrophic failures like
ClassCastException, ArrayOutOfBoundsException, ArithmeticException,
IllegalArgumentException or NumberFormatException, etc. that might or
might not have empty messages, but that are real core bugs if they occur
and "bubble up" to the CLI.

I think your argument that from a user perspective printing fatal
exceptions just plain sucks is right. But if there are really fatal
exceptions in the core code that then get hidden by the CLI layer we
(and we are also our own users) have a hard time fixing those. At least
our users will be unable to properly report what went wrong. So the real
solution is to just not plain suck, by not having bugs in our core
code :)

> I'll also add a prototype version of CLI.printError() that we discussed; 
> and have just EvalCommands use it; we can then refine it as needed.

That seems fine. I recommend you also just rip out the whole Message
queuing structure and handle that in the same way with a simple
printMessage(). Having two different mechanisms for creating user
feedback in the CLI is probably confusing.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Kris,

On Thu, 2007-11-15 at 15:40 -0500, Kris Van Hees wrote:
> On Thu, Nov 15, 2007 at 06:41:56PM +0100, Mark Wielaard wrote:
> > Hi Phil,
> > > << Example exception output deleted - see original message for details. >>
> > > If this is the way forward, I'll have to gobble exceptions locally in 
> > > CoreCommand, and just deal with them locally.
> > 
> > Yes, I think that is the way forward. Something terribly failed. And
> > just passing the "address 6992f8 cannot be found in metadata table."
> > message to the user is clearly not very helpful if the user just wanted
> > to run a specific command. Only the command knows if this is something
> > fatal or not and should catch it at the appropriate level and report
> > what the exact action was that was attempted and which structure
> > couldn't be created because of the error.
> 
> I do not think that an exception like this should ever get handled at the
> level of the command module.  In fact, in a case like this it should never
> have floated up that high without getting handled.  Clearly, being unable to
> peek at the given address was the result of trying to do some action.  The
> failure to peek at the address either causes that action to fail, or it can be
> ignored (or otherwise recovered from).  Either way, the command module should
> not be exposed to this level of detail (though the information may need to be
> retained as a nested exception for more verbose output, etc...).

Yes I did mean that it would be ideal if the right submodule handled
issues and returned a more specific result or exception, so the command
doesn't have to handle it. The "at the appropriate level" in the above
was meant to indicate that it isn't necessarily at the Command class
level, but through the while chain of commands that are needed for the
command. Bad English there, sorry.

But I don't think it is bad for all exceptions to be passed up to the
CLI level. If we had exception types for "unable to access needed
inferior/core data" that would already be helpful at the Command level.
Then the Command could at least report that it tried but failed for a
specific reason. The current situation that we don't have specific
exception messages for different kinds of failures (some fatal system
issues, some logic programming errors, some invalid data, some unable to
access data, etc) makes it impossible to properly report to the user why
something failed or why a result may only be partly correct.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Phil,

On Thu, 2007-11-15 at 18:19 +0000, Phil Muldoon wrote:
> Mark Wielaard wrote:
> > On Thu, 2007-11-15 at 17:01 +0000, Phil Muldoon wrote:
> >   
> >> As talked about on IRC over the corefile 
> >> message design, exceptions can and are used to carry warnings, messages 
> >> and so on. How do you differentiate between a warning and an error in 
> >> this case?
> >
> > By using different exception types, so a higher level can distinquish
> > between a "recoverable" warning and a "unrecoverable" error.

> Like I mentioned in reply to Sami's email yesterday, having a napi throw 
> several different unchecked exception types places a huge and unfair 
> burden on the user to know the code beyond the api. The places "must be 
> an expert on Frysk to call Frysk apis" charge at our feet.

Yes, if we stick with unchecked exceptions then they cannot really be
part of the api (or they need to be very clearly documented). Unless we
define a very small specific set (that doesn't overlap with the core
RuntimeExceptions) that are used consistently in all the code.

> > Yes, I think that is the way forward. Something terribly failed. And
> > just passing the "address 6992f8 cannot be found in metadata table."
> > message to the user is clearly not very helpful if the user just wanted
> > to run a specific command. 
> 
> What would be helpful here? It's a message, not an error? Basically it 
> just means cannot read the address here as we don't know how to read it. 
> I can format the  message however I want, but it is still not an error 
> message. However it sure looks like one now ;)

It is neither a user message, nor an error. I do see your point. You
have only partial information and would let the user know somehow that
you will be unable to perform all requests. I like Kris suggestion to
have a more specific result or exception here that says "Missing Map
Data", then a higher level can decide to act on that and suggest how the
user can help, or suggest what normal causes for such missing/corrupt
information is.

Cheers,

Mark

Re: fhpd vs RuntimeExceptions

[Andrew Cagney]

Mark Wielaard wrote:
> Hi Andrew,
>
> On Thu, 2007-11-15 at 14:05 -0500, Andrew Cagney wrote:
>   
>> That unfortunatly isn't sufficient; the old code in CLI.java was 
>> differentiating between and NPE and other exceptions - dumping the stack 
>> when an NPE occured.  I'll restore this behavior; so we're at least back 
>> to a usable status quo (and from a HPD user prospective in a better 
>> position - these back-traces plain suck)
>>     
>
> OK, but I don't understand this heuristic with NullPointerException. You
> introduced a "nasty()" method (cute name) that gobbles up the Exception
> except if it is a NullPointerException or has an empty message. But that
> doesn't seem to cover other catastrophic failures like
> ClassCastException, ArrayOutOfBoundsException, ArithmeticException,
> IllegalArgumentException or NumberFormatException, etc. that might or
> might not have empty messages, but that are real core bugs if they occur
> and "bubble up" to the CLI.
>
>   
True, it gets rid of the immediate problem.  More importantly it lets us 
walk away from this bike shed and focus on more critical - a corrupt 
variable or wrong back-trace is far more serious than the exact text of 
an error message.  Just like how the dog hears in the farside cartoon 
<<Rover, blah blah blah ...>>, we've ensured that the user at least sees 
"Error: ".  As our user base expands we can refine this.

[...]
>
> That seems fine. I recommend you also just rip out the whole Message
> queuing structure and handle that in the same way with a simple
> printMessage(). Having two different mechanisms for creating user
> feedback in the CLI is probably confusing.
>
>   
Of course; however the cli code base is still in rehab from me 
refactoring N ways of implementing a command down to one.
Can you create a bug?

Andrew

Re: fhpd vs RuntimeExceptions

[Mark Wielaard]

Hi Andrew,

On Fri, 2007-11-16 at 09:31 -0500, Andrew Cagney wrote:
> True, it gets rid of the immediate problem.  More importantly it lets us 
> walk away from this bike shed and focus on more critical - a corrupt 
> variable or wrong back-trace is far more serious than the exact text of 
> an error message.  Just like how the dog hears in the farside cartoon 
> <<Rover, blah blah blah ...>>, we've ensured that the user at least sees 
> "Error: ".  As our user base expands we can refine this.

The immediate problem is not nice error messages (which would be
somewhat of a bikeshed that I wouldn't even try to get incolved with
seeing I am not even a native speaker). The problem is how we and our
users can help each other pinpointing bugs in our code. For example I
had to debug a monitor lock issue last Friday. The current setup doesn't
show me any stacktraces to help me so it is hard to see where this came
from. Hiding information from the current users (us!) is not the right
default imho. If unexpected exceptions happen the default should be to
show all information to help our users and developers to get at the root
cause.

> > That seems fine. I recommend you also just rip out the whole Message
> > queuing structure and handle that in the same way with a simple
> > printMessage(). Having two different mechanisms for creating user
> > feedback in the CLI is probably confusing.
> >   
> Of course; however the cli code base is still in rehab from me 
> refactoring N ways of implementing a command down to one.
> Can you create a bug?

Here you go: http://sourceware.org/bugzilla/show_bug.cgi?id=5402

Cheers,

Mark