[Bug translator/6851] New: printf %c

[Bug translator/6851] New: printf %c

[jkenisto at us dot ibm dot com]

This may be multiple bugs, but I suspect it's just one bug with an
incomplete fix.

The primary bug is that the translator doesn't support the %c printf
conversion specifier.  (All this is on an i686 system.)  Given the
following script...
----- bug.stp -----
probe process("bug").function("echo_char"),
    process("nobug").function("echo_char")
{
	printf("%s:%s(%c/%#x)\n", execname(), probefunc(), $c, $c)
}
probe process("bug").function("echo_char"),
    process("nobug").function("echo_char")
{
	printf("%s:%s(%#x)\n", execname(), probefunc(), $c)
}
-----
... stap -v bug.stp reports
parse error: invalid or missing conversion specifier
	saw: string '%s:%s(%c/%#x)\n' at bug.stp:4:9
parse error: expected statement
	saw: keyword at bug.stp:6:1
parse error: expected statement
	saw: bug.stp EOF
3 parse error(s).
Pass 1: parsed user script and 45 library script(s) in 460usr/20sys/557real ms.
Pass 1: parse failed.  Try again with more '-v' (verbose) options.

I made what seemed to be the obvious changes to the translator to add %c
support:
----- printf_c.patch -----
diff -ur old/elaborate.cxx new/elaborate.cxx
--- old/elaborate.cxx	2008-08-08 17:09:28.000000000 -0700
+++ new/elaborate.cxx	2008-08-08 17:09:28.000000000 -0700
@@ -3623,6 +3623,7 @@
 	    case print_format::conv_unsigned_uppercase_hex:
 	    case print_format::conv_unsigned_lowercase_hex:
 	    case print_format::conv_binary:
+	    case print_format::conv_char:
 	    case print_format::conv_memory:
 	      wanted = pe_long;
 	      break;
diff -ur old/staptree.cxx new/staptree.cxx
--- old/staptree.cxx	2008-08-08 17:09:28.000000000 -0700
+++ new/staptree.cxx	2008-08-08 17:13:27.000000000 -0700
@@ -458,6 +458,10 @@
 	      oss << "b";
 	      break;
 
+	    case conv_char:
+	      oss << "llc";
+	      break;
+
 	    case conv_signed_decimal:
 	      oss << "lld";
 	      break;
@@ -635,14 +639,18 @@
       if (i == str.end())
 	break;
 
-      // Parse the actual conversion specifier (sdiouxX)
+      // Parse the actual conversion specifier (bcsmdioupxXn)
       switch (*i)
 	{
 	  // Valid conversion types
 	case 'b':
 	  curr.type = conv_binary;
 	  break;
-	  
+
+	case 'c':
+	  curr.type = conv_char;
+	  break;
+
 	case 's':
 	  curr.type = conv_string;
 	  break;
diff -ur old/staptree.h new/staptree.h
--- old/staptree.h	2008-08-08 17:09:28.000000000 -0700
+++ new/staptree.h	2008-08-08 17:09:28.000000000 -0700
@@ -287,6 +287,7 @@
       conv_unsigned_uppercase_hex,
       conv_unsigned_lowercase_hex,
       conv_string,
+      conv_char,
       conv_memory,
       conv_literal,
       conv_binary,
----- end of patch -----
Now stap accepts the %c conversion specifier, but the generated code
is wrong.  Given the following...
----- echo_chars.c -----
#include <stdio.h>

#ifdef BUG
void echo_char(char c)
#else
void echo_char(int c)
#endif
{
	asm volatile("");	// defeat inlining
	printf("%c", c);
}

main()
{
	echo_char('s');
	echo_char('t');
	echo_char('a');
	echo_char('p');
	printf("\n");
}
----- Makefile -----
all: bug nobug

bug: echo_chars.c
	$(CC) -g -DBUG echo_chars.c -o bug

nobug: echo_chars.c
	$(CC) -g echo_chars.c -o nobug

clean:
	rm -f bug nobug
-----
... if you run
	$ stap -v bug.stp
in one window and
	$ ./bug
	$ ./nobug
in another window, stap reports:
Pass 1: parsed user script and 45 library script(s) in 450usr/20sys/594real ms.
Pass 2: analyzed script: 4 probe(s), 4 function(s), 0 embed(s), 0 global(s) in
10usr/10sys/11real ms.
Pass 3: translated to C into
"/tmp/stapoq2MDW/stap_baf6e71039a9725fc5cb2616250691db_3376.c" in
20usr/20sys/46real ms.
Pass 4: compiled C into "stap_baf6e71039a9725fc5cb2616250691db_3376.ko" in
3230usr/300sys/6758real ms.
Pass 5: starting run.
bug:echo_char(,/0x2c00000000)
bug:echo_char(0x2c)
bug:echo_char(s/0x7300000000)
bug:echo_char(0x73)
bug:echo_char(t/0x7400000000)
bug:echo_char(0x74)
bug:echo_char(a/0x6100000000)
bug:echo_char(0x61)
nobug:echo_char(s/0x7300000000)
nobug:echo_char(0x73)
nobug:echo_char(t/0x7400000000)
nobug:echo_char(0x74)
nobug:echo_char(a/0x6100000000)
nobug:echo_char(0x61)
nobug:echo_char(p/0x7000000000)
nobug:echo_char(0x70)
Pass 5: run completed in 0usr/50sys/31160real ms.

Tracing ./nobug, where the arg is declared as an int, stap at least reports
the %c value correctly (i.e., [s t a p]).  Tracing ./bug, where the arg is
declared as a char, stap reports these same values as [, s t a].

But in both cases, the %#x specifier following the %c specifier in the
same format string gets it wrong -- the byte order is reversed.  Given
the same value, %#x alone gets it right.

So ./nobug is a misnomer, but at least it demonstrates one less error than
./bug.

-- 
           Summary: printf %c
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: translator
        AssignedTo: systemtap at sources dot redhat dot com
        ReportedBy: jkenisto at us dot ibm dot com


http://sourceware.org/bugzilla/show_bug.cgi?id=6851

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/6851] printf %c

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2008-09-09 01:16 -------
(In reply to comment #0)
> But in both cases, the %#x specifier following the %c specifier in the
> same format string gets it wrong -- the byte order is reversed.  Given
> the same value, %#x alone gets it right.

The byte order is not reversed -- it's just picking up bytes left over from the
char.  The translator is pushing an int64_t, but the print runtime is only
pulling off an int for %c formats.  You can fix it in either place, e.g.:

---- begin 1 ----
diff --git a/runtime/vsprintf.c b/runtime/vsprintf.c
index 4ffcf72..f807c4f 100644
--- a/runtime/vsprintf.c
+++ b/runtime/vsprintf.c
@@ -392,7 +392,7 @@ int _stp_vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
 					++str;
 				}
 			}
