I first noticed this when `test_compare_function_objects` was taking forever to run. The culprit is that the following statement just hangs forever. Note that "eval(2)", for instance, runs fine, but when a builtin object is used (eg: None, True), it hangs.

  python -c "eval('None')"

This is reproducible on Python 2.7 (rc1 and latest trunk) and AIX 5.1.

This is regression, as it used to work on 2.6.

eval('ghjsdjhgh') too hangs, btw.

Since it works fine on Linux, maybe you could do some bisecting on the revision history to try to identify what rev broke it on AIX?

How does it behave with the head of the current 2.6 maintenance branch, by the way?

I traced the "infinite loop" to tokenizer.c:tok_get around line 1368:

        while (Py_ISALNUM(c) || c == '_') {
            c = tok_nextc(tok);
        }

Adding a `printf` statement at the beginning of the loop:

            printf("tok_get: third while: c = %c (%d) ... Py_ISALNUM=%d\n", c, c, Py_ISALNUM(c));

Output:

tok_get: third while: c = � (-1) ... Py_ISALNUM=2
tok_get: third while: c = � (-1) ... Py_ISALNUM=2
tok_get: third while: c = � (-1) ... Py_ISALNUM=2
tok_get: third while: c = � (-1) ... Py_ISALNUM=2
tok_get: third while: c = � (-1) ... Py_ISALNUM=2
tok_get: third while: c = � (-1) ... Py_ISALNUM=2
tok_get: third while: c = � (-1) ... Py_ISALNUM=2
tok_get: third while: c = � (-1) ... Py_ISALNUM=2
[...]


I may not spend much time on this bug, unless someone can hint at what might have gone wrong here?

As for 2.6-maint, I will find that out sometime this week.

Py_CHARMASK(c) = 4294967295

And I think I found the problem: from Include/Python.h

/* Convert a possibly signed character to a nonnegative int */
/* XXX This assumes characters are 8 bits wide */
#ifdef __CHAR_UNSIGNED__
#define Py_CHARMASK(c)		(c)
#else
#define Py_CHARMASK(c)		((unsigned char)((c) & 0xff))
#endif

__CHAR_UNSIGNED__ is defined to 1, but when I printed the value of sizeof(c) in the above while loop, it printed 4. Therefore .. c is 32 bits side.

Ok, I now have a fix for this issue. The reason for sizeof(c) being 4 bytes is because it is defined as `register int` ... and yet `Py_CHARMASK` fails to type-cast `c` to a `char` type, which is exactly what the attached patch does.

David, to answer your question "How does it behave with the head of the current 2.6 maintenance branch, by the way?", since this bug appears to be in Include/pyctype.h, which file was available only in 2.7+ and 3.x, I don't believe it will reproduce in 2.6 maint branch that doesn't even have pyctype.h.

I am also adding Eric Smith, the original author of pyctype.h that got added as a fix for issue5793, to this bug. 

Eric, I've attached a patch ... can you review it please?

I suppose that's correct, although I have no way to test it.

I haven't spent a lot of time looking at the code in tokenizer.c, but if there's a problem with sign-extending signed chars, it wouldn't surprise me if it shows up in more than one place.

Does anyone know what other compilers use signed chars? I would think this would be a problem on those platforms, too.

It would also be good to get a test case for this. I realize it's difficult, but that's the sort of change that might get undone some day by someone going through and "optimizing" the code.

On 2010-06-18, at 11:47 AM, Eric Smith wrote:

> 
> Eric Smith <eric@trueblade.com> added the comment:
> 
> I suppose that's correct, although I have no way to test it.

I have tested it on Linux 64-bit by running `test.regrtest`. It doesn't seem to break anything.

> I haven't spent a lot of time looking at the code in tokenizer.c, but if there's a problem with sign-extending signed chars, it wouldn't surprise me if it shows up in more than one place.

