2007-08-10 Josh Stone <joshua.i.stone@intel.com>

	PR 4593
	* translate.cxx (c_unparser::emit_common_header): Add an
	error_buffer to the context to allow dynamic error messages.
	* tapsets.cxx (dwflpp::express_as_string): Let deref / store_deref
	fill in last_error with a detailed message.

runtime/
	* loc2c-runtime.h (deref, store_deref): Set an error message with
	the pointer value and name into last_error, since it's hard to
	determine the details once you've already jumped to deref_fault.

tapset/
	* conversions.stp (kernel_string, kernel_long, kernel_int,
	kernel_short, kernel_char, user_string_warn): Use the
	CONTEXT->error_buffer to create an error message instead of a static
	local array.

7 files changed, 95 insertions, 73 deletions

diff --git a/ChangeLog b/ChangeLog
index 61444b34..8ee6731a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2007-08-10  Josh Stone  <joshua.i.stone@intel.com>
+
+	PR 4593
+	* translate.cxx (c_unparser::emit_common_header): Add an
+	error_buffer to the context to allow dynamic error messages.
+	* tapsets.cxx (dwflpp::express_as_string): Let deref / store_deref
+	fill in last_error with a detailed message.
+
 2007-08-09  Frank Ch. Eigler  <fche@elastic.org>
 
 	From Lai Jiangshan <laijs@cn.fujitsu.com>:
diff --git a/runtime/ChangeLog b/runtime/ChangeLog
index 2f7f8140..56ad6103 100644
--- a/runtime/ChangeLog
+++ b/runtime/ChangeLog
@@ -1,3 +1,10 @@
+2007-08-10  Josh Stone  <joshua.i.stone@intel.com>
+
+	PR 4593
+	* loc2c-runtime.h (deref, store_deref): Set an error message with
+	the pointer value and name into last_error, since it's hard to
+	determine the details once you've already jumped to deref_fault.
+
 2007-07-09  Martin Hunt  <hunt@redhat.com>
 
 	* sym.h (STP_MAX_MODULES): Raise limit to 256.
diff --git a/runtime/loc2c-runtime.h b/runtime/loc2c-runtime.h
index 02d4df17..34b4ba10 100644
--- a/runtime/loc2c-runtime.h
+++ b/runtime/loc2c-runtime.h
@@ -60,6 +60,21 @@
    must work right for kernel addresses, and can use whatever existing
    machine-specific kernel macros are convenient.  */
 