-			c = (unsigned char) va_arg(args, int);
+			c = (unsigned char) va_arg(args, int64_t);
 			if (str <= end)
 				*str = c;
 			++str;
---- end 1 ----

OR

---- begin 2 ----
diff --git a/translate.cxx b/translate.cxx
index 2fe3331..4f91e80 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -4193,6 +4193,9 @@ c_unparser::visit_print_format (print_format* e)
 	/* The type of the %m argument is 'char*'.  */
 	if (components[i].type == print_format::conv_memory)
 	  o->line() << ", (char*)(uintptr_t)" << tmp[arg_ix++].value();
+	/* The type of the %c argument is 'char*'.  */
+	else if (components[i].type == print_format::conv_char)
+	  o->line() << ", (int)" << tmp[arg_ix++].value();
 	else
 	  o->line() << ", " << tmp[arg_ix++].value();
       }
---- end 2 ----

Changing the runtime makes me a little nervous about unseen effects, so the
second patch may be the safer route.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=6851

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/6851] printf %c

[joshua dot i dot stone at intel dot com]

------- Additional Comments From joshua dot i dot stone at intel dot com  2008-09-09 01:21 -------
(In reply to comment #1)
> +	/* The type of the %c argument is 'char*'.  */

Oops, should be 'int' -- in full:

diff --git a/translate.cxx b/translate.cxx
index 2fe3331..4f91e80 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -4193,6 +4193,9 @@ c_unparser::visit_print_format (print_format* e)
 	/* The type of the %m argument is 'char*'.  */
 	if (components[i].type == print_format::conv_memory)
 	  o->line() << ", (char*)(uintptr_t)" << tmp[arg_ix++].value();
+	/* The type of the %c argument is 'int'.  */
+	else if (components[i].type == print_format::conv_char)
+	  o->line() << ", (int)" << tmp[arg_ix++].value();
 	else
 	  o->line() << ", " << tmp[arg_ix++].value();
       }

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=6851

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/6851] printf %c

[fche at redhat dot com]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
Bug 6851 depends on bug 6871, which changed state.

Bug 6871 Summary: uprobes $context values often wrong
http://sourceware.org/bugzilla/show_bug.cgi?id=6871

           What    |Old Value                   |New Value
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXEDBug 6851 depends on bug 6871, which changed state.

Bug 6871 Summary: uprobes $context values often wrong
http://sourceware.org/bugzilla/show_bug.cgi?id=6871

           What    |Old Value                   |New Value
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

http://sourceware.org/bugzilla/show_bug.cgi?id=6851

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/6851] printf %c

[ebaron at redhat dot com]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|systemtap at sources dot    |ebaron at redhat dot com
                   |redhat dot com              |
             Status|NEW                         |ASSIGNED


http://sourceware.org/bugzilla/show_bug.cgi?id=6851

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.