My conclusive understanding of the problem: `register int` is 4 bytes in size, and this (`c`) is used without any cast as an index to the array _Py_ctype_table (in pyctype.c) ... by passing it to `Py_CHARMASK` which, if CHAR_UNSIGNED is defined (as is the case with AIX compiler), *assumes* that `c` will always be a char. And that assumption is not respected by tokenizer.c:tok_get which (indirectly) passes `register int` to this macro.

On 2010-06-18, at 11:49 AM, Eric Smith wrote:

> It would also be good to get a test case for this. I realize it's difficult, but that's the sort of change that might get undone some day by someone going through and "optimizing" the code.

Running existing tests (some of which use `eval`) on a platform/compiler combination (eg: AIX/xlc_r) where __CHAR_UNSIGNED__ gets defined should be enough.

> Does anyone know what other compilers use signed chars?

Most of them do, including gcc, on most platforms. unsigned char is really the uncommon case.

The patch is incorrect; Py_CHARMASK is correct as it stands. It is *not* the objective of Py_CHARMASK to produce a char, but (as the comment above its definition explains) to produce an int.

The question really is why you get a value of -1 into c in the first place. Could it be that you are past the end of file, and reading EOF "characters"?

Py_CHARMASK should return a non-negative integer. As I understand it:

tokenizer.c:tok_get around line 1368:

        while (Py_ISALNUM(c) || c == '_') {
            c = tok_nextc(tok);
        }


1) tok_nextc(tok) returns EOF (correct).

2) c is an int.

3) c == -1 gets passed to Py_ISALNUM(c):

   #define Py_ISALNUM(c)  (_Py_ctype_table[Py_CHARMASK(c)] & PY_CTF_ALNUM)



So either it should be enforced that only chars are passed to
Py_CHARMASK, or a cast for the EOF case is needed (but it should
be to (unsigned char)).


Sridhar, did I sum this up correctly?

> Py_CHARMASK should return a non-negative integer.

And it does, also on AIX. Do we have proof to the contrary?

> tokenizer.c:tok_get around line 1368:
>
>          while (Py_ISALNUM(c) || c == '_') {
>              c = tok_nextc(tok);
>          }
>
>
> 1) tok_nextc(tok) returns EOF (correct).
>
> 2) c is an int.
>
> 3) c == -1 gets passed to Py_ISALNUM(c):
>
>     #define Py_ISALNUM(c)  (_Py_ctype_table[Py_CHARMASK(c)]&  PY_CTF_ALNUM)
>
>
>
> So either it should be enforced that only chars are passed to
> Py_CHARMASK, or a cast for the EOF case is needed (but it should
> be to (unsigned char)).

Why do you say that? If c is -1, then Py_CHARMASK(c) is 255, which is a
positive integer. Passing -1, or any other integer, to Py_CHARMASK is 
perfectly fine.

There seems to be a minor bug in the loop above, which doesn't actually 
break at EOF. Instead, it implicitly trusts that Py_ISALNUM(EOF) is 
false (because 255 is not alpha-numeric); this is a flaw - but that
shouldn't cause breakage on AIX.

> Why do you say that? If c is -1, then Py_CHARMASK(c) is 255, which is a
> positive integer.

What srid seems to be saying is that chars are unsigned on AIX, and therefore Py_CHARMASK() returns -1. Hence his patch proposal.

Of course, it is dubious why EOF is not tested separately rather than passing it to Py_ISALNUM(). Micro-optimization? At least a comment should be added.

Also, really, the Py_CHARMASK() macro seems poorly specified. It claims to "convert a possibly signed character to a nonnegative int", but this is wrong: it doesn't convert to an int at all. Furthermore, it does a cast in one branch but not in the other, which can give bad surprises as here.

You can simulate this on Linux by compiling with:

  BASECFLAGS="-funsigned-char"

Then:

Index: Parser/tokenizer.c
===================================================================
--- Parser/tokenizer.c  (revision 81682)
+++ Parser/tokenizer.c  (working copy)
@@ -1366,6 +1366,8 @@
             break;
         }
         while (Py_ISALNUM(c) || c == '_') {
+            c = EOF; // tok_nextc can return EOF
+            printf("c: %d\n", Py_CHARMASK(c));
             c = tok_nextc(tok);
         }
         tok_backup(tok, c);



