Re: [PATCH] print format does not match argument type

Re: [PATCH] print format does not match argument type

[Roland Schwingel]

Hi...

Yesterday I could not finish this... Sorry...

 >  > But for now, if your suggestion works (Roland will hopefully
 >  > verify),
 >  > then I say we go with that for the time being.
%llx appears to work (tested it on win32,win64 and linux 32bit).
We can even remove the cast at all. I got no warning here.

So ok... Who is doing the change?

Roland

RE: [PATCH] print format does not match argument type

[Patrick Monnerat]

Roland Schwingel wrote:
> %llx appears to work (tested it on win32,win64 and linux 32bit).
> We can even remove the cast at all. I got no warning here.

No warning, but you may have a wrong value converted at run-time: this
is the case if the %llx descriptor uses a value larger than the one that
gets pushed and your're running on a big-endian machine or the bytes
above the pushed value are not zero...
I would really keep the (unsigned long long) cast, at least for run-time
purpose.

> So ok... Who is doing the change?

Not me: I'm not a committer ;-))

Cheers,
Patrick

Re: [PATCH] print format does not match argument type

[Roland Schwingel]

Hi...

Keith Seitz <keiths@redhat.com> wrote on 10.04.2012 18:27:42:

 > On 04/10/2012 09:08 AM, Patrick Monnerat wrote:
 > > I think I've found the universal (well: almost!) coding:
 > >
 > > Use format "%llx" and cast arg to (unsigned long long): This is
 > > C99 and supported by MSVC, at least since 2005. Not OK for C89 :-(
MS is not really caring about standards ... for native printf on windows 
(as deliverd in msvcrt.dll) "%llx" is not supported.

 > But for now, if your suggestion works (Roland will hopefully verify),
 > then I say we go with that for the time being.
I am already testing but seeing a "funny" hang now when opening
the register window when using %llx.

Still evaluating. Have to see what xstrprintf() does exactly.
But as far as I can see at the moment it appears that vasprintf is 
getting handled by libiberty in gdb on windows.

Roland

BTW: Insight is not built using MSVC on windows, but with gcc...

Re: [PATCH] print format does not match argument type

[Roland Schwingel]

Hi Patrick,

insight-owner@sourceware.org wrote on 05.04.2012 18:34:27:
 > In gdb/gdbtk/generic/gdbtk-register.c, 2 statements try to use a print
 > format of "%lx" for a size_t argument. The correctness of such
 > statements is architecture dependent. If warnings are treated as
 > errors,the compilation fails.
Which warnings do you get on which platform?

The casts to size_t where introduced by myself some days ago.
Previously they where casted to long.

The TYPE_FIELD_TYPE macro returns a pointer to a struct type.
Treating this as (unsigned) long is pretty correct on *nix style
systems but not on windows. On 64bit windows a long is still 4 bytes
long, so the downcasted value is simply wrong.

Roland

RE: [PATCH] print format does not match argument type

[Patrick Monnerat]

 
Roland Schwingel wrote:

Hi Roland,
Thanks for having paid attention to my report.

> Which warnings do you get on which platform?

Running on a Fedora Linux 64bit, but building for a 32bit (using mock).
Thus this is a 32bit build on a (emulated) 32bit machine.

When compiling with the default fedora rpm build flags, the following
occurs:

gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom
-fasynchronous-unwind-tables   -I. -I. -I./common -I./config
-DLOCALEDIR="\"/usr/share/locale\"" -DHAVE_CONFIG_H
-I./../include/opcode -I./../opcodes/..  -I../bfd -I./../bfd
-I./../include -I../libdecnumber -I./../libdecnumber  -I./gnulib
-Ignulib   -DTUI=1 -DGDBTK  -Wall -Wdeclaration-after-statement
-Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign -Wno-unused
-Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts
-Wmissing-prototypes -Werror -c -o gdbtk-register.o -MT gdbtk-register.o
-MMD -MP -MF .deps/gdbtk-register.Tpo -I./../libgui/src
-I/usr/include -I/usr/include/tcl-private/generic -I/usr/include
-I/usr/include/tk-private/generic
-DGDBTK_LIBRARY=\"/usr/share/insight\"
-DSRC_DIR=\"/builddir/build/BUILD/insight-7.4.50/gdb\"
./gdbtk/generic/gdbtk-register.c
cc1: warnings being treated as errors
./gdbtk/generic/gdbtk-register.c: In function 'get_register_types':
./gdbtk/generic/gdbtk-register.c:242:4: error: format '%lx' expects type
'long unsigned int', but argument 2 has type 'unsigned int'
./gdbtk/generic/gdbtk-register.c:258:7: error: format '%lx' expects type
'long unsigned int', but argument 2 has type 'unsigned int'


I don't know how W$ works, but in any case, "%lx" targets an unsigned
long, whatever its real size is. The patch I've provided retains the
conversion to size_t (for eventual truncation reasons), but finally
converts the value to the type expected by the format.

If the solution I provide is not OK on some platform, then a
conditionally defined print format should be used, IMHO...

Cheers,
Patrick

RE: [PATCH] print format does not match argument type

[Patrick Monnerat]

 
Roland Schwingel wrote:

> The TYPE_FIELD_TYPE macro returns a pointer to a struct type.
> Treating this as (unsigned) long is pretty correct on *nix style
> systems but not on windows. On 64bit windows a long is still 4 bytes
> long, so the downcasted value is simply wrong.

I think I've found the universal (well: almost!) coding:

Use format "%llx" and cast arg to (unsigned long long): This is C99 and
supported by MSVC, at least since 2005. Not OK for C89 :-(

I hope it helps.

Patrick

Re: [PATCH] print format does not match argument type

[Keith Seitz]

On 04/10/2012 09:08 AM, Patrick Monnerat wrote:

> I think I've found the universal (well: almost!) coding:
>
> Use format "%llx" and cast arg to (unsigned long long): This is C99 and
> supported by MSVC, at least since 2005. Not OK for C89 :-(

To be honest, I didn't even know that we did this. [The register code is 
some of the oldest code in insight.] Passing host memory addresses into 
the interpreter and back again as a string is just plain evil.

The C code should create a map/hashtable and store the pointers to the 
types itself; then pass a string representation of the key into the 
interpreter.

But for now, if your suggestion works (Roland will hopefully verify), 
then I say we go with that for the time being.

Thank you!
Keith

RE: [PATCH] print format does not match argument type

[Pierre Muller]

> -----Message d'origine-----
> De : insight-owner@sourceware.org [mailto:insight-owner@sourceware.org] De
> la part de Keith Seitz
> Envoyé : mardi 10 avril 2012 18:28
> À : Patrick Monnerat
> Cc : insight@sourceware.org; roland@onevision.com
> Objet : Re: [PATCH] print format does not match argument type
> 
> On 04/10/2012 09:08 AM, Patrick Monnerat wrote:
> 
> > I think I've found the universal (well: almost!) coding:
> >
> > Use format "%llx" and cast arg to (unsigned long long): This is C99 and
> > supported by MSVC, at least since 2005. Not OK for C89 :-(
> 
> To be honest, I didn't even know that we did this. [The register code is
> some of the oldest code in insight.] Passing host memory addresses into
> the interpreter and back again as a string is just plain evil.
> 
> The C code should create a map/hashtable and store the pointers to the
> types itself; then pass a string representation of the key into the
> interpreter.
> 
> But for now, if your suggestion works (Roland will hopefully verify),
> then I say we go with that for the time being.
  The gdb/utils.c function called host_address_to_string
was created to allow printing of host pointer
without need to bother with %llX or typecasts...

It does add a '0x' at the start of the 
generated pchar, but I don't think that this is a big problem.

This second field (ar[1]) is probably added to avoid confusion between
type with same name...

Should we use this patch?

Pierre Muller


ChangeLog entry:

2012-04-13  Pierre Muller  <muller@ics.u-strasbg.fr>

       * generic/gdbtk-register.c (get_register_types): Use
       host_address_to_string function to avoid use of "%lx" and
       typecasts.


cvs diff: Diffing generic
Index: generic/gdbtk-register.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtk/generic/gdbtk-register.c,v
retrieving revision 1.42
diff -u -p -r1.42 gdbtk-register.c
--- generic/gdbtk-register.c    30 Mar 2012 07:36:10 -0000      1.42
+++ generic/gdbtk-register.c    13 Apr 2012 14:42:56 -0000
@@ -239,7 +239,8 @@ get_register_types (int regnum, map_arg
        {
          Tcl_Obj *ar[3], *list;
          char *buff;
-         buff = xstrprintf ("%lx", (size_t)TYPE_FIELD_TYPE (reg_vtype, i));
+         buff = xstrprintf ("%s", host_address_to_string (
+                                    TYPE_FIELD_TYPE (reg_vtype, i)));
          ar[0] = Tcl_NewStringObj (TYPE_FIELD_NAME (reg_vtype, i), -1);
          ar[1] = Tcl_NewStringObj (buff, -1);
          if (TYPE_CODE (TYPE_FIELD_TYPE (reg_vtype, i)) == TYPE_CODE_FLT)
@@ -255,7 +256,7 @@ get_register_types (int regnum, map_arg
     {
       Tcl_Obj *ar[3], *list;
       char *buff;
-      buff = xstrprintf ("%lx", (size_t)reg_vtype);
+      buff = xstrprintf ("%s", host_address_to_string (reg_vtype));
       ar[0] = Tcl_NewStringObj (TYPE_NAME(reg_vtype), -1);
       ar[1] = Tcl_NewStringObj (buff, -1);
       if (TYPE_CODE (reg_vtype) == TYPE_CODE_FLT)
cvs diff: Diffing library
cvs diff: Diffing library/help
cvs diff: Diffing library/help/images
cvs diff: Diffing library/help/trace
cvs diff: Diffing library/images
cvs diff: Diffing library/images2
cvs diff: Diffing plugins
cvs diff: Diffing plugins/intel-pentium
cvs diff: Diffing plugins/rhabout

Re: [PATCH] print format does not match argument type

[Keith Seitz]

On 04/13/2012 07:52 AM, Pierre Muller wrote:
>    The gdb/utils.c function called host_address_to_string
> was created to allow printing of host pointer
> without need to bother with %llX or typecasts...
>
> It does add a '0x' at the start of the
> generated pchar, but I don't think that this is a big problem.
>
> This second field (ar[1]) is probably added to avoid confusion between
> type with same name...
>
> Should we use this patch?

Yes, thank you for pointing that out. I just *knew* there had to be a 
way to do this "safely." [As safe as it can be to pass host pointers 
into the interpreter at least.]

Keith

RE: [PATCH] print format does not match argument type

[Pierre Muller]

> -----Message d'origine-----
> De : insight-owner@sourceware.org [mailto:insight-owner@sourceware.org] De
> la part de Keith Seitz
> Envoyé : vendredi 13 avril 2012 17:17
> À : Pierre Muller
> Cc : 'Patrick Monnerat'; insight@sourceware.org; roland@onevision.com
> Objet : Re: [PATCH] print format does not match argument type
> 
> On 04/13/2012 07:52 AM, Pierre Muller wrote:
> >    The gdb/utils.c function called host_address_to_string
> > was created to allow printing of host pointer
> > without need to bother with %llX or typecasts...
> >
> > It does add a '0x' at the start of the
> > generated pchar, but I don't think that this is a big problem.
> >
> > This second field (ar[1]) is probably added to avoid confusion between
> > type with same name...
> >
> > Should we use this patch?
> 
> Yes, thank you for pointing that out. I just *knew* there had to be a
> way to do this "safely." [As safe as it can be to pass host pointers
> into the interpreter at least.]


Patch committed in
http://sourceware.org/ml/gdb-cvs/2012-04/msg00086.html

Thanks 

Pierre

[PATCH] print format does not match argument type

[Patrick Monnerat]

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

In gdb/gdbtk/generic/gdbtk-register.c, 2 statements try to use a print
format of "%lx" for a size_t argument. The correctness of such
statements is architecture dependent. If warnings are treated as errors,
the compilation fails.
 
The attached patch fixes these statements by unconditionally converting
the size_t argument to an unsigned long.
 
Cheers,
Patrick

[-- Attachment #2: insight-7.4.50-sizesizet.patch --]
[-- Type: application/octet-stream, Size: 1072 bytes --]

diff -Naur insight-7.4.50.orig/gdb/gdbtk/generic/gdbtk-register.c insight-7.4.50.new/gdb/gdbtk/generic/gdbtk-register.c
--- insight-7.4.50.orig/gdb/gdbtk/generic/gdbtk-register.c	2012-03-30 09:36:10.000000000 +0200
+++ insight-7.4.50.new/gdb/gdbtk/generic/gdbtk-register.c	2012-04-05 18:03:20.875181085 +0200
@@ -239,7 +239,8 @@
 	{
 	  Tcl_Obj *ar[3], *list;
 	  char *buff;
-	  buff = xstrprintf ("%lx", (size_t)TYPE_FIELD_TYPE (reg_vtype, i));
+	  buff = xstrprintf ("%lx", (unsigned long)
+				    (size_t)TYPE_FIELD_TYPE (reg_vtype, i));
 	  ar[0] = Tcl_NewStringObj (TYPE_FIELD_NAME (reg_vtype, i), -1);
 	  ar[1] = Tcl_NewStringObj (buff, -1);
 	  if (TYPE_CODE (TYPE_FIELD_TYPE (reg_vtype, i)) == TYPE_CODE_FLT)
@@ -255,7 +256,7 @@
     {
       Tcl_Obj *ar[3], *list;
       char *buff;
-      buff = xstrprintf ("%lx", (size_t)reg_vtype);
+      buff = xstrprintf ("%lx", (unsigned long)(size_t)reg_vtype);
       ar[0] = Tcl_NewStringObj (TYPE_NAME(reg_vtype), -1);
       ar[1] = Tcl_NewStringObj (buff, -1);
       if (TYPE_CODE (reg_vtype) == TYPE_CODE_FLT)