From: Sascha W. <wi...@sh...> - 2005-05-18 15:35:27
|
Hello *, this are some small fixes I made to get x86-arch.c compile with gcc-4.0.0. As all tests get passed, I think I didn't break it... cheers sascha ps. please cc me in any reply, I'm not subscribed to this list. Index: x86-arch.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvsroot/sbcl/sbcl/src/runtime/x86-arch.c,v retrieving revision 1.27 diff -c -r1.27 x86-arch.c *** x86-arch.c 9 Sep 2004 12:10:15 -0000 1.27 --- x86-arch.c 18 May 2005 14:27:54 -0000 *************** *** 85,91 **** vlen =3D *(char*)(*os_context_pc_addr(context))++; /* Skip Lisp error arg data bytes. */ while (vlen-- > 0) { ! ( (char*)(*os_context_pc_addr(context)) )++; } break; =20 --- 85,91 ---- vlen =3D *(char*)(*os_context_pc_addr(context))++; /* Skip Lisp error arg data bytes. */ while (vlen-- > 0) { ! (*os_context_pc_addr(context)) +=3D sizeof(char); } break; =20 *************** *** 265,276 **** break; =20 case trap_Breakpoint: ! (char*)(*os_context_pc_addr(context)) -=3D 1; handle_breakpoint(signal, info, context); break; =20 case trap_FunEndBreakpoint: ! (char*)(*os_context_pc_addr(context)) -=3D 1; *os_context_pc_addr(context) =3D (int)handle_fun_end_breakpoint(signal, info, context); break; --- 265,276 ---- break; =20 case trap_Breakpoint: ! *os_context_pc_addr(context) -=3D sizeof(char); handle_breakpoint(signal, info, context); break; =20 case trap_FunEndBreakpoint: ! *os_context_pc_addr(context) -=3D sizeof(char); *os_context_pc_addr(context) =3D (int)handle_fun_end_breakpoint(signal, info, context); break; --=20 Sascha Wilde=20 - no sig today... sorry! |
From: Nikodemus S. <nik...@ra...> - 2005-05-18 16:00:39
|
On Wed, 18 May 2005, Sascha Wilde wrote: > this are some small fixes I made to get x86-arch.c compile with gcc-4.0.0. > As all tests get passed, I think I didn't break it... > ! ( (char*)(*os_context_pc_addr(context)) )++; changing to > ! (*os_context_pc_addr(context)) += sizeof(char); Um, is this just gcc-4.0 brokenness, or is there an actual correctness issue here. As far as I can tell the first one is quite legal -- not that I've though either long or deep about it. Cheers, -- Nikodemus Schemer: "Buddha is small, clean, and serious." Lispnik: "Buddha is big, has hairy armpits, and laughs." |
From: Daniel B. <da...@te...> - 2005-05-18 16:30:06
|
Nikodemus Siivola <nik...@ra...> writes: >> this are some small fixes I made to get x86-arch.c compile with gcc-4.0.0. >> As all tests get passed, I think I didn't break it... > >> ! ( (char*)(*os_context_pc_addr(context)) )++; > > changing to > >> ! (*os_context_pc_addr(context)) += sizeof(char); > > Um, is this just gcc-4.0 brokenness, or is there an actual correctness > issue here. As far as I can tell the first one is quite legal -- not > that I've though either long or deep about it. I've not reviewed this code recently, but my memory is that in that situation we really /do/ want to increment by one octet. That said, my understanding is also that sizeof(char) is by definition 1 in C anyway, so I really don't understand the goal of this change. Did this change in C99 or something? -dan |
From: Jachym H. <fr...@li...> - 2005-05-18 16:56:45
|
Hello, > >> this are some small fixes I made to get x86-arch.c compile with gcc-4.0.0. > >> As all tests get passed, I think I didn't break it... > > > >> ! ( (char*)(*os_context_pc_addr(context)) )++; > > > > changing to > > > >> ! (*os_context_pc_addr(context)) += sizeof(char); > > > > Um, is this just gcc-4.0 brokenness, or is there an actual correctness > > issue here. As far as I can tell the first one is quite legal -- not > > that I've though either long or deep about it. > > I've not reviewed this code recently, but my memory is that in that > situation we really /do/ want to increment by one octet. That said, > my understanding is also that sizeof(char) is by definition 1 in C > anyway, so I really don't understand the goal of this change. Did > this change in C99 or something? Given that os_context_register_t seems to always be an integer data type (intptr_t, really?), (*os_context_pc_addr(context)) += 1; would look best I think. It might be enlightening to see gcc4's exact complaint -- the original code, despite confusing, seems correct to me as well. Anyway, this is just a quick glance, I may easily be missing something. Regards, -- Jachym Holecek |
From: Sascha W. <wi...@sh...> - 2005-05-18 17:21:39
|
On Wed, May 18, 2005 at 05:31:55PM +0100, Daniel Barlow wrote: > Nikodemus Siivola <nik...@ra...> writes: >=20 > >> this are some small fixes I made to get x86-arch.c compile with gcc-4.= 0.0. > >> As all tests get passed, I think I didn't break it... > > > >> ! ( (char*)(*os_context_pc_addr(context)) )++; > > > > changing to > > > >> ! (*os_context_pc_addr(context)) +=3D sizeof(char); > > > > Um, is this just gcc-4.0 brokenness, or is there an actual correctness > > issue here. As far as I can tell the first one is quite legal -- not > > that I've though either long or deep about it. >=20 > I've not reviewed this code recently, but my memory is that in that > situation we really /do/ want to increment by one octet. That said, > my understanding is also that sizeof(char) is by definition 1 in C > anyway, so I really don't understand the goal of this change. Did > this change in C99 or something? As far as I can tell, what the original code made work were a gcc extension called "cast-as-lvalue" which was deprecated in 3.3.4 and 3.4 and now finally removed <http://gcc.gnu.org/gcc-4.0/changes.html> So it seems it never were ansi C code -- but I'm no expert in that kind of esoteric standard stuff.. Anyway it's true, that sizeof(char) is one by definition, and maybe it should be replaced by it (the compiler will do it anyway) -- the reason I wrote it that way, was to preserve the semantics of the original code. :-) cheers sascha --=20 Sascha Wilde Hauptfunktion einer GUI ist es IMHO, die dadurch verlorene Zeit durch einen h=F6heren Spa=DF-Faktor zu kompensieren. Essentiell ein Computerspiel. -- Rainer Weikusat in d.c.o.u.d |
From: Ingvar <in...@ca...> - 2005-05-19 09:48:10
|
Sascha writes: > On Wed, May 18, 2005 at 05:31:55PM +0100, Daniel Barlow wrote: > > Nikodemus Siivola <nikodemus=40random-state.net> writes: > >=20 > > >> this are some small fixes I made to get x86-arch.c compile with gc= c-4.0.0. > > >> As all tests get passed, I think I didn't break it... > > > > > >> =21 ( (char*)(*os_context_pc_addr(context)) )++; > > > > > > changing to > > > > > >> =21 (*os_context_pc_addr(context)) +=3D sizeof(char); > > > > > > Um, is this just gcc-4.0 brokenness, or is there an actual correctn= ess > > > issue here. As far as I can tell the first one is quite legal -- no= t > > > that I've though either long or deep about it. > >=20 > > I've not reviewed this code recently, but my memory is that in that > > situation we really /do/ want to increment by one octet. That said, > > my understanding is also that sizeof(char) is by definition 1 in C > > anyway, so I really don't understand the goal of this change. Did > > this change in C99 or something? >=20 > As far as I can tell, what the original code made work were a gcc > extension called =22cast-as-lvalue=22 which was deprecated in 3.3.4 and= > 3.4 and now finally removed <http://gcc.gnu.org/gcc-4.0/changes.html> >=20 > So it seems it never were ansi C code -- but I'm no expert in that > kind of esoteric standard stuff.. >=20 > Anyway it's true, that sizeof(char) is one by definition, and maybe it > should be replaced by it (the compiler will do it anyway) -- the > reason I wrote it that way, was to preserve the semantics of the > original code. :-) Well, adding to (or incrementing) a pointer in C will take it to an align= ment=20 suitable for the next value of whatever type the pointer is. So, if we have a pointer, whose value is 0 and it's a char*, adding 1 wil= l=20 result in it pointing at 1. If it's a pointer to a 16-bit integer, it wil= l be=20 pointing at 2. For a 32-bit pointer, it wil be pointing at 4. For a point= er to=20 a struct taking 256 bytes, it will (most probably) be 256. So, if the original intent was to get a 1-byte offset from os_context_pc_= addr,=20 the code as-changed-to will be, urm, amusingly broken, unless that is a c= har*=20 and if it were, just removing the cast from the original code would (prob= ably) be better. To convince C programmers that the above-stated poinetr arithmetic rules = are=20 correct, remember that a=5Bb=5D is defined as being *identical* to *(a+b)= and that=20 (for any pointer to an array a) a=5B0=5D is the first element, a=5B1=5D t= he second and=20 so on, without overlap. //ingvar |
From: Sascha W. <wi...@sh...> - 2005-05-19 10:50:03
|
On Thu, May 19, 2005 at 10:47:53AM +0100, Ingvar wrote: > Sascha writes: > > On Wed, May 18, 2005 at 05:31:55PM +0100, Daniel Barlow wrote: > > > Nikodemus Siivola <nik...@ra...> writes: > > >=20 > > > >> this are some small fixes I made to get x86-arch.c compile with gc= c-4.0.0. > > > > > > > >> ! ( (char*)(*os_context_pc_addr(context)) )++; > > > > > > > > changing to > > > > > > > >> ! (*os_context_pc_addr(context)) +=3D sizeof(char); > > > > > > > > Um, is this just gcc-4.0 brokenness, or is there an actual correctn= ess > > > > issue here. As far as I can tell the first one is quite legal -- not > > > > that I've though either long or deep about it. [...] > > As far as I can tell, what the original code made work were a gcc > > extension called "cast-as-lvalue" which was deprecated in 3.3.4 and > > 3.4 and now finally removed <http://gcc.gnu.org/gcc-4.0/changes.html> > >=20 > > So it seems it never were ansi C code -- but I'm no expert in that > > kind of esoteric standard stuff.. > >=20 > > Anyway it's true, that sizeof(char) is one by definition, and maybe it > > should be replaced by it (the compiler will do it anyway) -- the > > reason I wrote it that way, was to preserve the semantics of the > > original code. :-) >=20 > Well, adding to (or incrementing) a pointer in C will take it to an align= ment=20 > suitable for the next value of whatever type the pointer is. >=20 > So, if we have a pointer, whose value is 0 and it's a char*, adding 1 wil= l=20 > result in it pointing at 1. If it's a pointer to a 16-bit integer, it wil= l be=20 > pointing at 2. For a 32-bit pointer, it wil be pointing at 4. For a point= er to=20 > a struct taking 256 bytes, it will (most probably) be 256. true, but ... > So, if the original intent was to get a 1-byte offset from os_context_pc_= addr,=20 > the code as-changed-to will be, urm, amusingly broken, unless that is a c= har*=20 > and if it were, just removing the cast from the original code would (prob= ably) > be better. =2E.. the original code wasn't intended to get a 1-byte offset from os_context_pc_addr but from *os_context_pc_addr (whish is the value os_context_pc_addr is pointing to, used as a pointer) and as of the fact that it were casted to char* we have the most simple case of "normal" arithmetic. Despite all the magic ((char*)(*foo))++; is fully equivalent to *foo++; with the only difference, that the later is legal standard c. =20 If it would have been=20 ((long*)(*foo))++; the translation would have been *foo +=3D sizeof(long); the form I used in my original patch to "preserve semantics"... cheers sascha --=20 Sascha Wilde Life's too short to read boring signatures |
From: James Y K. <fo...@fu...> - 2005-05-18 16:39:50
|
On May 18, 2005, at 12:00 PM, Nikodemus Siivola wrote: > On Wed, 18 May 2005, Sascha Wilde wrote: > > >> this are some small fixes I made to get x86-arch.c compile with >> gcc-4.0.0. >> As all tests get passed, I think I didn't break it... >> > > >> ! ( (char*)(*os_context_pc_addr(context)) )++; >> > > changing to > > >> ! (*os_context_pc_addr(context)) += sizeof(char); >> > > Um, is this just gcc-4.0 brokenness, or is there an actual > correctness issue here. As far as I can tell the first one is quite > legal -- not that I've though either long or deep about it. Using a cast as an lvalue is illegal in C, but is a gcc extension. gcc4 removes the extension. However, sizeof(char) is always 1 according to the C specification. No point in ever writing that. So it really should simply be: (*os_context_pc_addr(context)))++. os_context_pc_addr should always return a pointer to an integer value, so there was never really any point in that cast. James |
From: Thiemo S. <th...@ne...> - 2005-05-18 16:56:18
|
James Y Knight wrote: [snip] > >>! (*os_context_pc_addr(context)) += sizeof(char); > > > >Um, is this just gcc-4.0 brokenness, or is there an actual > >correctness issue here. As far as I can tell the first one is quite > >legal -- not that I've though either long or deep about it. > > Using a cast as an lvalue is illegal in C, but is a gcc extension. > gcc4 removes the extension. > > However, sizeof(char) is always 1 according to the C specification. > No point in ever writing that. So it really should simply be: > (*os_context_pc_addr(context)))++. > os_context_pc_addr should always return a pointer to an integer > value, so there was never really any point in that cast. Or even ++*os_context_pc_addr(context); Thiemo |
From: Sascha W. <wi...@sh...> - 2005-05-19 09:47:49
|
On Wed, May 18, 2005 at 05:35:14PM +0200, Sascha Wilde wrote: > this are some small fixes I made to get x86-arch.c compile with gcc-4.0.0. after reading the replies, here a simplified version of the patch. cheers sascha =20 ps. please cc me in any reply, I'm not subscribed to this list. Index: src/runtime/x86-arch.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvsroot/sbcl/sbcl/src/runtime/x86-arch.c,v retrieving revision 1.27 diff -u -r1.27 x86-arch.c --- src/runtime/x86-arch.c 9 Sep 2004 12:10:15 -0000 1.27 +++ src/runtime/x86-arch.c 19 May 2005 08:27:30 -0000 @@ -85,7 +85,7 @@ vlen =3D *(char*)(*os_context_pc_addr(context))++; /* Skip Lisp error arg data bytes. */ while (vlen-- > 0) { - ( (char*)(*os_context_pc_addr(context)) )++; + ++*os_context_pc_addr(context); } break; =20 @@ -265,12 +265,12 @@ break; =20 case trap_Breakpoint: - (char*)(*os_context_pc_addr(context)) -=3D 1; + --*os_context_pc_addr(context); handle_breakpoint(signal, info, context); break; =20 case trap_FunEndBreakpoint: - (char*)(*os_context_pc_addr(context)) -=3D 1; + --*os_context_pc_addr(context); *os_context_pc_addr(context) =3D (int)handle_fun_end_breakpoint(signal, info, context); break; --=20 Sascha Wilde : The most exciting phrase to hear in science, the one : that heralds new discoveries, is not "Eureka!" (I found : it!) but "That's funny ..." -- Isaac Asimov |
From: Sascha W. <wi...@sh...> - 2005-05-26 11:21:07
|
On Thu, May 19, 2005 at 11:47:36AM +0200, Sascha Wilde wrote: > On Wed, May 18, 2005 at 05:35:14PM +0200, Sascha Wilde wrote: > > this are some small fixes I made to get x86-arch.c compile with gcc-4.0= =2E0. >=20 > after reading the replies, here a simplified version of the patch. > [...] With no further reply/objection on this small patch for about a week, could you please consider committing it to the CVS? Please CC me on any reply, I'm not subscribed to this list. Cheers sascha --=20 Sascha Wilde : "GUIs normally make it simple to accomplish simple=20 : actions and impossible to accomplish complex actions." : (Doug Gwyn - 22/Jun/91 in comp.unix.wizards) |
From: Juho S. <js...@ik...> - 2005-05-30 05:30:25
|
On Thu, May 19, 2005 at 11:47:36AM +0200, Sascha Wilde wrote: > On Wed, May 18, 2005 at 05:35:14PM +0200, Sascha Wilde wrote: > > this are some small fixes I made to get x86-arch.c compile with gcc-4.0.0. > > after reading the replies, here a simplified version of the patch. Thanks, committed in 0.9.1.8. (I also applied the same changes to x86-64-arch.c). -- Juho Snellman |
From: Sascha W. <wi...@sh...> - 2005-05-30 07:44:51
|
On Mon, May 30, 2005 at 08:29:11AM +0300, Juho Snellman wrote: > On Thu, May 19, 2005 at 11:47:36AM +0200, Sascha Wilde wrote: > > On Wed, May 18, 2005 at 05:35:14PM +0200, Sascha Wilde wrote: > > > this are some small fixes I made to get x86-arch.c compile with gcc-4.0.0. > > > > after reading the replies, here a simplified version of the patch. > > Thanks, committed in 0.9.1.8. (I also applied the same changes to > x86-64-arch.c). Thanks! btw. is it possible, that the sourceforge ro CVS is sometimes a bit out of sync? Just did a cvs up, but still got revision 1.27 of src/runtime/x86-arch.c for head... cheers sascha -- Sascha Wilde No sig today. |
From: Nikodemus S. <nik...@ra...> - 2005-05-30 09:33:44
|
On Mon, 30 May 2005, Sascha Wilde wrote: > btw. is it possible, that the sourceforge ro CVS is sometimes a bit > out of sync? Just did a cvs up, but still got revision 1.27 of Nor sometimes, always: the sourceforge anoncvs has 24h or so lag. Cheers, -- Nikodemus Schemer: "Buddha is small, clean, and serious." Lispnik: "Buddha is big, has hairy armpits, and laughs." |