>>> eval("abcd")
c: -1
c: -1
c: -1
c: -1
c: -1
c: -1




Also, during compilation you get tons of warnings about using
char as an array subscript:

Objects/stringlib/split.h: In function ‘stringlib_split_whitespace’:
Objects/stringlib/split.h:70: warning: array subscript has type ‘char’
Objects/stringlib/split.h:74: warning: array subscript has type ‘char’
Objects/stringlib/split.h:91: warning: array subscript has type ‘char’

> What srid seems to be saying is that chars are unsigned on AIX, and
> therefore Py_CHARMASK() returns -1. Hence his patch proposal.

Ah, ok. I misread some of the messages (and got confused by msg108125,
which seems to suggest that chars are signed on AIX).

> Of course, it is dubious why EOF is not tested separately rather than
> passing it to Py_ISALNUM(). Micro-optimization? At least a comment
> should be added.

No, I think this is an error that EOF is not processed separately.
Otherwise, char 255 may be confused with EOF.

Of course, this would have to be done throughout.

> Also, really, the Py_CHARMASK() macro seems poorly specified. It
> claims to "convert a possibly signed character to a nonnegative int",
> but this is wrong: it doesn't convert to an int at all. Furthermore,
> it does a cast in one branch but not in the other, which can give bad
> surprises as here.

I think the specification is correct: it ought to convert to a
non-negative int. In the signed char case, it already returns an int.
So if it is changed at all, it needs to be changed, in the unsigned
case, to

#define  Py_CHARMASK(c)		((int)(c))

Martin v. Löwis <report@bugs.python.org> wrote:
> > Of course, it is dubious why EOF is not tested separately rather than
> > passing it to Py_ISALNUM(). Micro-optimization? At least a comment
> > should be added.
> 
> No, I think this is an error that EOF is not processed separately.
> Otherwise, char 255 may be confused with EOF.

Indeed. I think Py_ISALNUM() should check for EOF, to be consistent
with the C isalnum(int c).

> Indeed. I think Py_ISALNUM() should check for EOF, to be consistent
> with the C isalnum(int c).

Ah, that sounds fine.

Re: EOF checking in Py_ISXXX() for consistency with C functions.

After reflecting on this a bit I think it's ultimately not a good
idea. While it is possible to do the EOF check, the macros would
then take either an int in [EOF, 0-UCHAR_MAX] or a signed/unsigned
char. This would be another inconsistency with the C functions,
which are not supposed to take a signed char.


I checked every usage of Py_IS* in the tree and this is an isolated
case. So I think it's better to do the check explicitly and add a
comment to the Py_IS* macros.


Does the patch look good?

I agree with this approach.

> I checked every usage of Py_IS* in the tree and this is an isolated
> case. So I think it's better to do the check explicitly and add a
> comment to the Py_IS* macros.
> 
> 
> Does the patch look good?

Nice. I suppose Py_CHARMASK still needs fixing for the general case,
though).

Committed fix in r82191. Thanks Sridhar for tracking this down.


New issues emerging from this one:

  1) Simplify Py_CHARMASK: issue 9036

  2) Use macros from pyctype.h: issue 9067



Additionally, I noticed that the macros is_potential_identifier* from
py3k/Parser/tokenizer.c assume a contiguous mapping for A-Z, a-z. Is
it ok in Python to assume this (in C it isn't)?

> Additionally, I noticed that the macros is_potential_identifier* from
> py3k/Parser/tokenizer.c assume a contiguous mapping for A-Z, a-z. Is
> it ok in Python to assume this (in C it isn't)?

I don't think we support writing Python source code in an encoding
that's not a superset of ASCII, so it should be fine.

2.7 final works fine on AIX. This issue can be closed.

> 2.7 final works fine on AIX. This issue can be closed.

Ok. Thanks for your reports!