+#define DEREF_FAULT(addr) ({						    \
+    snprintf(c->error_buffer, sizeof(c->error_buffer),			    \
+      "kernel read fault at 0x%p (%s)", (void *)(intptr_t)(addr), #addr);   \
+    c->last_error = c->error_buffer;					    \
+    goto deref_fault;							    \
+    })
+
+#define STORE_DEREF_FAULT(addr) ({					    \
+    snprintf(c->error_buffer, sizeof(c->error_buffer),			    \
+      "kernel write fault at 0x%p (%s)", (void *)(intptr_t)(addr), #addr);  \
+    c->last_error = c->error_buffer;					    \
+    goto deref_fault;							    \
+    })
+
+
 #if defined __i386__
 
 /* The stack pointer is unlike other registers.  When a trap happens in
@@ -143,7 +158,7 @@
       default: _v = __get_user_bad();					      \
       }									      \
     if (_bad)								      \
-      goto deref_fault;							      \
+      DEREF_FAULT(addr);							      \
     _v;									      \
   })
 
@@ -158,7 +173,7 @@
       default: __put_user_bad();					      \
       }									      \
     if (_bad)								      \
-      goto deref_fault;							      \
+      STORE_DEREF_FAULT(addr);							      \
   })
 
 
@@ -178,7 +193,7 @@
       default: _v = __get_user_bad();					      \
       }									      \
     if (_bad)								      \
-      goto deref_fault;							      \
+      DEREF_FAULT(addr);							      \
     _v;									      \
   })
 
@@ -194,7 +209,7 @@
       default: __put_user_bad();					      \
       }									      \
     if (_bad)								      \
-      goto deref_fault;							      \
+      STORE_DEREF_FAULT(addr);							      \
   })
 
 #elif defined __ia64__
@@ -210,7 +225,7 @@
 	default: __get_user_unknown(); break;				\
 	}								\
     if (_bad)  								\
-	goto deref_fault;						\
+	DEREF_FAULT(addr);						\
      _v;								\
    })
 
@@ -225,7 +240,7 @@
 	default: __put_user_unknown(); break;				\
 	}								\
     if (_bad)								\
-	   goto deref_fault;						\
+	   STORE_DEREF_FAULT(addr);						\
    })
 
 #elif defined __powerpc__ || defined __powerpc64__
@@ -283,7 +298,7 @@
       default: _v = __get_user_bad();					      \
       }									      \
     if (_bad)								      \
-      goto deref_fault;							      \
+      DEREF_FAULT(addr);							      \
     _v;									      \
   })
 
@@ -299,7 +314,7 @@
       default: __put_user_bad();					      \
       }									      \
     if (_bad)								      \
-      goto deref_fault;							      \
+      STORE_DEREF_FAULT(addr);							      \
   })
 
 #elif defined (__arm__)
@@ -449,7 +464,7 @@
 	default: __get_user_bad(); break;				\
 	}								\
     if (_bad)  								\
-	goto deref_fault;						\
+	DEREF_FAULT(addr);						\
      _v;								\
    })
 
@@ -464,7 +479,7 @@
 	default: __put_user_bad(); break;				\
 	}								\
     if (_bad)								\
-	   goto deref_fault;						\
+	   STORE_DEREF_FAULT(addr);						\
    })
 
 #elif defined (__s390__) || defined (__s390x__)
@@ -550,7 +565,7 @@
 		_bad = -EFAULT;					\
 	}							\
 	if (_bad)						\
-		goto deref_fault;				\
+		DEREF_FAULT(addr);				\
 	_v;							\
 })
 
@@ -562,7 +577,7 @@
 		__stp_put_asm((u8)(value>>((size-i-1)*8)&0xff),	\
                              (u64)addr+i,_bad); 		\
 		if (_bad)					\
-			goto deref_fault;			\
+			STORE_DEREF_FAULT(addr);			\
         }                                                       \
 })
 
@@ -617,6 +632,5 @@
 
 #define CATCH_DEREF_FAULT()				\
   if (0) {						\
-deref_fault:						\
-    CONTEXT->last_error = "pointer dereference fault";	\
+deref_fault: ;						\
   }
diff --git a/tapset/ChangeLog b/tapset/ChangeLog
index f4d20344..a164aa28 100644
--- a/tapset/ChangeLog
+++ b/tapset/ChangeLog
@@ -1,3 +1,10 @@
+2007-08-10  Josh Stone  <joshua.i.stone@intel.com>
+
+	* conversions.stp (kernel_string, kernel_long, kernel_int,
+	kernel_short, kernel_char, user_string_warn): Use the
+	CONTEXT->error_buffer to create an error message instead of a static
+	local array.
+
 2007-08-10  Frank Ch. Eigler  <fche@elastic.org>
 
 	From "Zhaolei" zhaolei@cn.fujitsu.com:
diff --git a/tapset/conversions.stp b/tapset/conversions.stp
index 940cc2c5..3993c8ac 100644
--- a/tapset/conversions.stp
+++ b/tapset/conversions.stp
@@ -1,5 +1,6 @@
 // conversions tapset
 // Copyright (C) 2005-2007 Red Hat Inc.
+// Copyright (C) 2007 Intel Corporation.
 //
 // This file is part of systemtap, and is free software.  You can
 // redistribute it and/or modify it under the terms of the GNU General
@@ -9,68 +10,52 @@
 function kernel_string:string (addr:long) %{ /* pure */
   char *destination = THIS->__retvalue;
   deref_string (destination, THIS->addr, MAXSTRINGLEN);
