|
From: Bart V. A. <bva...@ac...> - 2011-10-04 16:54:29
|
In order to support escaping XML meta-characters several formatting functions in Valgrind support the %t format specifier. That's an unfortunate choice since gcc's format specification checker doesn't recognize %t. My proposal is to use the format specifier %pS instead of %t and to remove the _no_f_c variants of the formatting functions, as is done in the patch below. This patch has been inspired by the format specifiers supported by the vsnprintf() function in the Linux kernel (source file lib/vsprintf.c -- see e.g. http://github.com/torvalds/linux/blob/master/lib/vsprintf.c#L1128). diff --git a/coregrind/m_debuginfo/debuginfo.c b/coregrind/m_debuginfo/debuginfo.c index 97373e8..ec99d59 100644 --- a/coregrind/m_debuginfo/debuginfo.c +++ b/coregrind/m_debuginfo/debuginfo.c @@ -2563,14 +2563,14 @@ Bool VG_(use_FPO_info) ( /*MOD*/Addr* ipP, /*--------------------------------------------------------------*/ /* Try to make p2XA(dst, fmt, args..) turn into - VG_(xaprintf_no_f_c)(dst, fmt, args) without having to resort to + VG_(xaprintf)(dst, fmt, args) without having to resort to vararg macros. As usual with everything to do with varargs, it's an ugly hack. //#define p2XA(dstxa, format, args...) - // VG_(xaprintf_no_f_c)(dstxa, format, ##args) + // VG_(xaprintf)(dstxa, format, ##args) */ -#define p2XA VG_(xaprintf_no_f_c) +#define p2XA VG_(xaprintf) /* Add a zero-terminating byte to DST, which must be an XArray* of HChar. */ @@ -2712,7 +2712,7 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside local var \"%t\",", + "Location 0x%lx is %lu byte%s inside local var \"%pS\",", data_addr, var_offset, vo_plural, var->name ); TAGR( dn1 ); TAGL( dn2 ); @@ -2736,18 +2736,18 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside local var \"%t\"", + "Location 0x%lx is %lu byte%s inside local var \"%pS\"", data_addr, var_offset, vo_plural, var->name ); TAGR( dn1 ); XAGL( dn2 ); TXTL( dn2 ); p2XA( dn2, - "declared at %t:%d, in frame #%d of thread %d", + "declared at %pS:%d, in frame #%d of thread %d", var->fileName, var->lineNo, frameNo, (Int)tid ); TXTR( dn2 ); // FIXME: also do <dir> p2XA( dn2, - " <file>%t</file> <line>%d</line> ", + " <file>%pS</file> <line>%d</line> ", var->fileName, var->lineNo ); XAGR( dn2 ); } else { @@ -2768,7 +2768,7 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside %t%t", + "Location 0x%lx is %lu byte%s inside %pS%pS", data_addr, residual_offset, ro_plural, var->name, (HChar*)(VG_(indexXA)(described,0)) ); TAGR( dn1 ); @@ -2792,19 +2792,19 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside %t%t,", + "Location 0x%lx is %lu byte%s inside %pS%pS,", data_addr, residual_offset, ro_plural, var->name, (HChar*)(VG_(indexXA)(described,0)) ); TAGR( dn1 ); XAGL( dn2 ); TXTL( dn2 ); p2XA( dn2, - "declared at %t:%d, in frame #%d of thread %d", + "declared at %pS:%d, in frame #%d of thread %d", var->fileName, var->lineNo, frameNo, (Int)tid ); TXTR( dn2 ); // FIXME: also do <dir> p2XA( dn2, - " <file>%t</file> <line>%d</line> ", + " <file>%pS</file> <line>%d</line> ", var->fileName, var->lineNo ); XAGR( dn2 ); } else { @@ -2826,7 +2826,7 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside global var \"%t\"", + "Location 0x%lx is %lu byte%s inside global var \"%pS\"", data_addr, var_offset, vo_plural, var->name ); TAGR( dn1 ); } else { @@ -2844,18 +2844,18 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside global var \"%t\"", + "Location 0x%lx is %lu byte%s inside global var \"%pS\"", data_addr, var_offset, vo_plural, var->name ); TAGR( dn1 ); XAGL( dn2 ); TXTL( dn2 ); p2XA( dn2, - "declared at %t:%d", + "declared at %pS:%d", var->fileName, var->lineNo); TXTR( dn2 ); // FIXME: also do <dir> p2XA( dn2, - " <file>%t</file> <line>%d</line> ", + " <file>%pS</file> <line>%d</line> ", var->fileName, var->lineNo ); XAGR( dn2 ); } else { @@ -2876,7 +2876,7 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside %t%t,", + "Location 0x%lx is %lu byte%s inside %pS%pS,", data_addr, residual_offset, ro_plural, var->name, (HChar*)(VG_(indexXA)(described,0)) ); TAGR( dn1 ); @@ -2900,19 +2900,19 @@ static void format_message ( /*MOD*/XArray* /* of HChar */ dn1, if (xml) { TAGL( dn1 ); p2XA( dn1, - "Location 0x%lx is %lu byte%s inside %t%t,", + "Location 0x%lx is %lu byte%s inside %pS%pS,", data_addr, residual_offset, ro_plural, var->name, (HChar*)(VG_(indexXA)(described,0)) ); TAGR( dn1 ); XAGL( dn2 ); TXTL( dn2 ); p2XA( dn2, - "a global variable declared at %t:%d", + "a global variable declared at %pS:%d", var->fileName, var->lineNo); TXTR( dn2 ); // FIXME: also do <dir> p2XA( dn2, - " <file>%t</file> <line>%d</line> ", + " <file>%pS</file> <line>%d</line> ", var->fileName, var->lineNo ); XAGR( dn2 ); } else { diff --git a/coregrind/m_debuglog.c b/coregrind/m_debuglog.c index ccd2ad9..7ad3f8c 100644 --- a/coregrind/m_debuglog.c +++ b/coregrind/m_debuglog.c @@ -721,12 +721,23 @@ VG_(debugLog_vprintf) ( ret += myvprintf_int64(send, send_arg2, flags, 10, width, False, (ULong)(va_arg (vargs, UInt))); break; - case 'p': /* %p */ - ret += 2; - send('0',send_arg2); - send('x',send_arg2); - ret += myvprintf_int64(send, send_arg2, flags, 16, width, True, - (ULong)((UWord)va_arg (vargs, void *))); + case 'p': + if (format[i+1] == 'S') { + i++; + /* %pS, like %s but escaping chars for XML safety */ + /* Note: simplistic; ignores field width and flags */ + char *str = va_arg (vargs, char *); + if (str == (char*) 0) + str = "(null)"; + ret += myvprintf_str_XML_simplistic(send, send_arg2, str); + } else { + /* %p */ + ret += 2; + send('0',send_arg2); + send('x',send_arg2); + ret += myvprintf_int64(send, send_arg2, flags, 16, width, True, + (ULong)((UWord)va_arg (vargs, void *))); + } break; case 'x': /* %x */ case 'X': /* %X */ @@ -754,13 +765,6 @@ VG_(debugLog_vprintf) ( flags, width, str, format[i]=='S'); break; } - case 't': { /* %t, like %s but escaping chars for XML safety */ - /* Note: simplistic; ignores field width and flags */ - char *str = va_arg (vargs, char *); - if (str == (char*) 0) str = "(null)"; - ret += myvprintf_str_XML_simplistic(send, send_arg2, str); - break; - } // case 'y': { /* %y - print symbol */ // Char buf[100]; diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c index 89e7d46..17be47a 100644 --- a/coregrind/m_errormgr.c +++ b/coregrind/m_errormgr.c @@ -309,12 +309,12 @@ static void printSuppForIp_XML(UInt n, Addr ip, void* uu_opaque) { static UChar buf[ERRTXT_LEN]; if ( VG_(get_fnname_no_cxx_demangle) (ip, buf, ERRTXT_LEN) ) { - VG_(printf_xml_no_f_c)(" <sframe> <fun>%t</fun> </sframe>\n", buf); + VG_(printf_xml)(" <sframe> <fun>%pS</fun> </sframe>\n", buf); } else if ( VG_(get_objname)(ip, buf, ERRTXT_LEN) ) { - VG_(printf_xml_no_f_c)(" <sframe> <obj>%t</obj> </sframe>\n", buf); + VG_(printf_xml)(" <sframe> <obj>%pS</obj> </sframe>\n", buf); } else { - VG_(printf_xml_no_f_c)(" <sframe> <obj>*</obj> </sframe>\n"); + VG_(printf_xml)(" <sframe> <obj>*</obj> </sframe>\n"); } } @@ -405,10 +405,10 @@ static void gen_suppression(Error* err) again. */ VG_(printf_xml)(" <suppression>\n"); VG_(printf_xml)(" <sname>%s</sname>\n", dummy_name); - VG_(printf_xml_no_f_c)( - " <skind>%t:%t</skind>\n", VG_(details).name, name); + VG_(printf_xml)( + " <skind>%pS:%pS</skind>\n", VG_(details).name, name); if (anyXtra) - VG_(printf_xml_no_f_c)(" <skaux>%t</skaux>\n", xtra); + VG_(printf_xml)(" <skaux>%pS</skaux>\n", xtra); // Print stack trace elements VG_(apply_StackTrace)(printSuppForIp_XML, @@ -885,9 +885,9 @@ static Bool show_used_suppressions ( void ) if (su->count <= 0) continue; if (VG_(clo_xml)) { - VG_(printf_xml_no_f_c)( " <pair>\n" + VG_(printf_xml)( " <pair>\n" " <count>%d</count>\n" - " <name>%t</name>\n" + " <name>%pS</name>\n" " </pair>\n", su->count, su->sname ); } else { diff --git a/coregrind/m_libcprint.c b/coregrind/m_libcprint.c index 75eff72..263b2a5 100644 --- a/coregrind/m_libcprint.c +++ b/coregrind/m_libcprint.c @@ -166,17 +166,6 @@ UInt VG_(printf_xml) ( const HChar *format, ... ) return ret; } -/* An exact clone of VG_(printf_xml), unfortunately. */ -UInt VG_(printf_xml_no_f_c) ( const HChar *format, ... ) -{ - UInt ret; - va_list vargs; - va_start(vargs, format); - ret = VG_(vprintf_xml)(format, vargs); - va_end(vargs); - return ret; -} - /* --------- sprintf --------- */ @@ -500,17 +489,6 @@ UInt VG_(vmessage) ( VgMsgKind kind, const HChar* format, va_list vargs ) } /* Send a simple single-part XML message. */ -UInt VG_(message_no_f_c) ( VgMsgKind kind, const HChar* format, ... ) -{ - UInt count; - va_list vargs; - va_start(vargs,format); - count = VG_(vmessage) ( kind, format, vargs ); - va_end(vargs); - return count; -} - -/* Send a simple single-part message. */ UInt VG_(message) ( VgMsgKind kind, const HChar* format, ... ) { UInt count; diff --git a/coregrind/m_main.c b/coregrind/m_main.c index ceb852e..a8edffb 100644 --- a/coregrind/m_main.c +++ b/coregrind/m_main.c @@ -1004,9 +1004,9 @@ static void print_file_vars(Char* format) i++; } - VG_(printf_xml_no_f_c)( - "<logfilequalifier> <var>%t</var> " - "<value>%t</value> </logfilequalifier>\n", + VG_(printf_xml)( + "<logfilequalifier> <var>%pS</var> " + "<value>%pS</value> </logfilequalifier>\n", qualname,qual ); format[i] = '}'; @@ -1073,7 +1073,7 @@ static void print_preamble ( Bool logging_to_fd, VG_(printf_xml)("<preamble>\n"); /* Tool details */ - umsg_or_xml( VG_(clo_xml) ? "%s%t%t%t, %t%s\n" : "%s%s%s%s, %s%s\n", + umsg_or_xml( VG_(clo_xml) ? "%s%pS%pS%pS, %pS%s\n" : "%s%s%s%s, %s%s\n", xpre, VG_(details).name, NULL == VG_(details).version ? "" : "-", @@ -1089,7 +1089,7 @@ static void print_preamble ( Bool logging_to_fd, ); } - umsg_or_xml( VG_(clo_xml) ? "%s%t%s\n" : "%s%s%s\n", + umsg_or_xml( VG_(clo_xml) ? "%s%pS%s\n" : "%s%s%s\n", xpre, VG_(details).copyright_author, xpost ); /* Core details */ @@ -1125,13 +1125,13 @@ static void print_preamble ( Bool logging_to_fd, VG_(printf_xml)("\n"); VG_(printf_xml)("<pid>%d</pid>\n", VG_(getpid)()); VG_(printf_xml)("<ppid>%d</ppid>\n", VG_(getppid)()); - VG_(printf_xml_no_f_c)("<tool>%t</tool>\n", toolname); + VG_(printf_xml)("<tool>%pS</tool>\n", toolname); if (xml_fname_unexpanded) print_file_vars(xml_fname_unexpanded); if (VG_(clo_xml_user_comment)) { /* Note: the user comment itself is XML and is therefore to be passed through verbatim (%s) rather than escaped - (%t). */ + (%pS). */ VG_(printf_xml)("<usercomment>%s</usercomment>\n", VG_(clo_xml_user_comment)); } @@ -1140,14 +1140,14 @@ static void print_preamble ( Bool logging_to_fd, VG_(printf_xml)(" <vargv>\n"); if (VG_(name_of_launcher)) - VG_(printf_xml_no_f_c)(" <exe>%t</exe>\n", + VG_(printf_xml)(" <exe>%pS</exe>\n", VG_(name_of_launcher)); else - VG_(printf_xml_no_f_c)(" <exe>%t</exe>\n", + VG_(printf_xml)(" <exe>%pS</exe>\n", "(launcher name unknown)"); for (i = 0; i < VG_(sizeXA)( VG_(args_for_valgrind) ); i++) { - VG_(printf_xml_no_f_c)( - " <arg>%t</arg>\n", + VG_(printf_xml)( + " <arg>%pS</arg>\n", * (HChar**) VG_(indexXA)( VG_(args_for_valgrind), i ) ); } @@ -1155,11 +1155,11 @@ static void print_preamble ( Bool logging_to_fd, VG_(printf_xml)(" <argv>\n"); if (VG_(args_the_exename)) - VG_(printf_xml_no_f_c)(" <exe>%t</exe>\n", + VG_(printf_xml)(" <exe>%pS</exe>\n", VG_(args_the_exename)); for (i = 0; i < VG_(sizeXA)( VG_(args_for_client) ); i++) { - VG_(printf_xml_no_f_c)( - " <arg>%t</arg>\n", + VG_(printf_xml)( + " <arg>%pS</arg>\n", * (HChar**) VG_(indexXA)( VG_(args_for_client), i ) ); } @@ -2170,12 +2170,12 @@ Int valgrind_main ( Int argc, HChar **argv, HChar **envp ) if (VG_(clo_xml)) { HChar buf[50]; VG_(elapsed_wallclock_time)(buf); - VG_(printf_xml_no_f_c)( "<status>\n" + VG_(printf_xml)( "<status>\n" " <state>RUNNING</state>\n" - " <time>%t</time>\n" + " <time>%pS</time>\n" "</status>\n", buf ); - VG_(printf_xml_no_f_c)( "\n" ); + VG_(printf_xml)( "\n" ); } VG_(debugLog)(1, "main", "Running thread 1\n"); @@ -2280,9 +2280,9 @@ void shutdown_actions_NORETURN( ThreadId tid, if (VG_(clo_xml)) { HChar buf[50]; VG_(elapsed_wallclock_time)(buf); - VG_(printf_xml_no_f_c)( "<status>\n" + VG_(printf_xml)( "<status>\n" " <state>FINISHED</state>\n" - " <time>%t</time>\n" + " <time>%pS</time>\n" "</status>\n" "\n", buf); diff --git a/coregrind/m_xarray.c b/coregrind/m_xarray.c index 7491fdd..8c4c0e9 100644 --- a/coregrind/m_xarray.c +++ b/coregrind/m_xarray.c @@ -337,15 +337,6 @@ void VG_(xaprintf)( XArray* dst, const HChar* format, ... ) va_end(vargs); } -/* and again .. */ -void VG_(xaprintf_no_f_c)( XArray* dst, const HChar* format, ... ) -{ - va_list vargs; - va_start(vargs, format); - VG_(vcbprintf)( add_char_to_XA, (void*)dst, format, vargs ); - va_end(vargs); -} - /*--------------------------------------------------------------------*/ /*--- end m_xarray.c ---*/ diff --git a/exp-sgcheck/pc_common.c b/exp-sgcheck/pc_common.c index 89846fd..fa1aa42 100644 --- a/exp-sgcheck/pc_common.c +++ b/exp-sgcheck/pc_common.c @@ -326,9 +326,9 @@ void pc_pp_Error ( Error* err ) emit( " <auxwhat>Address %#lx expected vs actual:</auxwhat>\n", xe->XE.SorG.addr ); - emiN( " <auxwhat>Expected: %t</auxwhat>\n", + emiN( " <auxwhat>Expected: %pS</auxwhat>\n", &xe->XE.SorG.expect[0] ); - emiN( " <auxwhat>Actual: %t</auxwhat>\n", + emiN( " <auxwhat>Actual: %pS</auxwhat>\n", &xe->XE.SorG.actual[0] ); } else { @@ -433,14 +433,14 @@ void pc_pp_Error ( Error* err ) if (xml) { if (xe->XE.Heap.descr1) - emiN( " %t\n", + emiN( " %pS\n", (HChar*)VG_(indexXA)( xe->XE.Heap.descr1, 0 ) ); if (xe->XE.Heap.descr2) - emiN( " %t\n", + emiN( " %pS\n", (HChar*)VG_(indexXA)( xe->XE.Heap.descr2, 0 ) ); if (xe->XE.Heap.datasym[0] != 0) emiN( " <auxwhat>Address 0x%llx is %llu bytes " - "inside data symbol \"%t\"</auxwhat>\n", + "inside data symbol \"%pS\"</auxwhat>\n", (ULong)xe->XE.Heap.addr, (ULong)xe->XE.Heap.datasymoff, xe->XE.Heap.datasym ); diff --git a/exp-sgcheck/tests/bad_percentify.c b/exp-sgcheck/tests/bad_percentify.c index 94f5559..7b93c8b 100644 --- a/exp-sgcheck/tests/bad_percentify.c +++ b/exp-sgcheck/tests/bad_percentify.c @@ -447,12 +447,23 @@ VG_(debugLog_vprintf) ( ret += myvprintf_int64(send, send_arg2, flags, 10, width, False, (ULong)(va_arg (vargs, UInt))); break; - case 'p': /* %p */ - ret += 2; - send('0',send_arg2); - send('x',send_arg2); - ret += myvprintf_int64(send, send_arg2, flags, 16, width, True, - (ULong)((UWord)va_arg (vargs, void *))); + case 'p': + if (format[i+1] == 'S') { + i++; + /* %pS, like %s but escaping chars for XML safety */ + /* Note: simplistic; ignores field width and flags */ + char *str = va_arg (vargs, char *); + if (str == (char*) 0) + str = "(null)"; + ret += myvprintf_str_XML_simplistic(send, send_arg2, str); + } else { + /* %p */ + ret += 2; + send('0',send_arg2); + send('x',send_arg2); + ret += myvprintf_int64(send, send_arg2, flags, 16, width, True, + (ULong)((UWord)va_arg (vargs, void *))); + } break; case 'x': /* %x */ case 'X': /* %X */ @@ -480,13 +491,6 @@ VG_(debugLog_vprintf) ( flags, width, str, format[i]=='S'); break; } - case 't': { /* %t, like %s but escaping chars for XML safety */ - /* Note: simplistic; ignores field width and flags */ - char *str = va_arg (vargs, char *); - if (str == (char*) 0) str = "(null)"; - ret += myvprintf_str_XML_simplistic(send, send_arg2, str); - break; - } // case 'y': { /* %y - print symbol */ // Char buf[100]; diff --git a/helgrind/hg_errors.c b/helgrind/hg_errors.c index 010f5e0..d7bc6ef 100644 --- a/helgrind/hg_errors.c +++ b/helgrind/hg_errors.c @@ -755,13 +755,6 @@ static void emit ( HChar* format, ... ) emit_WRK(format, vargs); va_end(vargs); } -static void emit_no_f_c ( HChar* format, ... ) -{ - va_list vargs; - va_start(vargs, format); - emit_WRK(format, vargs); - va_end(vargs); -} /* Announce (that is, print the point-of-creation) of 'thr'. Only do @@ -1052,8 +1045,8 @@ void HG_(pp_Error) ( Error* err ) if (xml) { emit( " <xwhat>\n" ); - emit_no_f_c( - " <text>Thread #%d's call to %t failed</text>\n", + emit( + " <text>Thread #%d's call to %pS failed</text>\n", (Int)xe->XE.PthAPIerror.thr->errmsg_index, xe->XE.PthAPIerror.fnname ); emit( " <hthreadid>%d</hthreadid>\n", @@ -1065,7 +1058,7 @@ void HG_(pp_Error) ( Error* err ) } else { - emit_no_f_c( "Thread #%d's call to %t failed\n", + emit( "Thread #%d's call to %pS failed\n", (Int)xe->XE.PthAPIerror.thr->errmsg_index, xe->XE.PthAPIerror.fnname ); emit( " with error code %ld (%s)\n", diff --git a/include/pub_tool_libcprint.h b/include/pub_tool_libcprint.h index 1621851..5cd4e76 100644 --- a/include/pub_tool_libcprint.h +++ b/include/pub_tool_libcprint.h @@ -86,28 +86,18 @@ extern UInt VG_(printf) ( const HChar *format, ... ) extern UInt VG_(vprintf) ( const HChar *format, va_list vargs ) PRINTF_CHECK(1, 0); -// The "_no_f_c" functions here are just like their non-"_no_f_c" counterparts -// but without the PRINTF_CHECK, so they can be used with our non-standard %t -// format specifier. - -// These are the same as the non "_xml" versions above, except the -// output goes on the selected XML output channel instead of the -// normal one. extern UInt VG_(printf_xml) ( const HChar *format, ... ) PRINTF_CHECK(1, 2); extern UInt VG_(vprintf_xml) ( const HChar *format, va_list vargs ) PRINTF_CHECK(1, 0); -extern UInt VG_(printf_xml_no_f_c) ( const HChar *format, ... ); - /* Yet another, totally general, version of vprintf, which hands all output bytes to CHAR_SINK, passing it OPAQUE as the second arg. */ extern void VG_(vcbprintf)( void(*char_sink)(HChar, void* opaque), void* opaque, const HChar* format, va_list vargs ); -extern UInt VG_(message_no_f_c)( VgMsgKind kind, const HChar* format, ... ); extern UInt VG_(message)( VgMsgKind kind, const HChar* format, ... ) PRINTF_CHECK(2, 3); diff --git a/include/pub_tool_xarray.h b/include/pub_tool_xarray.h index b7c646e..69a8eac 100644 --- a/include/pub_tool_xarray.h +++ b/include/pub_tool_xarray.h @@ -138,15 +138,10 @@ extern void VG_(getContentsXA_UNSAFE)( XArray* sr, /* Convenience function: printf into an XArray of HChar, adding stuff at the end. This is very convenient for concocting arbitrary length printf output in an XArray. Note that the resulting string - is NOT zero-terminated. Versions are provided with and without a - format check, the latter so the unknown (to gcc) "%t" can be used - without gcc complaining. */ + is NOT zero-terminated. */ extern void VG_(xaprintf)( XArray* dst, const HChar* format, ... ) PRINTF_CHECK(2, 3); -extern void VG_(xaprintf_no_f_c) - ( XArray* dst, const HChar* format, ... ); - #endif // __PUB_TOOL_XARRAY_H /*--------------------------------------------------------------------*/ diff --git a/memcheck/mc_errors.c b/memcheck/mc_errors.c index 4e2e019..36d3595 100644 --- a/memcheck/mc_errors.c +++ b/memcheck/mc_errors.c @@ -349,7 +349,7 @@ static void mc_pp_AddrInfo ( Addr a, AddrInfo* ai, Bool maybe_gcc ) case Addr_DataSym: emiN( "%sAddress 0x%llx is %llu bytes " - "inside data symbol \"%t\"%s\n", + "inside data symbol \"%pS\"%s\n", xpre, (ULong)a, (ULong)ai->Addr.DataSym.offset, @@ -372,7 +372,7 @@ static void mc_pp_AddrInfo ( Addr a, AddrInfo* ai, Bool maybe_gcc ) break; case Addr_SectKind: - emiN( "%sAddress 0x%llx is in the %t segment of %t%s\n", + emiN( "%sAddress 0x%llx is in the %pS segment of %pS%s\n", xpre, (ULong)a, VG_(pp_SectKind)(ai->Addr.SectKind.kind), @@ -460,7 +460,7 @@ void MC_(pp_Error) ( Error* err ) // the following code is untested. Bad. if (xml) { emit( " <kind>CoreMemError</kind>\n" ); - emiN( " <what>%t contains unaddressable byte(s)</what>\n", + emiN( " <what>%pS contains unaddressable byte(s)</what>\n", VG_(get_error_string)(err)); VG_(pp_ExeContext)( VG_(get_error_where)(err) ); } else { @@ -518,7 +518,7 @@ void MC_(pp_Error) ( Error* err ) MC_(any_value_errors) = True; if (xml) { emit( " <kind>SyscallParam</kind>\n" ); - emiN( " <what>Syscall param %t contains " + emiN( " <what>Syscall param %pS contains " "uninitialised byte(s)</what>\n", VG_(get_error_string)(err) ); VG_(pp_ExeContext)( VG_(get_error_where)(err) ); @@ -540,7 +540,7 @@ void MC_(pp_Error) ( Error* err ) MC_(any_value_errors) = True; if (xml) { emit( " <kind>SyscallParam</kind>\n" ); - emiN( " <what>Syscall param %t points to %s byte(s)</what>\n", + emiN( " <what>Syscall param %pS points to %s byte(s)</what>\n", VG_(get_error_string)(err), extra->Err.MemParam.isAddrErr ? "unaddressable" : "uninitialised" ); @@ -669,7 +669,7 @@ void MC_(pp_Error) ( Error* err ) emit( " <kind>Overlap</kind>\n" ); if (extra->Err.Overlap.szB == 0) { emiN( " <what>Source and destination overlap " - "in %t(%#lx, %#lx)\n</what>\n", + "in %pS(%#lx, %#lx)\n</what>\n", VG_(get_error_string)(err), extra->Err.Overlap.dst, extra->Err.Overlap.src ); } else { @@ -682,7 +682,7 @@ void MC_(pp_Error) ( Error* err ) VG_(pp_ExeContext)( VG_(get_error_where)(err) ); } else { if (extra->Err.Overlap.szB == 0) { - emiN( "Source and destination overlap in %t(%#lx, %#lx)\n", + emiN( "Source and destination overlap in %pS(%#lx, %#lx)\n", VG_(get_error_string)(err), extra->Err.Overlap.dst, extra->Err.Overlap.src ); } else { |
|
From: Florian K. <br...@ac...> - 2011-10-05 04:09:36
|
On 10/04/2011 12:54 PM, Bart Van Assche wrote: > In order to support escaping XML meta-characters several formatting > functions in Valgrind support the %t format specifier. That's an unfortunate > choice since gcc's format specification checker doesn't recognize %t. My > proposal is to use the format specifier %pS instead of %t and to remove the > _no_f_c variants of the formatting functions, as is done in the patch below. > It would be good to be able to compile with -Wformat at some point. This patch is a step in that direction. However, %p is only meant for printing pointers to void -- according to c99. And we'd be passing in pointers to characters. I did a quick test with GCC 4.4.3 and 3.4.6 and those were benign and did not complain. Although I do remember a compiler issuing warnings about non-void pointers being provided as argument for %p. Perhaps it was EDG... What does clang 2.9 say? Will it let it slide by? Florian |
|
From: Bart V. A. <bva...@ac...> - 2011-10-05 06:05:37
|
On Wed, Oct 5, 2011 at 6:09 AM, Florian Krohm <br...@ac...> wrote: > On 10/04/2011 12:54 PM, Bart Van Assche wrote: > > In order to support escaping XML meta-characters several formatting > > functions in Valgrind support the %t format specifier. That's an unfortunate > > choice since gcc's format specification checker doesn't recognize %t. My > > proposal is to use the format specifier %pS instead of %t and to remove the > > _no_f_c variants of the formatting functions, as is done in the patch below. > > > > It would be good to be able to compile with -Wformat at some point. This > patch is a step in that direction. Does that mean that you are not aware that -Wformat is already enabled in the Valgrind project via -Wall and via the PRINTF_CHECK(x, y) macro ? > However, %p is only meant for printing pointers to void -- according to > c99. And we'd be passing in pointers to characters. I did a quick test > with GCC 4.4.3 and 3.4.6 and those were benign and did not complain. > Although I do remember a compiler issuing warnings about non-void > pointers being provided as argument for %p. Perhaps it was EDG... > What does clang 2.9 say? Will it let it slide by? I propose that we address this issue as soon as we encounter a compiler that complains about passing char* to %p. Bart. |
|
From: Florian K. <br...@ac...> - 2011-10-05 13:02:37
|
On 10/05/2011 02:05 AM, Bart Van Assche wrote: > >> However, %p is only meant for printing pointers to void -- according to >> c99. And we'd be passing in pointers to characters. I did a quick test >> with GCC 4.4.3 and 3.4.6 and those were benign and did not complain. >> Although I do remember a compiler issuing warnings about non-void >> pointers being provided as argument for %p. Perhaps it was EDG... >> What does clang 2.9 say? Will it let it slide by? > > I propose that we address this issue as soon as we encounter a > compiler that complains about passing char* to %p. > That is fine. GCC seems OK. How about clang. Does it complain? Florian |
|
From: Bart V. A. <bva...@ac...> - 2011-10-05 15:04:33
|
On Wed, Oct 5, 2011 at 3:02 PM, Florian Krohm <br...@ac...> wrote: > > On 10/05/2011 02:05 AM, Bart Van Assche wrote: > > > >> However, %p is only meant for printing pointers to void -- according to > >> c99. And we'd be passing in pointers to characters. I did a quick test > >> with GCC 4.4.3 and 3.4.6 and those were benign and did not complain. > >> Although I do remember a compiler issuing warnings about non-void > >> pointers being provided as argument for %p. Perhaps it was EDG... > >> What does clang 2.9 say? Will it let it slide by? > > > > I propose that we address this issue as soon as we encounter a > > compiler that complains about passing char* to %p. > > That is fine. GCC seems OK. How about clang. Does it complain? I don't have access to a system with clang unfortunately.. But according to another paragraph I found in the C99 spec, passing char * / const char * to %p is fine: <quote> A pointer to void shall have the same representation and alignment requirements as a pointer to a character type. Similarly, pointers to qualified or unqualified versions of compatible types shall have the same representation and alignment requirements. </quote> Bart. |
|
From: Florian K. <br...@ac...> - 2011-10-06 12:00:05
|
On 10/05/2011 11:04 AM, Bart Van Assche wrote: > On Wed, Oct 5, 2011 at 3:02 PM, Florian Krohm <br...@ac...> wrote: >> >> That is fine. GCC seems OK. How about clang. Does it complain? > > I don't have access to a system with clang unfortunately.. But > according to another paragraph I found in the C99 spec, passing char * > / const char * to %p is fine: > > <quote> > A pointer to void shall have the same representation and alignment > requirements as a > pointer to a character type. Similarly, pointers to qualified or > unqualified versions of > compatible types shall have the same representation and alignment requirements. > </quote> > Right. And because of that it would make perfect sense to also allow a character pointer for %p. But for some strange reason that is not so. Section 7.19.6.1 ..... description of format specifiers... p The argument shall be a pointer to void. Which would give an overly zealous compiler the right to warn. Hopefully, clang won't do that either. Florian |
|
From: Bart V. A. <bva...@ac...> - 2011-10-06 15:06:33
|
On Thu, Oct 6, 2011 at 1:59 PM, Florian Krohm <br...@ac...> wrote: > On 10/05/2011 11:04 AM, Bart Van Assche wrote: > > On Wed, Oct 5, 2011 at 3:02 PM, Florian Krohm <br...@ac...> wrote: > >> > >> That is fine. GCC seems OK. How about clang. Does it complain? > > > > I don't have access to a system with clang unfortunately.. But > > according to another paragraph I found in the C99 spec, passing char * > > / const char * to %p is fine: > > > > <quote> > > A pointer to void shall have the same representation and alignment > > requirements as a > > pointer to a character type. Similarly, pointers to qualified or > > unqualified versions of > > compatible types shall have the same representation and alignment > requirements. > > </quote> > > > > Right. And because of that it would make perfect sense to also allow a > character pointer for %p. But for some strange reason that is not so. > > Section 7.19.6.1 > > ..... description of format specifiers... > > p The argument shall be a pointer to void. > > Which would give an overly zealous compiler the right to warn. > Even if we would ever switch to a compiler that warns about passing char* to %p, I would still prefer to insert a cast in front of the argument over the code duplication we have now with the _no_f_c functions. By the way, I have tried to build Valgrind with clang, and clang doesn't complain about passing char * or const char * to %p. But it prints a huge number of other warnings, and several regression tests fail to compile. Bart. |