|
From: <sv...@va...> - 2007-10-01 10:33:41
|
Author: dirk
Date: 2007-10-01 11:33:41 +0100 (Mon, 01 Oct 2007)
New Revision: 6928
Log:
fix a few format string warnings
Modified:
trunk/coregrind/m_signals.c
trunk/coregrind/m_stacktrace.c
Modified: trunk/coregrind/m_signals.c
===================================================================
--- trunk/coregrind/m_signals.c 2007-10-01 10:21:07 UTC (rev 6927)
+++ trunk/coregrind/m_signals.c 2007-10-01 10:33:41 UTC (rev 6928)
@@ -431,7 +431,7 @@
Int sig;
VG_(printf)("\n\nSKSS:\n");
for (sig = 1; sig <= _VKI_NSIG; sig++) {
- VG_(printf)("sig %d: handler 0x%x, flags 0x%x\n", sig,
+ VG_(printf)("sig %d: handler %p, flags 0x%x\n", sig,
skss.skss_per_sig[sig].skss_handler,
skss.skss_per_sig[sig].skss_flags );
Modified: trunk/coregrind/m_stacktrace.c
===================================================================
--- trunk/coregrind/m_stacktrace.c 2007-10-01 10:21:07 UTC (rev 6927)
+++ trunk/coregrind/m_stacktrace.c 2007-10-01 10:33:41 UTC (rev 6928)
@@ -158,7 +158,7 @@
fp = (((UWord*)fp)[0]);
ips[i++] = ip;
if (debug)
- VG_(printf)(" ipsF[%d]=%08p\n", i-1, ips[i-1]);
+ VG_(printf)(" ipsF[%d]=%p\n", i-1, ips[i-1]);
ip = ip - 1;
continue;
}
@@ -168,7 +168,7 @@
if ( VG_(use_CF_info)( &ip, &sp, &fp, fp_min, fp_max ) ) {
ips[i++] = ip;
if (debug)
- VG_(printf)(" ipsC[%d]=%08p\n", i-1, ips[i-1]);
+ VG_(printf)(" ipsC[%d]=%p\n", i-1, ips[i-1]);
ip = ip - 1;
continue;
}
@@ -406,7 +406,7 @@
# endif
if (0)
- VG_(printf)("tid %d: stack_highest=%p ip=%p sp=%p fp=%p\n",
+ VG_(printf)("tid %d: stack_highest=0x%08lx ip=0x%08lx sp=0x%08lx fp=0x%08lx\n",
tid, stack_highest_word, ip, sp, fp);
return VG_(get_StackTrace2)(tid, ips, n_ips, ip, sp, fp, lr, sp,
|
|
From: Bart V. A. <bar...@gm...> - 2007-10-01 12:31:11
|
Hello Dirk,
Do you really know what you are doing ? You are not just fixing warnings,
you are changing code. In trunk/coregrind/m_stacktrace.c you changed the
format string %08p into %p. As a result, the printed pointers are no longer
right-aligned. Did you understand what I wrote in the mail I sent on
September 26 to the valgrind-developers mailing list ?
Bart.
On 10/1/07, sv...@va... <sv...@va...> wrote:
>
> Author: dirk
> Date: 2007-10-01 11:33:41 +0100 (Mon, 01 Oct 2007)
> New Revision: 6928
>
> Log:
> fix a few format string warnings
>
> Modified:
> trunk/coregrind/m_signals.c
> trunk/coregrind/m_stacktrace.c
>
>
> Modified: trunk/coregrind/m_signals.c
> ===================================================================
> --- trunk/coregrind/m_signals.c 2007-10-01 10:21:07 UTC (rev 6927)
> +++ trunk/coregrind/m_signals.c 2007-10-01 10:33:41 UTC (rev 6928)
> @@ -431,7 +431,7 @@
> Int sig;
> VG_(printf)("\n\nSKSS:\n");
> for (sig = 1; sig <= _VKI_NSIG; sig++) {
> - VG_(printf)("sig %d: handler 0x%x, flags 0x%x\n", sig,
> + VG_(printf)("sig %d: handler %p, flags 0x%x\n", sig,
> skss.skss_per_sig[sig].skss_handler,
> skss.skss_per_sig[sig].skss_flags );
>
>
> Modified: trunk/coregrind/m_stacktrace.c
> ===================================================================
> --- trunk/coregrind/m_stacktrace.c 2007-10-01 10:21:07 UTC (rev 6927)
> +++ trunk/coregrind/m_stacktrace.c 2007-10-01 10:33:41 UTC (rev 6928)
> @@ -158,7 +158,7 @@
> fp = (((UWord*)fp)[0]);
> ips[i++] = ip;
> if (debug)
> - VG_(printf)(" ipsF[%d]=%08p\n", i-1, ips[i-1]);
> + VG_(printf)(" ipsF[%d]=%p\n", i-1, ips[i-1]);
> ip = ip - 1;
> continue;
> }
> @@ -168,7 +168,7 @@
> if ( VG_(use_CF_info)( &ip, &sp, &fp, fp_min, fp_max ) ) {
> ips[i++] = ip;
> if (debug)
> - VG_(printf)(" ipsC[%d]=%08p\n", i-1, ips[i-1]);
> + VG_(printf)(" ipsC[%d]=%p\n", i-1, ips[i-1]);
> ip = ip - 1;
> continue;
> }
> @@ -406,7 +406,7 @@
> # endif
>
> if (0)
> - VG_(printf)("tid %d: stack_highest=%p ip=%p sp=%p fp=%p\n",
> + VG_(printf)("tid %d: stack_highest=0x%08lx ip=0x%08lx sp=0x%08lx
> fp=0x%08lx\n",
> tid, stack_highest_word, ip, sp, fp);
>
> return VG_(get_StackTrace2)(tid, ips, n_ips, ip, sp, fp, lr, sp,
>
|
|
From: Dirk M. <dm...@gm...> - 2007-10-04 21:18:27
|
On Monday, 1. October 2007, Bart Van Assche wrote: > you changed the > format string %08p into %p. As a result, the printed pointers are no longer > right-aligned. True. It was my impression that %p does this by default, but it seems I was wrong. I'll correct this mistake shortly, thanks for pointing it out. > Did you understand what I wrote in the mail I sent on > September 26 to the valgrind-developers mailing list ? Yes, I was able to understand it, thanks for asking again. Greetings, Dirk |
|
From: Nicholas N. <nj...@cs...> - 2007-10-03 01:30:11
|
On Mon, 1 Oct 2007, Bart Van Assche wrote: > Do you really know what you are doing ? You are not just fixing warnings, > you are changing code. In trunk/coregrind/m_stacktrace.c you changed the > format string %08p into %p. As a result, the printed pointers are no longer > right-aligned. Did you understand what I wrote in the mail I sent on > September 26 to the valgrind-developers mailing list ? I'm wondering what is the right thing to do here. Having the format string warnings is nice, but we should be consistent. Eg. when printing pointers, should we use %p and cast to (void*) when necessary, instead of using 0x%lx? Julian, what do you think? Nick |
|
From: Julian S. <js...@ac...> - 2007-10-03 10:22:19
|
> I'm wondering what is the right thing to do here. Having the format string
> warnings is nice, but we should be consistent. Eg. when printing pointers,
> should we use %p and cast to (void*) when necessary, instead of using
> 0x%lx?
In the long run fixing the format strings to keep gcc happy seems to me
a good thing to do. Most of the fixing required is in reconciling
Addr/Word/UWord args with %p formats, either by casting the arg to void*
or changing the format to 0x%lx. Just occasionally gcc reports something
more interesting, about too few arguments, or a fundamental type
mismatch (32-bit int vs word-size int vs 64-bit int, etc).
My concerns are:
(1) that the output is not changed at all
(2) it's a lot of boring work. Just a build for amd64-linux alone
generates 1438 warnings that need to be fixed.
(3) it's not worth doing unless we can be sure to get rid of all
warnings when built with all versions of gcc we care about.
Because I don't want to end up in a situation where a supposedly
clean build still produces warnings. At least for valgrind 3.2.X
I tried to ensure it would still build and work on RedHat 7.3, which
uses gcc-2.96, so that's a pretty old compiler.
Given these, and particularly (3), possibly a good solution is to do it
on a svn branch, and, if it looks ok, merge it. After all, that is what
branches are for.
J
|
|
From: Bart V. A. <bar...@gm...> - 2007-10-03 10:54:39
|
On 10/3/07, Julian Seward <js...@ac...> wrote: > > > > I'm wondering what is the right thing to do here. Having the format > string > > warnings is nice, but we should be consistent. Eg. when printing > pointers, > > should we use %p and cast to (void*) when necessary, instead of using > > 0x%lx? > > In the long run fixing the format strings to keep gcc happy seems to me > a good thing to do. Most of the fixing required is in reconciling > Addr/Word/UWord args with %p formats, either by casting the arg to void* > or changing the format to 0x%lx. Just occasionally gcc reports something > more interesting, about too few arguments, or a fundamental type > mismatch (32-bit int vs word-size int vs 64-bit int, etc). > > My concerns are: > > (1) that the output is not changed at all > > (2) it's a lot of boring work. Just a build for amd64-linux alone > generates 1438 warnings that need to be fixed. > > (3) it's not worth doing unless we can be sure to get rid of all > warnings when built with all versions of gcc we care about. > Because I don't want to end up in a situation where a supposedly > clean build still produces warnings. At least for valgrind 3.2.X > I tried to ensure it would still build and work on RedHat 7.3, which > uses gcc-2.96, so that's a pretty old compiler. > > Given these, and particularly (3), possibly a good solution is to do it > on a svn branch, and, if it looks ok, merge it. After all, that is what > branches are for. > > J > I agree with the concerns raised above. But regarding (3): how about the warnings gcc emits on format strings like "%,d", which are supported by Valgrind but which are not allowed by ANSI C ? I'm afraid every version of gcc that supports format string checking will emit a warning on such format strings. For an example of code that uses this kind of format specifications, see e.g. memcheck/mc_malloc_wrappers.c. Bart. |
|
From: Julian S. <js...@ac...> - 2007-10-03 11:07:09
|
> I agree with the concerns raised above. But regarding (3): how about the > warnings gcc emits on format strings like "%,d", which are supported by > Valgrind but which are not allowed by ANSI C ? I wondered about that too, but at least using gcc 4.1.2 I got no complaints about "%,d". So it seems ok. Do you have a setup/gcc version in which it does complain about "%,d" ? J |
|
From: Bart V. A. <bar...@gm...> - 2007-10-03 11:39:11
|
On 10/3/07, Julian Seward <js...@ac...> wrote: > > > > I agree with the concerns raised above. But regarding (3): how about the > > warnings gcc emits on format strings like "%,d", which are supported by > > Valgrind but which are not allowed by ANSI C ? > > I wondered about that too, but at least using gcc 4.1.2 I got no > complaints > about "%,d". So it seems ok. Do you have a setup/gcc version in which it > does complain about "%,d" ? > > J > The trend with gcc is that new versions move closer to the ANSI standard. Since the comma is not a valid flag character in ANSI C, we can expect that a future gcc version will complain about it. Do you think it would be a good idea to modify the Valgrind source code such that the formatting functions adhere strictly to the ANSI C standard and to remove extensions like the insertion of comma's as a thousands separator ? I'm in favor of this -- I find it confusing that Valgrind's format specifications differ slightly from those in the C language. Additionally, as far as I know there is currently no documentation available for Valgrind's format specifications. For more about format strings, see also paragraph 7.19.6.1 (The fprintf function) in http://std.dkuug.dk/JTC1/SC22/WG14/www/docs/n843.pdf Bart. |
|
From: Nicholas N. <nj...@cs...> - 2007-10-03 23:33:15
|
On Wed, 3 Oct 2007, Bart Van Assche wrote: > The trend with gcc is that new versions move closer to the ANSI standard. > Since the comma is not a valid flag character in ANSI C, we can expect that > a future gcc version will complain about it. Do you think it would be a good > idea to modify the Valgrind source code such that the formatting functions > adhere strictly to the ANSI C standard and to remove extensions like the > insertion of comma's as a thousands separator ? I'm in favor of this -- I > find it confusing that Valgrind's format specifications differ slightly from > those in the C language. Additionally, as far as I know there is currently > no documentation available for Valgrind's format specifications. Making things more ANSI would be good, but then the ',' prefix is also extremely useful -- it inserts commas into printed numbers, eg. prints "1,234,567" instead of "1234567". Any ideas for how to get the best of both worlds? Nick |
|
From: Oswald B. <os...@kd...> - 2007-10-04 08:58:55
|
On Thu, Oct 04, 2007 at 09:31:28AM +1000, Nicholas Nethercote wrote: > Making things more ANSI would be good, but then the ',' prefix is also > extremely useful -- it inserts commas into printed numbers, eg. prints > "1,234,567" instead of "1234567". Any ideas for how to get the best of both > worlds? > this one isn't particularly hard, because you just got the "elevation" of the sign wrong - the susv2 grouping char is '. unlike in the standard, you don't need to take the locale into account to make (newer versions of) gcc happy ... -- Hi! I'm a .signature virus! Copy me into your ~/.signature, please! -- Chaos, panic, and disorder - my work here is done. |
|
From: Bart V. A. <bar...@gm...> - 2007-10-04 10:26:34
|
On 10/4/07, Oswald Buddenhagen <os...@kd...> wrote: > > On Thu, Oct 04, 2007 at 09:31:28AM +1000, Nicholas Nethercote wrote: > > Making things more ANSI would be good, but then the ',' prefix is also > > extremely useful -- it inserts commas into printed numbers, eg. prints > > "1,234,567" instead of "1234567". Any ideas for how to get the best of > both > > worlds? > > > this one isn't particularly hard, because you just got the "elevation" > of the sign wrong - the susv2 grouping char is '. unlike in the > standard, you don't need to take the locale into account to make (newer > versions of) gcc happy ... > This is an interesting observation. I have found confirmation of the fact that the apostrophe character is a thousands group separator on the following URL (Single UNIX Specification, Version 2): http://www.opengroup.org/onlinepubs/007908775/xsh/fprintf.html Bart. |
|
From: Dirk M. <dm...@gm...> - 2007-10-04 21:30:58
|
On Thursday, 4. October 2007, Nicholas Nethercote wrote: > Making things more ANSI would be good My primary concern was to ensure that no format string mismatches occur that could cause strange misbehaviour or crashes on rare (possible not regression tested) scenarios. > , but then the ',' prefix is also > extremely useful -- it inserts commas into printed numbers, eg. prints > "1,234,567" instead of "1234567". Any ideas for how to get the best of > both worlds? There is one death to die. For now, we can keep the ',' prefix as long as gcc does not complain about it. Otherwise we have to either overload the meaning of one of the ANSI format string characters with it - e.g. '00' could mean comma insertion instead of zero extension, or remove the flag alltogether. the only fully ANSI correct solution would be to use %s and wrap the actual integer in a "comma_convert_decimal()" helper routine which formats the number and returns a pointer to a string instead. Greetings, Dirk |
|
From: Dirk M. <dm...@gm...> - 2007-10-04 21:32:41
|
On Wednesday, 3. October 2007, Julian Seward wrote:
> Addr/Word/UWord args with %p formats, either by casting the arg to void*
> or changing the format to 0x%lx.
I'm currently unable to decide which route to take. We can represent Addr in
two ways:
VG_(printf)("%#lx", ..)
or
VG_(printf)("%p", (void*)..)
I would prefer the first, given that it is shorter. Any comments? I'll make
sure that I change it consistently (in a new branch, like requested).
Greetings,
Dirk
|
|
From: Nicholas N. <nj...@cs...> - 2007-10-05 01:14:35
|
On Thu, 4 Oct 2007, Dirk Mueller wrote:
> On Wednesday, 3. October 2007, Julian Seward wrote:
>
>> Addr/Word/UWord args with %p formats, either by casting the arg to void*
>> or changing the format to 0x%lx.
>
> I'm currently unable to decide which route to take. We can represent Addr in
> two ways:
>
> VG_(printf)("%#lx", ..)
>
> or
> VG_(printf)("%p", (void*)..)
>
> I would prefer the first, given that it is shorter. Any comments? I'll make
> sure that I change it consistently (in a new branch, like requested).
I prefer the second, because I find it easier to remember. Also, AFAICT
Valgrind's 'printf' doesn't handle '#'.
Nick
|
|
From: Nicholas N. <nj...@cs...> - 2007-10-05 01:20:16
|
On Thu, 4 Oct 2007, Dirk Mueller wrote: > My primary concern was to ensure that no format string mismatches occur that > could cause strange misbehaviour or crashes on rare (possible not regression > tested) scenarios. I've been annoyed several times in my recent Massif2 work with incorrect format strings, so automatic checking is good. >> , but then the ',' prefix is also >> extremely useful -- it inserts commas into printed numbers, eg. prints >> "1,234,567" instead of "1234567". Any ideas for how to get the best of >> both worlds? > > There is one death to die. For now, we can keep the ',' prefix as long as gcc > does not complain about it. Otherwise we have to either overload the meaning > of one of the ANSI format string characters with it - e.g. '00' could mean > comma insertion instead of zero extension, or remove the flag alltogether. > > the only fully ANSI correct solution would be to use %s and wrap the actual > integer in a "comma_convert_decimal()" helper routine which formats the > number and returns a pointer to a string instead. It sounds like changing it from a ',' to an apostrophe will fix any problem there. Dirk, are you happy to make that change along with the others in the branch? The relevant code is in coregrind/m_debuglog.c. Nick |
|
From: Dirk M. <dm...@gm...> - 2007-10-05 10:00:55
|
On Friday, 5. October 2007, Nicholas Nethercote wrote: > It sounds like changing it from a ',' to an apostrophe will fix any problem > there. Dirk, are you happy to make that change along with the others in > the branch? The relevant code is in coregrind/m_debuglog.c. sure, I'm more wondering what to do with "%t". in valgrind ,this is an XML-escaped "%s", while for printf its a length modifier, which causes quite some warnings. do we aim for thread safety? otherwise a xml_quote() around the argument sounds like the easiest solution for me. Greetings, Dirk |
|
From: Dirk M. <dm...@gm...> - 2007-10-05 10:09:37
|
On Friday, 5. October 2007, Dirk Mueller wrote: > sure, I'm more wondering what to do with "%t". in valgrind ,this is an > XML-escaped "%s", while for printf its a length modifier, which causes > quite some warnings. > > do we aim for thread safety? otherwise a xml_quote() around the argument > sounds like the easiest solution for me. alternatively we could encode "%t" as "%.s". this is an apparently non-sensical construct (print zero characters of the string) that we could overload with this meaning. Dirk |
|
From: Julian S. <js...@ac...> - 2007-10-05 10:16:47
|
> alternatively we could encode "%t" as "%.s". this is an apparently > non-sensical construct (print zero characters of the string) that we could > overload with this meaning. I prefer that; doing a lot of xml_quote everywhere is hassle. gcc will not complain about "%.s" ? At the end of this it would be good to put in a comment somewhere, a short description of the nonstandard format extensions we now support. |