From: Chris S. <ir0...@gm...> - 2009-07-27 17:16:43
|
I'm almost ready to release a new mingwrt to address the issues with GCC 4.4.0. Keith, one of the changes I would like to commit is the one you supplied a patch for: https://sourceforge.net/tracker/?func=detail&atid=302435&aid=2009559&group_id=2435 I have verified that it works correctly for Cygwin. Would you like me to commit it, or would you prefer to? Cheers! Chris -- Chris Sutcliffe http://emergedesktop.org |
From: Keith M. <kei...@us...> - 2009-07-27 21:19:13
|
On Monday 27 July 2009 18:16:33 Chris Sutcliffe wrote: > I'm almost ready to release a new mingwrt to address the issues > with GCC 4.4.0. > > Keith, one of the changes I would like to commit is the one you > supplied a patch for: > > https://sourceforge.net/tracker/?func=detail&atid=302435&aid=2009559&group_id=2435 > > I have verified that it works correctly for Cygwin. Would you > like me to commit it, or would you prefer to? I've done it, Chris. I noticed you made some changes to mingwex/stdio/pformat.c, to avoid some warnings with gcc-4.4. I was curious, so I had a look: @@ -734,7 +734,8 @@ * his `__gdtoa()' function in a manner to provide extended precision * replacements for `ecvt()' and `fcvt()'. */ - unsigned int k, e = 0; char *ep; + int k; + unsigned int e = 0; char *ep; static FPI fpi = { 64, 1-16383-64+1, 32766-16383-64+1, FPI_Round_near, 0 }; /* Classify the argument into an appropriate `__gdtoa()' category... Okay. Did the __gdtoa() prototype change, with the last round of updates, or did I just have a signed/unsigned mismatch first time around? @@ -1816,7 +1817,7 @@ /* Save the current format scan position, so that we can backtrack * in the event of encountering an invalid format specification... */ - char *backtrack = fmt; + const char *backtrack = fmt; /* Restart capture for dynamic field width and precision specs... */ Okay; const makes sense here. @@ -1882,7 +1883,8 @@ * `wchar_t' data, (which is promoted to an `int' argument)... */ argval.__pformat_ullong_t = (wchar_t)(va_arg( argv, int )); - __pformat_wputchars( (wchar_t *)(&argval), 1, &stream ); + void *tmp = &argval; + __pformat_wputchars( (wchar_t *)tmp, 1, &stream ); } else This one just looks plain wrong! I'm guessing that the warning was related to casting from a union with an aggregate size of 64-bits, (to accommodate the unsigned long long element) to a type with a size of only 16-bits. Implicitly casting the address of the union to a void* may circumvent the warning, but it doesn't seem right that it should; (we are still passing the address of a 64-bit aggregate to a function expecting a 16-bit parameter's address). Surely the more correct solution would be to pass the address of an element of the union, which represents an appropriate data type directly. (Of course, whichever solution we adopt, the effect is going to be the same; this is just gcc-4.4 being ultra-pernickity, where we know that the cast is safe, whichever way we write it). -- Regards, Keith. |
From: Chris S. <ir0...@gm...> - 2009-07-28 01:40:00
|
Hey Keith, >> I have verified that it works correctly for Cygwin. Would you >> like me to commit it, or would you prefer to? > > I've done it, Chris. Excellent, thank you! > I noticed you made some changes to mingwex/stdio/pformat.c, to avoid > some warnings with gcc-4.4. I was curious, so I had a look: > > @@ -734,7 +734,8 @@ > * his `__gdtoa()' function in a manner to provide extended precision > * replacements for `ecvt()' and `fcvt()'. > */ > - unsigned int k, e = 0; char *ep; > + int k; > + unsigned int e = 0; char *ep; > static FPI fpi = { 64, 1-16383-64+1, 32766-16383-64+1, FPI_Round_near, 0 }; > > /* Classify the argument into an appropriate `__gdtoa()' category... > > Okay. Did the __gdtoa() prototype change, with the last round of > updates, or did I just have a signed/unsigned mismatch first time > around? The __gdtoa() prototype may have changed with the massive update to the math functions. Either way, as you pointed out, there was signed / unsigned mismatch. > @@ -1882,7 +1883,8 @@ > * `wchar_t' data, (which is promoted to an `int' argument)... > */ > argval.__pformat_ullong_t = (wchar_t)(va_arg( argv, int )); > - __pformat_wputchars( (wchar_t *)(&argval), 1, &stream ); > + void *tmp = &argval; > + __pformat_wputchars( (wchar_t *)tmp, 1, &stream ); > } > > else > > This one just looks plain wrong! I'm guessing that the warning was > related to casting from a union with an aggregate size of 64-bits, > (to accommodate the unsigned long long element) to a type with a size > of only 16-bits. Implicitly casting the address of the union to a > void* may circumvent the warning, but it doesn't seem right that it > should; (we are still passing the address of a 64-bit aggregate to > a function expecting a 16-bit parameter's address). Surely the more > correct solution would be to pass the address of an element of the > union, which represents an appropriate data type directly. (Of > course, whichever solution we adopt, the effect is going to be the > same; this is just gcc-4.4 being ultra-pernickity, where we know > that the cast is safe, whichever way we write it). Agreed, looking at this in the larger scope of this function, and in particular the members of the union, this was not the most elegant appoach to correcting the issue. I've checked in a different fix to address the issue by using the exist void * (__pformat_ptr_t) member of the union and type casting it to wchar_t *. The net result is the same, I think it's just a more elegant implementation. Cheers! Chris -- Chris Sutcliffe http://emergedesktop.org |
From: Keith M. <kei...@us...> - 2009-07-28 19:29:51
Attachments:
pformat.c.patch
|
On Tuesday 28 July 2009 02:32:17 Chris Sutcliffe wrote: > Agreed, looking at this in the larger scope of this function, and > in particular the members of the union, this was not the most > elegant appoach to correcting the issue. I've checked in a > different fix to address the issue by using the exist void * > (__pformat_ptr_t) member of the union and type casting it to > wchar_t *. > > The net result is the same, I think it's just a more elegant > implementation. Sorry Chris, but I'm afraid the net result is *not* the same; your latest patch interprets a wchar_t *value* as if it were a pointer, which is then passed to the output helper as the *address* of that value. This clearly isn't correct. I can't reproduce any warning with gcc-4.2, even with the original pformat.c implementation, and I haven't yet had time to build a cross mingw32-gcc-4.4. Does the attached alternative avoid the warning for your build case? -- Regards, Keith. |
From: Chris S. <ir0...@gm...> - 2009-07-28 22:50:43
|
> Sorry Chris, but I'm afraid the net result is *not* the same; your > latest patch interprets a wchar_t *value* as if it were a pointer, > which is then passed to the output helper as the *address* of that > value. This clearly isn't correct. D'oh, yep, dunno what I was thinking... No more late night patching for me. > I can't reproduce any warning with gcc-4.2, even with the original > pformat.c implementation, and I haven't yet had time to build a > cross mingw32-gcc-4.4. Does the attached alternative avoid the > warning for your build case? Your patch worked like a charm. I can honestly say it never occurred to me to recast argval when setting it in the first place, in fact I didn't know that it was valid to do so, so I've learned something new. Thank you for the patch, would you like to commit it, or would you prefer I do? Cheers! Chris -- Chris Sutcliffe http://emergedesktop.org |
From: Keith M. <kei...@us...> - 2009-07-29 07:42:17
|
On Tuesday 28 July 2009 23:50:32 Chris Sutcliffe wrote: > Your patch worked like a charm. I can honestly say it never > occurred to me to recast argval when setting it in the first > place, in fact I didn't know that it was valid to do so, so I've > learned something new. It doesn't recast it, as such; it creates a completely distint entity which just happens to have the same name, but is visible only within the inner scope. (It also hides the more widely scoped entity, in the inner scope, but we don't need it there). On leaving the inner scope, that redefined replacement entity is destroyed, and the original becomes visible again, (and it remains unchanged). > Thank you for the patch, would you like to commit it, or would you > prefer I do? Tis done. -- Regards, Keith. |
From: Chris S. <ir0...@gm...> - 2009-07-29 12:43:10
|
>> Your patch worked like a charm. I can honestly say it never >> occurred to me to recast argval when setting it in the first >> place, in fact I didn't know that it was valid to do so, so I've >> learned something new. > > It doesn't recast it, as such; it creates a completely distint entity > which just happens to have the same name, but is visible only within > the inner scope. (It also hides the more widely scoped entity, in > the inner scope, but we don't need it there). On leaving the inner > scope, that redefined replacement entity is destroyed, and the > original becomes visible again, (and it remains unchanged). Cool, thank you for the detailed explanation. >> Thank you for the patch, would you like to commit it, or would you >> prefer I do? > > Tis done. Excellent, I'll roll a new release tonight and upload it. Cheers! Chris -- Chris Sutcliffe http://emergedesktop.org |
From: Keith M. <kei...@us...> - 2009-07-29 18:15:34
|
On Wednesday 29 July 2009 13:43:02 Chris Sutcliffe wrote: > Cool, thank you for the detailed explanation. You're welcome. > >> Thank you for the patch, would you like to commit it, or would > >> you prefer I do? > > > > Tis done. Arrrgh! I missed out an asterisk, (as bullet mark), from my ChangeLog entry; if you see this in time, perhaps you would be so kind as to insert it... > Excellent, I'll roll a new release tonight and upload it. ...when you add the record for this. Thanks. -- Keith. |
From: Chris S. <ir0...@gm...> - 2009-07-30 02:09:36
|
> Arrrgh! I missed out an asterisk, (as bullet mark), from my > ChangeLog entry; if you see this in time, perhaps you would be so > kind as to insert it... > >> Excellent, I'll roll a new release tonight and upload it. > > ...when you add the record for this. Thanks. I've added the missing asterisic and have rolled the new tarballs, but it occurred to me that mingw10.dll is missing in the -dev package. I added to the 3.15.2 release after releasing it to accomodate the installer. My question is, given the effort Keith and Chuck put in to the naming scheme, and given how gcc-4.4 is packaged, should I add mingw10.dll to the -dev package again, or simply leave it in the -dll package? Additionally, are we still updating mingw.ini for the installer? Chris -- Chris Sutcliffe http://emergedesktop.org |
From: Keith M. <kei...@us...> - 2009-07-30 16:45:43
|
On Thursday 30 July 2009 03:09:22 Chris Sutcliffe wrote: > I've added the missing asterisic... Thanks, Chris. I didn't mean you to make any special effort to add it; when next you had anything more concrete to add would have been sufficient, but no matter... > I added to the 3.15.2 release after releasing it to accomodate the > installer. My question is, given the effort Keith and Chuck put > in to the naming scheme, and given how gcc-4.4 is packaged, should > I add mingw10.dll to the -dev package again, or simply leave it in > the -dll package? It should be in the -dll package only, and the installer should be updated to accommodate that; (I suppose simply adding it to the file manifest in mingw.ini would be too much wishful thinking). > Additionally, are we still updating mingw.ini for the installer? For the time being, yes please. -- Regards, Keith. |
From: Chris S. <ir0...@gm...> - 2009-07-30 18:42:50
|
>> I added to the 3.15.2 release after releasing it to accomodate the >> installer. My question is, given the effort Keith and Chuck put >> in to the naming scheme, and given how gcc-4.4 is packaged, should >> I add mingw10.dll to the -dev package again, or simply leave it in >> the -dll package? > > It should be in the -dll package only, and the installer should be > updated to accommodate that; (I suppose simply adding it to the file > manifest in mingw.ini would be too much wishful thinking). Unfortunately, as I recall when we investigated this for 3.15.2, the installer expects only one entry for runtime, which is why I ended up adding the dll to the -dev package. >> Additionally, are we still updating mingw.ini for the installer? > > For the time being, yes please. This being the case, should I add the dll to the -dev package again? Chris -- Chris Sutcliffe http://emergedesktop.org |
From: Keith M. <kei...@us...> - 2009-07-31 19:05:55
Attachments:
mingw-5.1.5.patch
|
On Thursday 30 July 2009 19:42:34 Chris Sutcliffe wrote: > > It should be in the -dll package only, and the installer should > > be updated to accommodate that; (I suppose simply adding it to > > the file manifest in mingw.ini would be too much wishful > > thinking). > > Unfortunately, as I recall when we investigated this for 3.15.2, > the installer expects only one entry for runtime, which is why I > ended up adding the dll to the -dev package. Indeed. The installer seems to declare a few very specific single valued variables, then just read their values from mingw.ini; IMO, this is a very poor design restriction, which will definitely not be reproduced in mingw-get. > >> Additionally, are we still updating mingw.ini for the > >> installer? > > > > For the time being, yes please. > > This being the case, should I add the dll to the -dev package > again? No, we should fix the installer, at least to the extent that it adds say a runtimeDLL variable[*], to which you can assign the tarball. [*] Okay, I'm saying this as if it should be trivial. I'm no expert on NSIS, but it looks like it shouldn't be too difficult; the attached patch seems to do the trick, but perhaps someone more experienced with NSIS than I am could review it, before I commit it? -- Regards, Keith. |
From: Chris S. <ir0...@gm...> - 2009-08-02 01:49:19
|
>> This being the case, should I add the dll to the -dev package >> again? > > No, we should fix the installer, at least to the extent that it adds > say a runtimeDLL variable[*], to which you can assign the tarball. > > [*] Okay, I'm saying this as if it should be trivial. I'm no expert > on NSIS, but it looks like it shouldn't be too difficult; the attached > patch seems to do the trick, but perhaps someone more experienced with > NSIS than I am could review it, before I commit it? Fair enough, I'm on vacation for the next week, leaving first thing tomorrow morning. I'll upload the new mingwrt when I get back. Hopefully the installer will be updated by the time I return. Cheers! Chris -- Chris Sutcliffe http://emergedesktop.org |
From: Chris S. <ir0...@gm...> - 2009-08-08 13:47:53
|
>> [*] Okay, I'm saying this as if it should be trivial. I'm no expert >> on NSIS, but it looks like it shouldn't be too difficult; the attached >> patch seems to do the trick, but perhaps someone more experienced with >> NSIS than I am could review it, before I commit it? > > Fair enough, I'm on vacation for the next week, leaving first thing > tomorrow morning. I'll upload the new mingwrt when I get back. > Hopefully the installer will be updated by the time I return. The patch to the installer looks good to me. I'm also ready to upload the updated mingwrt. Should I proceed with the upload and the announcement? Chris -- Chris Sutcliffe http://emergedesktop.org |