re: msvcrt: scanf fix a typo
Here's the gcc error:
scanf.c:66: warning: unknown conversion type character `P' in format
I'm not a programmer, but I play one on TV. And here's what I came up with in five minutes of typing and not enough thinking: That particular error depends on gcc knowing intimate details of sscanf. Unless we teach gcc about the particular sscanf we're implementing, it's likely to give false errors. It could be that the line #include <stdio.h> at the top of that file is introducing a conflict. See http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html#Functio... for the function attributes used to teach gcc about scanf-like format arguments. This may be an example of the problem Juan was anticipating, where people rush to provide fixes to problems that they don't fully understand, just to try to get rid of gcc warnings :-( - Dan
On Fri, Sep 19, 2008 at 5:51 PM, Dan Kegel <dank(a)kegel.com> wrote:
Here's the gcc error:
scanf.c:66: warning: unknown conversion type character `P' in format
I'm not a programmer, but I play one on TV. And here's what I came up with in five minutes of typing and not enough thinking:
That particular error depends on gcc knowing intimate details of sscanf. Unless we teach gcc about the particular sscanf we're implementing, it's likely to give false errors.
It could be that the line
#include <stdio.h>
at the top of that file is introducing a conflict.
See http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html#Functio... for the function attributes used to teach gcc about scanf-like format arguments.
This may be an example of the problem Juan was anticipating, where people rush to provide fixes to problems that they don't fully understand, just to try to get rid of gcc warnings :-( - Dan
Point taken. I'll just file a bug and let someone who knows what they're doing take a look at it. - Austin
Am Freitag, den 19.09.2008, 15:51 -0700 schrieb Dan Kegel:
Here's the gcc error:
scanf.c:66: warning: unknown conversion type character `P' in format That particular error depends on gcc knowing intimate details of sscanf. Unless we teach gcc about the particular sscanf we're implementing, it's likely to give false errors.
See http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html#Functio... for the function attributes used to teach gcc about scanf-like format arguments.
Looks like we need to compile with -fno-builtin-sscanf, because gcc knows the implementation of glibc's sscanf, and you explicitly have to forbid it to use this knowledge which does not apply perfectly to Microsofts sscanf. This (of course) is only applicable to those parts of wine that link to msvcrt and thus use msvcrt's scanf implementation. Regards, Michael Karcher
Am Samstag, den 20.09.2008, 10:50 +0200 schrieb Michael Karcher:
Looks like we need to compile with -fno-builtin-sscanf, because gcc knows the implementation of glibc's sscanf,
OK, I looked further into it. On Linux, we don't get the warning, because gcc is called with the sledgehammer option "-fno-builtin" for msvcrt related stuff (which includes *all* -fno-builtin-* options, so also -fno-builtin-sscanf) if gcc's builtin prototype for the wchar ctype function conflicts with "int iswlower(unsigned short);". Probably on your system, your C library does not have these functions or your gcc is older than 4.0, so there are no builtin prototypes for the wchar stuff. There are some ways to fix it: a) Detect whether gcc recognizes scanf as builtin function, and enable -fno-builtin in that case too. This will trigger on gcc version 3.3 and up. b) Always pass -fno-builtin to msvcrt related stuff when compiling with gcc, i.e. drop the wchar test. c) Make the -fno-builtin option more specific. i.e. if wchar stuff conflicts with msvcrt, enable -fno-builtin-iswalnum -fno-builtin-iswalpha -fno-builtin-iswascii -fno-builtin-iswcntrl -fno-builtin-iswdigit -fno-builtin-iswgraph -fno-builtin-iswlower -fno-builtin-iswprint -fno-builtin_iswpunct -fno-builtin-iswspace -fno-builtin-iswupper -fno-builtin-iswxdigit -fno-builtin-towlower -fno-builtin-towupper and if scanf is detected, enable -fno-builtin-fscanf -fno-builtin-scanf -fno-builtin-sscanf The upside is that we don't lose gcc's checking of other standard C functions, while the downside is that the gcc command line in msvcrt related modules gets awfully long. These modules are: dlls/msvcrtd/tests dlls/msvcrtd dlls/msvcrt/tests dlls/crtdll (Just grep Makefile.in for @BUILTINCFLAG@) While the specific -fno-builtin-foo optins have been introduced in gcc 3.1, we do not need to check whether gcc is new enough, as these old versions of gcc have neither wchar nor sscanf functions built in. If wine-devel agrees to one approach, I am happy to submit a patch that implements this approach. I personally prefer a crossover between approach b and c. Approach a is out, because on recent versions of gcc, it is essentially approach b, but unnecessarily complicating the configure script. Also it tends to hide problems, just as it happened here. gcc has added a sanity check for sscanf parameters (which is a good thing, IMHO), but we never noticed it, because it got suppressed because of configure finding wchar builtins on the platform most Wine developers work on. With this approach, changing the platform might change gcc's behaviour on sscanf without anyone expecting it. Approach b is correct in the sense that the MS C library is not the platform C library, so gcc better does not assume anything about it, but that also might incur performance penalties, as things like memcpy with small constant size will never get inlined. Probably b really is what the wine project needs in the tests of msvcrt, as the goal is to test msvcrt and not gcc's inline code. For the tests, performance penalties do not matter. On the other hand, for the implementation side (msvcrtd and crtdll), option c seems attractive to me, because it just disables the stuff we need disabled but allows gcc to take full advantage about knowing the C standard on functions where msvcrt is not incompatible. A problem with option C is, that it is playing a cat-and-mouse game with the gcc developers: As soon as they add some more checks to calling standard library functions that are incompatible with Microsofts extensions, we start to get warnings again. Regards, Michael Karcher
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
On the other hand, for the implementation side (msvcrtd and crtdll), option c seems attractive to me, because it just disables the stuff we need disabled but allows gcc to take full advantage about knowing the C standard on functions where msvcrt is not incompatible. A problem with option C is, that it is playing a cat-and-mouse game with the gcc developers: As soon as they add some more checks to calling standard library functions that are incompatible with Microsofts extensions, we start to get warnings again.
The real problem is that the -fno-builtin-xxx option is broken on some gcc versions. We used to use it, but we had to switch to a global -fno-builtin because of this bug. -- Alexandre Julliard julliard(a)winehq.org
Am Samstag, den 20.09.2008, 12:17 +0200 schrieb Alexandre Julliard:
The real problem is that the -fno-builtin-xxx option is broken on some gcc versions. We used to use it, but we had to switch to a global -fno-builtin because of this bug.
OK. Thanks for the pointer. I have a configure test ready that checks whether the global -fno-builtin is really needed., and uses separate flags otherwise. I am planning to submit it in series with a second patch that checks for whether -fno-builtin-scanf is needed (check for warning on "%P"). Regards, Michael Karcher
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
OK. Thanks for the pointer. I have a configure test ready that checks whether the global -fno-builtin is really needed., and uses separate flags otherwise. I am planning to submit it in series with a second patch that checks for whether -fno-builtin-scanf is needed (check for warning on "%P").
I don't think there's any reason to make it that complex. You can just use -fno-builtin in all cases. -- Alexandre Julliard julliard(a)winehq.org
Am Sonntag, den 21.09.2008, 11:25 +0200 schrieb Alexandre Julliard:
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
OK. Thanks for the pointer. I have a configure test ready that checks whether the global -fno-builtin is really needed., and uses separate flags otherwise. I am planning to submit it in series with a second patch that checks for whether -fno-builtin-scanf is needed (check for warning on "%P"). I don't think there's any reason to make it that complex. You can just use -fno-builtin in all cases.
Sure I can, this was approach b I suggested. But that may be harmful to performance, as it forbids gcc to use any knowledge about the standard library (includes inlining of memcpy with small constant sizes or built-in pureness annotations). But the issue that started the discussion was: What to do about the scanf warning? On a system that does not have wchar builtins in gcc, currently all built-in functions are enabled. Even on these systems, we should disable the built-in scanf to prevent the warning in the scanf test. This need has long been unnoticed, as wchar ctype functions are built-in on our main development platform. Regards, Michael Karcher
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
Sure I can, this was approach b I suggested. But that may be harmful to performance, as it forbids gcc to use any knowledge about the standard library (includes inlining of memcpy with small constant sizes or built-in pureness annotations).
If performance is an issue you most likely don't want to use msvcrt at all, native Unix libc will always be faster. And knowing MS, they are capable of adding an exception handler in memcpy someday...
But the issue that started the discussion was: What to do about the scanf warning? On a system that does not have wchar builtins in gcc, currently all built-in functions are enabled. Even on these systems, we should disable the built-in scanf to prevent the warning in the scanf test. This need has long been unnoticed, as wchar ctype functions are built-in on our main development platform.
Just use -fno-builtin if it's supported, regardless of wchar functions. This will fix scanf and avoid possible similar problems in the future. Something like this should do the trick: diff --git a/configure.ac b/configure.ac index 27ae9ef..cda7f47 100644 --- a/configure.ac +++ b/configure.ac @@ -1238,6 +1238,7 @@ then WINE_TRY_CFLAGS([-fshort-wchar], [AC_DEFINE(CC_FLAG_SHORT_WCHAR, "-fshort-wchar", [Specifies the compiler flag that forces a short wchar_t])]) + WINE_TRY_CFLAGS([-fno-builtin],[AC_SUBST(BUILTINFLAG,"-fno-builtin")]) WINE_TRY_CFLAGS([-fno-strict-aliasing]) WINE_TRY_CFLAGS([-Wdeclaration-after-statement]) WINE_TRY_CFLAGS([-Wwrite-strings]) @@ -1253,17 +1254,6 @@ then then EXTRACFLAGS="$EXTRACFLAGS -Wpointer-arith" fi - - AC_SUBST(BUILTINFLAG,"") - saved_CFLAGS="$CFLAGS" - CFLAGS="$CFLAGS -Werror" - AC_CACHE_CHECK([for builtin wchar inlines], ac_cv_c_builtin_wchar_ctype, - AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[]], [[int iswlower(unsigned short);]])],[ac_cv_c_builtin_wchar_ctype=no],[ac_cv_c_builtin_wchar_ctype=yes])) - CFLAGS="$saved_CFLAGS" - if test "$ac_cv_c_builtin_wchar_ctype" = "yes" - then - BUILTINFLAG="-fno-builtin" - fi fi dnl **** Check how to define a function in assembly code **** -- Alexandre Julliard julliard(a)winehq.org
Am Sonntag, den 21.09.2008, 13:07 +0200 schrieb Alexandre Julliard:
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
Sure I can, this was approach b I suggested. But that may be harmful to performance, as it forbids gcc to use any knowledge about the standard library (includes inlining of memcpy with small constant sizes or built-in pureness annotations). If performance is an issue you most likely don't want to use msvcrt at all, native Unix libc will always be faster. And knowing MS, they are capable of adding an exception handler in memcpy someday...
I was thinking about our dll implementations. crtdll and msvcrtd both link to msvcrt. Looking at the implementations of these DLLs, it seems you are right. No need to not compile them with -fno-builtin. On the other hand, regedit, taskmgr, xcopy, wordpad and notepad are linked to msvcrt. Should that be changed? Regards, Michael Karcher
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
Am Sonntag, den 21.09.2008, 13:07 +0200 schrieb Alexandre Julliard:
If performance is an issue you most likely don't want to use msvcrt at all, native Unix libc will always be faster. And knowing MS, they are capable of adding an exception handler in memcpy someday...
I was thinking about our dll implementations. crtdll and msvcrtd both link to msvcrt. Looking at the implementations of these DLLs, it seems you are right. No need to not compile them with -fno-builtin.
That's not at all what I said. Any module that uses msvcrt headers needs to use -fno-builtin.
On the other hand, regedit, taskmgr, xcopy, wordpad and notepad are linked to msvcrt. Should that be changed?
No, they are just fine the way they are, and it provides some nice testing for our msvcrt. -- Alexandre Julliard julliard(a)winehq.org
Am Sonntag, den 21.09.2008, 13:46 +0200 schrieb Alexandre Julliard:
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
Am Sonntag, den 21.09.2008, 13:07 +0200 schrieb Alexandre Julliard:
If performance is an issue you most likely don't want to use msvcrt at all, native Unix libc will always be faster. I was thinking about our dll implementations. crtdll and msvcrtd both link to msvcrt. Looking at the implementations of these DLLs, it seems you are right. No need to not compile them with -fno-builtin. That's not at all what I said. Any module that uses msvcrt headers needs to use -fno-builtin.
Sorry, I didn't manage to write what I mean. The only dll modules in the wine tree that currently use @BUILTINFLAG@ (and include msvcrt headers) are the two I listed. And after examing the little amount of C code they contain, I see no problem to compile them with -fno-builtin. That's what I meant to say. So your patch of always using -fno-builtin with gcc is the right way to go. Regards, Michael Karcher
Michael Karcher <wine(a)mkarcher.dialup.fu-berlin.de> writes:
Sorry, I didn't manage to write what I mean. The only dll modules in the wine tree that currently use @BUILTINFLAG@ (and include msvcrt headers) are the two I listed. And after examing the little amount of C code they contain, I see no problem to compile them with -fno-builtin.
Right; I agree we wouldn't want to use -fno-builtin in other dlls, but we don't want to use msvcrt in dlls anyway, and we should never need to. -- Alexandre Julliard julliard(a)winehq.org
On Sat, 20 Sep 2008, Michael Karcher wrote: [...]
Looks like we need to compile with -fno-builtin-sscanf, because gcc knows the implementation of glibc's sscanf, and you explicitly have to forbid it to use this knowledge which does not apply perfectly to Microsofts sscanf. This (of course) is only applicable to those parts of wine that link to msvcrt and thus use msvcrt's scanf implementation.
Just mentioning an alternative approach in case the -fno-builtin-* approach does not pan out: maybe we can fool gcc by doing something like this: char buf[3]; strcpy(buf, "%P"); ok( sscanf("1234", buf, &ptr) == 1, "sscanf failed\n" ); -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ tcA thgirypoC muinelliM latigiD eht detaloiv tsuj evah uoY
On Mon, Sep 22, 2008 at 5:27 AM, Francois Gouget <fgouget(a)free.fr> wrote:
On Sat, 20 Sep 2008, Michael Karcher wrote: [...]
Looks like we need to compile with -fno-builtin-sscanf, because gcc knows the implementation of glibc's sscanf, and you explicitly have to forbid it to use this knowledge which does not apply perfectly to Microsofts sscanf. This (of course) is only applicable to those parts of wine that link to msvcrt and thus use msvcrt's scanf implementation.
Just mentioning an alternative approach in case the -fno-builtin-* approach does not pan out: maybe we can fool gcc by doing something like this:
char buf[3]; strcpy(buf, "%P"); ok( sscanf("1234", buf, &ptr) == 1, "sscanf failed\n" );
-- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ tcA thgirypoC muinelliM latigiD eht detaloiv tsuj evah uoY
The fix Alexandre committed (http://source.winehq.org/git/wine.git/?a=commitdiff;h=6538cb44e371bcd3a82122...) fixed it for me (gcc version 3.4.6). -Austin
On Fri, Sep 19, 2008 at 03:51:00PM -0700, Dan Kegel wrote:
Here's the gcc error:
scanf.c:66: warning: unknown conversion type character `P' in format
I'm not a programmer, but I play one on TV. And here's what I came up with in five minutes of typing and not enough thinking:
That particular error depends on gcc knowing intimate details of sscanf. Unless we teach gcc about the particular sscanf we're implementing, it's likely to give false errors.
For the tests, the compile time error can be avoided by using a non-constant format string. Quite possibly adding a global variable whose value is a always zero to the format string is enough. David -- David Laight: david(a)l8s.co.uk
participants (6)
-
Alexandre Julliard -
Austin English -
Dan Kegel -
David Laight -
Francois Gouget -
Michael Karcher