-  goto success;
-deref_fault: /* branched to from deref() */
-  {
-    /* XXX: concurrent errors could result in corrupted message string */
-    static char errmsg[60];
-    snprintf (errmsg, sizeof(errmsg), "kernel string copy fault at 0x%p",
-	 (void *) (uintptr_t) THIS->addr);
-    CONTEXT->last_error = errmsg;
+  if (0) {
+deref_fault: /* branched to from deref_string() */
+    snprintf (CONTEXT->error_buffer, sizeof(CONTEXT->error_buffer),
+        "kernel string copy fault at 0x%p", (void *) (uintptr_t) THIS->addr);
+    CONTEXT->last_error = CONTEXT->error_buffer;
   }
-success: ;
 %}
 
 function kernel_long:long (addr:long) %{ /* pure */
-  THIS->__retvalue = (int64_t) deref (sizeof (long), (long *) (intptr_t) THIS->addr);
-  goto success;
+  THIS->__retvalue = kread((long *) (intptr_t) THIS->addr);
+  if (0) {
 deref_fault: /* branched to from deref() */
-  {
-    static char errmsg[60];
-    snprintf (errmsg, sizeof(errmsg), "kernel long copy fault at 0x%p",
-	 (void *) (uintptr_t) THIS->addr);
-    CONTEXT->last_error = errmsg;
+    snprintf (CONTEXT->error_buffer, sizeof(CONTEXT->error_buffer),
+        "kernel long copy fault at 0x%p", (void *) (uintptr_t) THIS->addr);
+    CONTEXT->last_error = CONTEXT->error_buffer;
   }
-success: ;
 %}
 
 function kernel_int:long (addr:long) %{ /* pure */
-  THIS->__retvalue = (int64_t) deref (sizeof (int), (int *) (intptr_t) THIS->addr);
-  goto success;
+  THIS->__retvalue = kread((int *) (intptr_t) THIS->addr);
+  if (0) {
 deref_fault: /* branched to from deref() */
-  {
-    static char errmsg[60];
-    snprintf (errmsg, sizeof(errmsg), "kernel int copy fault at 0x%p",
-	 (void *) (uintptr_t) THIS->addr);
-    CONTEXT->last_error = errmsg;
+    snprintf (CONTEXT->error_buffer, sizeof(CONTEXT->error_buffer),
+        "kernel int copy fault at 0x%p", (void *) (uintptr_t) THIS->addr);
+    CONTEXT->last_error = CONTEXT->error_buffer;
   }
-success: ;
 %}
 
 function kernel_short:long (addr:long) %{ /* pure */
-  THIS->__retvalue = (int64_t) deref (sizeof (short), (short *) (intptr_t) THIS->addr);
-  goto success;
+  THIS->__retvalue = kread((short *) (intptr_t) THIS->addr);
+  if (0) {
 deref_fault: /* branched to from deref() */
-  {
-    static char errmsg[60];
-    snprintf (errmsg, sizeof(errmsg), "kernel short copy fault at 0x%p",
-	 (void *) (uintptr_t) THIS->addr);
-    CONTEXT->last_error = errmsg;
+    snprintf (CONTEXT->error_buffer, sizeof(CONTEXT->error_buffer),
+        "kernel short copy fault at 0x%p", (void *) (uintptr_t) THIS->addr);
+    CONTEXT->last_error = CONTEXT->error_buffer;
   }
-success: ;
 %}
 
 function kernel_char:long (addr:long) %{ /* pure */
-  THIS->__retvalue = (int64_t) deref (sizeof (char), (char *) (intptr_t) THIS->addr);
-  goto success;
+  THIS->__retvalue = kread((char *) (intptr_t) THIS->addr);
+  if (0) {
 deref_fault: /* branched to from deref() */
-  {
-    static char errmsg[60];
-    snprintf (errmsg, sizeof(errmsg), "kernel char copy fault at 0x%p",
-	 (void *) (uintptr_t) THIS->addr);
-    CONTEXT->last_error = errmsg;
+    snprintf (CONTEXT->error_buffer, sizeof(CONTEXT->error_buffer),
+        "kernel char copy fault at 0x%p", (void *) (uintptr_t) THIS->addr);
+    CONTEXT->last_error = CONTEXT->error_buffer;
   }
-success: ;
 %}
 
 
@@ -81,34 +66,37 @@ success: ;
 
 function user_string:string (addr:long) %{ /* pure */
   if (_stp_strncpy_from_user (THIS->__retvalue, 
-		(const char __user*) (uintptr_t) THIS->addr,
-		MAXSTRINGLEN) < 0)
-	strlcpy (THIS->__retvalue, "<unknown>", MAXSTRINGLEN);
+        (const char __user*) (uintptr_t) THIS->addr,
+        MAXSTRINGLEN) < 0)
+    strlcpy (THIS->__retvalue, "<unknown>", MAXSTRINGLEN);
 %}
 
 function user_string2:string (addr:long, err_msg:string) %{ /* pure */
   if (_stp_strncpy_from_user (THIS->__retvalue, 
-		(const char __user*) (uintptr_t) THIS->addr,
-		MAXSTRINGLEN) < 0)
+        (const char __user*) (uintptr_t) THIS->addr,
+        MAXSTRINGLEN) < 0)
     strlcpy (THIS->__retvalue, THIS->err_msg, MAXSTRINGLEN);
 %}
 
 function user_string_warn:string (addr:long) %{ /* pure */
   long rc = _stp_strncpy_from_user (THIS->__retvalue,
-		(const char __user*) (uintptr_t) THIS->addr, MAXSTRINGLEN);
+      (const char __user*) (uintptr_t) THIS->addr, MAXSTRINGLEN);
   if (rc < 0) {
-	static char errmsg[60];
-	snprintf (errmsg, sizeof(errmsg), "user string copy fault %ld at %p", rc,
-		(void *) (uintptr_t) THIS->addr);
-	_stp_warn(errmsg);
-	strlcpy (THIS->__retvalue, "<unknown>", MAXSTRINGLEN);
+    // NB: using error_buffer to get local space for the warning, but we're
+    // not aborting, so leave last_error alone.
+    snprintf (CONTEXT->error_buffer, sizeof(CONTEXT->error_buffer),
+        "user string copy fault %ld at %p", rc,
+        (void *) (uintptr_t) THIS->addr);
+    _stp_warn(CONTEXT->error_buffer);
+    strlcpy (THIS->__retvalue, "<unknown>", MAXSTRINGLEN);
   }
 %}
 
 function user_string_quoted:string (addr:long) %{ /* pure */
   if (THIS->addr == 0)	
-	strlcpy (THIS->__retvalue, "NULL", MAXSTRINGLEN);
+    strlcpy (THIS->__retvalue, "NULL", MAXSTRINGLEN);
   else
-        /* XXX: stp_text_str uses sleepy __get_user() => unsafe ?!  */
-	_stp_text_str(THIS->__retvalue, (char *)(uintptr_t)THIS->addr, MAXSTRINGLEN, 1, 1);
+    /* XXX: stp_text_str uses sleepy __get_user() => unsafe ?!  */
+    _stp_text_str(THIS->__retvalue, (char *)(uintptr_t)THIS->addr,
+        MAXSTRINGLEN, 1, 1);
 %}
