classification
Title: Error parsing arguments on OpenBSD <= 4.4
Type: Stage:
Components: Build, Interpreter Core Versions: Python 3.0
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, henry.precheur, pitrou
Priority: critical Keywords: needs review, patch

Created on 2008-08-27 06:29 by henry.precheur, last changed 2008-09-04 15:34 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
mbstowcs.patch amaury.forgeotdarc, 2008-08-27 11:55
test.c henry.precheur, 2008-08-28 02:36
mbstowcs-2.patch amaury.forgeotdarc, 2008-09-03 17:43
Messages (12)
msg72010 - (view) Author: Henry Precheur (henry.precheur) Date: 2008-08-27 06:29
The function mbstowcs is buggy on OpenBSD <= 4.4:
http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/locale/multibyte_sb.c#rev1.7

Because of this the following command fails:
./python -E setup.py build

The attached patch fixes the problem.
msg72012 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-27 07:53
See also issue3626, which is exactly the same for cygwin.

Maybe a "./configure" paragraph could discover whether mbstowcs is broken.
I don't know how to do it though.
msg72019 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-27 11:55
Here is another patch, which tries to replace all problematic usages of
mbstowcs, and a new "#define HAVE_BROKEN_MBSTOWCS" to activate it.

It needs a review, especially the "configure" stuff, it's my first time
to deal with this.

Tested on cygwin.
msg72037 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-27 18:06
The patch looks fine and harmless to me (but I'm a configure newbie
too). +1 for committing it and seeing if the OpenBSD buildbot feels better.

In the process of testing this patch, I've found another bug: #3705.
msg72038 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-27 18:21
Mmh, in Modules/python.c and Python/frozenmain.c, the return value of
the second call to mbstowcs() should be checked as well (since the first
one is sometimes replaced by a call to strlen(), which never fails).
msg72057 - (view) Author: Henry Precheur (henry.precheur) Date: 2008-08-28 02:36
I removed my previous patch. It was not dealing with all broken mbstowcs
cases and yours is much better.

Some comments on the other patch:

I don't think the macro HAVE_BROKEN_MBSTOWCS alone is a perfect idea. It
won't fix the problem in the long run:

Most contributors won't be aware of this problem and might be using
mbstowcs without putting the #ifdef's. Then the problem will come back
and bite us again.

I would rather do something like this:

#ifdef HAVE_BROKEN_MBSTOWCS
size_t  __non_broken_mbstowcs(wchar_t* pwcs, const char* s, size_t n)
{
    if (pwcs == NULL)
        return strlen(s);
    else
        return mbstowcs(pwcs, s, n);
}
#define mbstowcs    __non_broken_mbstowcs
#endif

It would fix the problem everywhere, and people won't have to worry
about putting #ifdef everywhere in the future.

I attached a test program, run it on cygwin to make sure this approach
works on your side.


Another small remark; #ifdef is better then #ifndef when doing
#ifdef ...
...
#else
...
#endif

Simply because it easier to get "Be positive" than "Don't be negative".

The negative form is generally harder to get whether your are
programming, writing or talking. Use the positive form and you will have
more impact and will make things clearer.
msg72070 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-28 08:02
> Another small remark; #ifdef is better then #ifndef 
> Simply because it easier to get "Be positive" than "Don't be negative".

Yes; but here, the symbol (HAVE_BROKEN_MBSTOWC) has a negative meaning.
I tried to put the optimistic (=not broken) case first.
However, if this makes the code more difficult to read, I'll change it.
msg72077 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-28 10:21
> Yes; but here, the symbol (HAVE_BROKEN_MBSTOWC) has a negative meaning.
> I tried to put the optimistic (=not broken) case first.
> However, if this makes the code more difficult to read, I'll change it.

You could change HAVE_BROKEN_MBSTOWC for a positive flag, e.g.
HAVE_WORKING_MBSTOWC, and then the #ifdef would be the optimistic case.
msg72393 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-03 16:45
Amaury, as long as you fix the small quirk mentioned above (checking the
return value of the second call to mbstowcs()), I think this patch can
go in, since it does no harm on already working platforms.
msg72402 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-09-03 17:43
Here is an updated patch:
- changed #ifndef into #ifdef. The "broken" case comes first now.
- check that the second call to mbstowcs does not fail.
- also, changed an assert, since strlen() does not always count the
exact number of chars.

I won't have SVN access for the next couple of days (behind a firewall...)
Antoine (or someone else), can you please check this in?
msg72412 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-03 18:59
Committed in r66187. Henry, could you confirm it fixes the problem on
your side?
msg72462 - (view) Author: Henry Precheur (henry.precheur) Date: 2008-09-04 02:21
I am now able to finish the build and the interpreter seems to be
working. So it is all good :)
History
Date User Action Args
2008-09-04 15:34:35pitrousetstatus: pending -> closed
resolution: accepted -> fixed
2008-09-04 02:21:15henry.precheursetmessages: + msg72462
2008-09-03 18:59:42pitrousetstatus: open -> pending
priority: release blocker -> critical
resolution: accepted
messages: + msg72412
2008-09-03 17:43:35amaury.forgeotdarcsetfiles: + mbstowcs-2.patch
messages: + msg72402
2008-09-03 16:45:20pitrousetmessages: + msg72393
2008-08-28 10:21:13pitrousetmessages: + msg72077
2008-08-28 08:02:23amaury.forgeotdarcsetmessages: + msg72070
2008-08-28 02:36:31henry.precheursetfiles: - fix_mbstowcs_openbsd.diff
2008-08-28 02:36:19henry.precheursetfiles: + test.c
messages: + msg72057
2008-08-27 18:21:59pitrousetmessages: + msg72038
2008-08-27 18:06:32pitrousetnosy: + pitrou
messages: + msg72037
2008-08-27 17:51:46amaury.forgeotdarcsetkeywords: + needs review
2008-08-27 11:56:07amaury.forgeotdarcsetpriority: release blocker
2008-08-27 11:55:50amaury.forgeotdarcsetfiles: + mbstowcs.patch
messages: + msg72019
2008-08-27 07:53:34amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg72012
2008-08-27 07:38:29amaury.forgeotdarcsettitle: mbstowcs -> Error parsing arguments on OpenBSD <= 4.4
2008-08-27 07:38:03amaury.forgeotdarcsettitle: Error parsing arguments on OpenBSD <= 4.4 -> mbstowcs
2008-08-27 06:29:32henry.precheurcreate