Yes. It's probably a diminishing user base, but we do occasionally see a whinge to the effect of "please don't abandon those of us who still use Win95".
But it hasn't diminished to the point of being completely abandoned. Someone gave the value of the MSVCRT_VERSION from ME when I asked on mingw-users for them; it is the reason I tend to look at the exported functions from it.
Should we ``#define utime _utime'' and remove the import declaration?
Looking at MSDN I find that utime() is no longer supported in the compiler (at least documentation wise), VC version 6 was the last mention of it. Looking at the exported functions from ME shows no sign of utime() being exported. So the define is probably warranted in this case.
My principal point here, however, is that, when you are distributing pre-compiled binaries -- as MinGW.org does -- you cannot realistically resolve this at compile time; you need a run-time probe to verify that your compile-time assumptions are valid for the particular version of MSVCRT.DLL which is actually available on the end user's box.
Which is the reason I opened the original ticket in the TCL issues.
/* See [Bug 3354324]: file mtime sets wrong time */
BTW the cmdAH test does not give an error for me if I remove the _USE_32BIT_TIME_T define in tclWinPort.h. Is there a bug in the alternate MSVCR##.DLL? Is the define really needed? Is it the correct fix for the stated issue? Could it be file system related, i.e. FAT32 versus NTFS or some NAT system?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Looking at MSDN I find that utime() is no longer supported in the compiler (at least documentation wise), VC version 6 was the last mention of it. Looking at the exported functions from ME shows no sign of utime() being exported. So the define is probably warranted in this case.
But it is clearly visible, among the exports from msvcrt.dll, on Win7 32-bit:
What I did with utime() is declare the import it if _USE_32BIT_TIME_T is defined else define it to _utime. The fact that it is actually exported from the recent MSVCRT.DLL but not declared in the recent documentation is more flukish.
Hmm... There is an import in moldnames for utime, does it actually work as expected?
I really need to work on the unit testing.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I would recommend getting io.h correct first: wchar.h contains many duplications from other header files, so as soon as io.h and sys/stat.h (and possible others) are fully correct, it's just a matter of copying the right sections over to wchar.h
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Here are some more fixes for io.h, now using the new __CRT_MAYBE_INLINE macro. Please evaluate. I didn't do anything on wchar.h yet, with those fixes io.h seems OK to me now.
As it currently stands, in the git repository, wchar.h exhibits:
An extraneous #endif on line 252, (presumably related to the incorrect annotation of the #else on line 249, which should be !_USE_32BIT_TIME_T, rather than MSVCRT_VERSION < 800).
An illegal implicit type coercion, (from struct _utimbuf32 * to struct _utimbuf *), on line 275.
A malformed (unterminated) C style comment on line 517.
(FWIW, the last of these seems to be responsible for both [#1975] and [#1984]).
Alas, when I correct these, the following trivial test case:
#include<wchar.h>#include<stat.h>
worryingly fails to compile as C++:
$ mingw32-g++ -c foo.cpp
In file included from foo.cpp:2:
/home/keith/mingw32/usr/local/include/sys/stat.h:372: error: expected declaration before '}' token
In file included from foo.cpp:2:
/home/keith/mingw32/usr/local/include/sys/stat.h:371:1: unterminated #ifdef
/home/keith/mingw32/usr/local/include/sys/stat.h:87:1: unterminated #ifndef
/home/keith/mingw32/usr/local/include/sys/stat.h:24:1: unterminated #ifndef
It seems that the compiler is being confused by nested:
extern "C" {
...
}
blocks; it's particularly worrying that the error is not reported, if the order of the two headers is reversed; however, it seems that the error is always raised in sys/stat.h, when it follows wchar.h, even when there are other intervening headers.
A malformed (unterminated) C style comment on line 517.
(FWIW, the last of these seems to be responsible for both [#1975] and [#1984]).
Alas, when I correct these, the following trivial test case:
#include <wchar.h>
#include <stat.h>
worryingly fails to compile as C++:
I've found this issue. It is in stat.h moving extern "C { above the #ifndef _STAT_DEFINED resolves the confusion. I'll provide a patch later today.
in addition to relocating the BEGIN_C_DECLS. Both BEGIN_C_DECLS and END_C_DECLS are defined in _mingw.h; IMO, using them is more elegant, and better serves to document intended purpose).
Unfortunately, while the relocation of BEGIN_C_DECLS resolves the issue for the trivial case, it then just exposes the next fatal inconsistency: in my real setup.cpp, with wchar.h included before sys/stat.h, I see:
$ make upxdir=$HOME/src/exp setup-tool
mingw32-g++ -MMD -MP -c -D DEBUGLEVEL=0x0fff -I ../../src -I ../../src/pkginfo -I ../../tinyxml -g -O2 -Os -o setup.o setup.cpp
In file included from setup.cpp:78:
mkpath.c: In function`int mkdir_recursive(const char*, int)':mkpath.c:348: error: no matching function for call to `_stat64i32::_stat64i32(const char*&, _stat64i32*)'
.../include/wchar.h:601: note: candidates are: _stat64i32::_stat64i32()
.../include/wchar.h:601: note: _stat64i32::_stat64i32(const _stat64i32&)
make[1]: *** [setup.o] Error 1
make: *** [setup-tool] Error 2
whereas, if I include sys/stat.h before wchar.h, this error isn't evident; setup.cpp compiles, and the entire setup-tool builds successfully.
We really must get away from this dreadful software engineering, whereby the same definitions and/or declarations are reproduced in multiple places, with potential for disparity which results in error such as the above. If it can go wrong, it inevitably will. Comments such as:
/* Also in stdio.h - keep in sync */
_CRTIMPint__cdecl__MINGW_NOTHROWfwprintf(FILE*,constwchar_t*,...);...
are indicative of an implementation/design error.
Last edit: Keith Marshall 2013-06-11
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I don't disagree with the BEGIN_C_DECLS/END_C_DECLS pair and have an action item to review all headers in favor of it. I also agree with the idea of code once and include as required but I'm not ready to do that yet and have an action item to review those as well. I'll consider it further when I'm back from vacation if I want these in the 4.0 release.
Another big issue on my plate are unit tests. You'll see in my latest patch attached earlier that I included a change introducing #ifndef __CRT_TESTING__ filter for the #pragma GCC system_header.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
BEGIN_C_DECLS...END_C_DECLS is a cosmetic change. It provides a documentary improvement, which I certainly believe will be worthwhile; however, it is not an imperative functional requirement. Consequently, we can adopt it on a file by file basis, as may be convenient. Completion of that exercise need not be a prerequisite to the release of 4.0; OTOH, there's no compelling reason that the pending release of 4.0 should exclude partial adoption. (FWIW, I see no reason why a simple sed script should not be able to apply it, as an automated wholesale change, but without a rigorous test framework, a more cautious approach may be preferred).
For me, __CRT_TESTING__ is a harmless non-issue. If it facilitates development of a test framework, by all means introduce it; it's another feature which may be introduced file by file, without necessarily delaying the 4.0 release. Once again, I'm confident that, if it's only intended to wrap the GCC header pragma, a simple sed script could provide a wholesale introduction.
The "code once" issue is different. I've provided a patch which illustrates how it may be implemented, in respect of rationalizing disparity between sys/stat.h and wchar.h. In formulating that, I've followed the path of expediency rather than of strict necessity, and I've included FIXME comments to indicate were it may exceed the criterion of "necessity". However, while it may exceed the "necessary" criterion, it does satisfy the "sufficiency" criterion, in respect of avoiding the issues I've reported, in respect of real world usage. OTOH, your proposed "fixes" have addressed minimal, and insufficient test cases; as such, they fail to satisfy the criterion of "sufficiency".
Given the extent of the mess currently facing us, adopting the "code once" philosophy isn't a task we can readily automate; however, neither is it a task that requires wholesale completion as a prerequisite to the 4.0 release. Partial adoption is a move in the right direction; where that move provides a sufficient resolution of an existing issue, such as this, surely that has got to be better than the current mess, and therefore, a desirable inclusion in the 4.0 release.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
We really must get away from this dreadful software engineering, whereby the same definitions and/or declarations are reproduced in multiple places, ...
Case in point, (and absolutely pertinent to this issue): both sys/stat.h and wchar.h have conditional blocks, guarded by _STAT_DEFINED and _WSTAT_DEFINED; both files leave those guards defined, so preventing redefinition of the respective block content by subsequent inclusion of the other file. However, comparing the content of these blocks between the two files, I see significant omissions and inconsistencies in wchar.h w.r.t. sys/stat.h, (and the inconsistencies point to a likelihood that both are incorrect).
Continuing to rely on copy-and-paste, (as we seem to be doing), to keep these in sync is just disgustingly bad software engineering. Such reproduced content should be implemented once only, in a single file, and #included where necessary. Whether that be by selective inclusion (of fragments) from (say) sys/stat.h into wchar.h, or both include from a third source, is immaterial; we must get away from this insanity of ultimately unmaintainable duplicate definitions in multiple files.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
FWIW, I've adjusted my own working tree to resolve my setup.cpp issues. The patch, against 4.0-dev HEAD as of 2013-06-10 at around 23h00, is attached. This eliminates the duplication between wchar.h and sys/stat.h; similarly duplicated content across other files remains unresolved, and potentially inconsistent. Also, I've left a number of FIXMEs to be resolved.
I applied your malformations.diff to a clean checkout of origin/4.0-dev HEAD. Apart from the the relocation of the BEGIN_C_DECLS equivalent fragment within sys/stat.h, it doesn't address a single one of the defects I've reported on this, and related tickets.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've (partially) reworked the mess which is sys/utime.h, to resolve conflicts with wchar.h; the patch, which follows on from my previous wchar.h vs. sys/stat.h changes, is attached.
Note that I think there is some residual redundancy, and I've added a FIXME to that effect. As it stands, it is sufficient to let me build mingw-get again.
Can you provide a test for the issue(s)? I have an issue with the dropping of the _utimbuf structure definition and the MINGW_RUNTIME < 800 shouldn't be used to the assignment of _utimbuf.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I have an issue with the dropping of the _utimbuf structure definition ...
Sorry, I don't understand. I didn't drop it; I #defined it, as an alias for one or other of the only two physical structural entities which have any logical application, viz. __utimbuf32 and __utimbuf64. Why create a physical clone of one or other of these, (as you appear to favour), with the extra maintenance burden that this imposes? Not to mention the additional attendant type casting that this requires because the two names are logically distinct, even though the underlying physical structures are identical? #define avoids both issues, in a simple manner. Simplicity is good; complexity is bad.
MINGW_RUNTIME < 800 shouldn't be used to the assignment of _utimbuf.
Then, you'll need to explain just what practical purpose MSVCRT_VERSION, (which is what my patch uses, and what I assume you mean by MINGW_RUNTIME), actually does serve, because I can't see that it now has any practical value at all. In mingwrt-3.x, I understood it to mean: "I intend to link with MSVCR7x.DLL, MSVCR80.DLL, MSVCR90.DLL, etc., so please expose APIs accordingly". This does not appear to match your interpretation of its intended use; you seem to have emasculated it, such that it now appears, to me, that it does nothing useful at all.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Here is another idea which would reduce the header-file complexity a lot. If msvcrt.def would define aliases for all functions in the form xxx32() or xxx32i64(), then this
switch doesn't have to be made in the header files any more.
Here is a patch file (mingw6.patch) which does exactly this. It only handles
time.h, sys/timeb.h and sys/utime.h. io.h and wchar.h can be done the same way.
Conceptually, I like this idea. However, I think we may need to keep some degree of filtering in the headers, lest we otherwise create a scenario whereby references are resolved via MinGW import library trampolines, and are thus incompatible with (legacy) MSVC, or even with MinGW's own direct DLL linking capability.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
One would need to ensure a more current binutils when building the import library. Also, there is no way to control when and when not to use this method when the newer MSVCRT.DLL contains the actual imports. However, it was my plan to consider this method in version 5.0.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Agreed, but the filtering should be restricted to only disabling functions when the MSVCRT version doesn't match. This idea solves the 'endless loop' problem, and another advantage is that gdb will show explicitly whether a 32-bit or 64-bit function was called, even though the dll might export it without the '32' and/or the source code has it without the '64'.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
But it hasn't diminished to the point of being completely abandoned. Someone gave the value of the MSVCRT_VERSION from ME when I asked on mingw-users for them; it is the reason I tend to look at the exported functions from it.
Looking at MSDN I find that utime() is no longer supported in the compiler (at least documentation wise), VC version 6 was the last mention of it. Looking at the exported functions from ME shows no sign of utime() being exported. So the define is probably warranted in this case.
Which is the reason I opened the original ticket in the TCL issues.
http://sourceforge.net/p/tcl/bugs/4845/
BTW the cmdAH test does not give an error for me if I remove the _USE_32BIT_TIME_T define in tclWinPort.h. Is there a bug in the alternate MSVCR##.DLL? Is the define really needed? Is it the correct fix for the stated issue? Could it be file system related, i.e. FAT32 versus NTFS or some NAT system?
But it is clearly visible, among the exports from msvcrt.dll, on Win7 32-bit:
$ pexports /c/Windows/System32/msvcrt.dll | grep utime _futime _futime32 _futime64 _utime _utime32 _utime64 _wutime _wutime32 _wutime64 utimeI'm not convinced that it's wise to occlude the entry point, with your proposed #define, when both names are clearly exported.
What I did with utime() is declare the import it if _USE_32BIT_TIME_T is defined else define it to _utime. The fact that it is actually exported from the recent MSVCRT.DLL but not declared in the recent documentation is more flukish.
Hmm... There is an import in moldnames for utime, does it actually work as expected?
I really need to work on the unit testing.
wchar.h is still malformed; see [#1975], and especially [#1984].
Related
Issues:
#1975Issues:
#1984I would recommend getting io.h correct first: wchar.h contains many duplications from other header files, so as soon as io.h and sys/stat.h (and possible others) are fully correct, it's just a matter of copying the right sections over to wchar.h
Here are some more fixes for io.h, now using the new __CRT_MAYBE_INLINE macro. Please evaluate. I didn't do anything on wchar.h yet, with those fixes io.h seems OK to me now.
As it currently stands, in the git repository, wchar.h exhibits:
(FWIW, the last of these seems to be responsible for both [#1975] and [#1984]).
Alas, when I correct these, the following trivial test case:
worryingly fails to compile as C++:
It seems that the compiler is being confused by nested:
extern "C" { ... }blocks; it's particularly worrying that the error is not reported, if the order of the two headers is reversed; however, it seems that the error is always raised in sys/stat.h, when it follows wchar.h, even when there are other intervening headers.
Related
Issues:
#1975Issues:
#1984I've found this issue. It is in stat.h moving extern "C { above the #ifndef _STAT_DEFINED resolves the confusion. I'll provide a patch later today.
Related
Issues:
#1975Issues:
#1984Yes, I too found that moving the 'extern "C" {' corrected the the structuring issue; (actually, I replaced:
with:
in addition to relocating the BEGIN_C_DECLS. Both BEGIN_C_DECLS and END_C_DECLS are defined in _mingw.h; IMO, using them is more elegant, and better serves to document intended purpose).
Unfortunately, while the relocation of BEGIN_C_DECLS resolves the issue for the trivial case, it then just exposes the next fatal inconsistency: in my real setup.cpp, with wchar.h included before sys/stat.h, I see:
whereas, if I include sys/stat.h before wchar.h, this error isn't evident; setup.cpp compiles, and the entire setup-tool builds successfully.
We really must get away from this dreadful software engineering, whereby the same definitions and/or declarations are reproduced in multiple places, with potential for disparity which results in error such as the above. If it can go wrong, it inevitably will. Comments such as:
are indicative of an implementation/design error.
Last edit: Keith Marshall 2013-06-11
I don't disagree with the BEGIN_C_DECLS/END_C_DECLS pair and have an action item to review all headers in favor of it. I also agree with the idea of code once and include as required but I'm not ready to do that yet and have an action item to review those as well. I'll consider it further when I'm back from vacation if I want these in the 4.0 release.
Another big issue on my plate are unit tests. You'll see in my latest patch attached earlier that I included a change introducing #ifndef __CRT_TESTING__ filter for the #pragma GCC system_header.
BEGIN_C_DECLS...END_C_DECLS is a cosmetic change. It provides a documentary improvement, which I certainly believe will be worthwhile; however, it is not an imperative functional requirement. Consequently, we can adopt it on a file by file basis, as may be convenient. Completion of that exercise need not be a prerequisite to the release of 4.0; OTOH, there's no compelling reason that the pending release of 4.0 should exclude partial adoption. (FWIW, I see no reason why a simple sed script should not be able to apply it, as an automated wholesale change, but without a rigorous test framework, a more cautious approach may be preferred).
For me,
__CRT_TESTING__is a harmless non-issue. If it facilitates development of a test framework, by all means introduce it; it's another feature which may be introduced file by file, without necessarily delaying the 4.0 release. Once again, I'm confident that, if it's only intended to wrap the GCC header pragma, a simple sed script could provide a wholesale introduction.The "code once" issue is different. I've provided a patch which illustrates how it may be implemented, in respect of rationalizing disparity between sys/stat.h and wchar.h. In formulating that, I've followed the path of expediency rather than of strict necessity, and I've included FIXME comments to indicate were it may exceed the criterion of "necessity". However, while it may exceed the "necessary" criterion, it does satisfy the "sufficiency" criterion, in respect of avoiding the issues I've reported, in respect of real world usage. OTOH, your proposed "fixes" have addressed minimal, and insufficient test cases; as such, they fail to satisfy the criterion of "sufficiency".
Given the extent of the mess currently facing us, adopting the "code once" philosophy isn't a task we can readily automate; however, neither is it a task that requires wholesale completion as a prerequisite to the 4.0 release. Partial adoption is a move in the right direction; where that move provides a sufficient resolution of an existing issue, such as this, surely that has got to be better than the current mess, and therefore, a desirable inclusion in the 4.0 release.
Case in point, (and absolutely pertinent to this issue): both sys/stat.h and wchar.h have conditional blocks, guarded by _STAT_DEFINED and _WSTAT_DEFINED; both files leave those guards defined, so preventing redefinition of the respective block content by subsequent inclusion of the other file. However, comparing the content of these blocks between the two files, I see significant omissions and inconsistencies in wchar.h w.r.t. sys/stat.h, (and the inconsistencies point to a likelihood that both are incorrect).
Continuing to rely on copy-and-paste, (as we seem to be doing), to keep these in sync is just disgustingly bad software engineering. Such reproduced content should be implemented once only, in a single file, and #included where necessary. Whether that be by selective inclusion (of fragments) from (say) sys/stat.h into wchar.h, or both include from a third source, is immaterial; we must get away from this insanity of ultimately unmaintainable duplicate definitions in multiple files.
FWIW, I've adjusted my own working tree to resolve my setup.cpp issues. The patch, against 4.0-dev HEAD as of 2013-06-10 at around 23h00, is attached. This eliminates the duplication between wchar.h and sys/stat.h; similarly duplicated content across other files remains unresolved, and potentially inconsistent. Also, I've left a number of FIXMEs to be resolved.
Last edit: Keith Marshall 2013-06-11
And here is my working changes. This resolves the stat.h file as well. I'll review you wchar.h changes to see if I missed anything.
My patch also includes some changes in sys/stat.h, to rationalize the common sections with wchar.h; the FIXMEs I've left are mostly in there.
I'll certainly review your changes too, but I won't get to that for a day or two.
I applied your malformations.diff to a clean checkout of origin/4.0-dev HEAD. Apart from the the relocation of the BEGIN_C_DECLS equivalent fragment within sys/stat.h, it doesn't address a single one of the defects I've reported on this, and related tickets.
I've (partially) reworked the mess which is sys/utime.h, to resolve conflicts with wchar.h; the patch, which follows on from my previous wchar.h vs. sys/stat.h changes, is attached.
Note that I think there is some residual redundancy, and I've added a FIXME to that effect. As it stands, it is sufficient to let me build mingw-get again.
Can you provide a test for the issue(s)? I have an issue with the dropping of the _utimbuf structure definition and the MINGW_RUNTIME < 800 shouldn't be used to the assignment of _utimbuf.
Sorry, I don't understand. I didn't drop it; I
#definedit, as an alias for one or other of the only two physical structural entities which have any logical application, viz.__utimbuf32and__utimbuf64. Why create a physical clone of one or other of these, (as you appear to favour), with the extra maintenance burden that this imposes? Not to mention the additional attendant type casting that this requires because the two names are logically distinct, even though the underlying physical structures are identical? #define avoids both issues, in a simple manner. Simplicity is good; complexity is bad.Then, you'll need to explain just what practical purpose
MSVCRT_VERSION, (which is what my patch uses, and what I assume you mean byMINGW_RUNTIME), actually does serve, because I can't see that it now has any practical value at all. In mingwrt-3.x, I understood it to mean: "I intend to link withMSVCR7x.DLL,MSVCR80.DLL,MSVCR90.DLL, etc., so please expose APIs accordingly". This does not appear to match your interpretation of its intended use; you seem to have emasculated it, such that it now appears, to me, that it does nothing useful at all.Here is another idea which would reduce the header-file complexity a lot. If msvcrt.def would define aliases for all functions in the form xxx32() or xxx32i64(), then this
switch doesn't have to be made in the header files any more.
Here is a patch file (mingw6.patch) which does exactly this. It only handles
time.h, sys/timeb.h and sys/utime.h. io.h and wchar.h can be done the same way.
Conceptually, I like this idea. However, I think we may need to keep some degree of filtering in the headers, lest we otherwise create a scenario whereby references are resolved via MinGW import library trampolines, and are thus incompatible with (legacy) MSVC, or even with MinGW's own direct DLL linking capability.
One would need to ensure a more current binutils when building the import library. Also, there is no way to control when and when not to use this method when the newer MSVCRT.DLL contains the actual imports. However, it was my plan to consider this method in version 5.0.
Agreed, but the filtering should be restricted to only disabling functions when the MSVCRT version doesn't match. This idea solves the 'endless loop' problem, and another advantage is that gdb will show explicitly whether a 32-bit or 64-bit function was called, even though the dll might export it without the '32' and/or the source code has it without the '64'.
I've just pushed my changes to the 4.0-dev branch. These allow the gui-dev branch of mingw-get to build.