diff --git a/tapsets.cxx b/tapsets.cxx
index eee1a6bc..3b08a348 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -1763,12 +1763,9 @@ struct dwflpp
     fprintf(memstream, "if (0) goto deref_fault;\n");
 
     // XXX: deref flag not reliable; emit fault label unconditionally
-    // XXX: print the faulting address, like the user_string/kernel_string
-    // tapset functions do
     (void) deref;
     fprintf(memstream,
             "deref_fault:\n"
-            "  c->last_error = \"pointer dereference fault\";\n"
             "  goto out;\n");
     fprintf(memstream, "}\n");
 
diff --git a/translate.cxx b/translate.cxx
index e6bd4e12..54878a40 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -1,6 +1,6 @@
 // translation pass
 // Copyright (C) 2005-2007 Red Hat Inc.
-// Copyright (C) 2005, 2006 Intel Corporation
+// Copyright (C) 2005-2007 Intel Corporation.
 //
 // This file is part of systemtap, and is free software.  You can
 // redistribute it and/or modify it under the terms of the GNU General
@@ -853,6 +853,7 @@ c_unparser::emit_common_header ()
   o->newline() << "const char *probe_point;";
   o->newline() << "int actionremaining;";
   o->newline() << "unsigned nesting;";
+  o->newline() << "string_t error_buffer;";
   o->newline() << "const char *last_error;";
   // NB: last_error is used as a health flag within a probe.
   // While it's 0, execution continues