[-- 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)
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
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
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
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
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...
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
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
> -----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
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
> -----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