Opened 5 years ago

Closed 5 years ago

#4142 defect closed fixed (fixed)

twisted/protocols/_c_urlarg.c compilation errors on IBM AIX

Reported by: aprilmay Owned by: spiv
Priority: normal Milestone:
Component: core Keywords:
Cc: spiv, exarkun Branch: branches/portable-c-4142
(diff, github, buildbot, log)
Author: spiv Launchpad Bug:

Description

Errors building on IBM AIX 5.3 with native compiler xlc_r:

Running Twisted-9.0.0/setup.py -q bdist_egg --dist-dir /tmp/easy_install-dO3vwp/Twisted-9.0.0/egg-dist-tmp-ECS6yS
"conftest.c", line 1.10: 1506-296 (S) #include file <sys/epoll.h> not found.
"conftest.c", line 1.23: 1506-356 (W) Compilation unit is empty.
"twisted/protocols/_c_urlarg.c", line 65.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 75.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 83.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 85.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 93.20: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 96.20: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 97.20: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.

Unfortunately _c_urlarg.so is generated, so:

>>> from twisted.protocols._c_urlarg import unquote
>>> unquote("")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError: out of memory

When running a twisted web server, there's a nasty CPU loop around line 122 of twisted/web/server.py. I guess other defects might occur so the easy workaround os to remove the _c_urlarg.so file.

Everything else seem to work like a charm on AIX (with python 2.6 from http://www.perzl.org/aix/index.php?n=Main.Python)

Attachments (2)

urlarg.diff (408 bytes) - added by spiv 5 years ago.
Possible fix?
fix-memory-error.diff (507 bytes) - added by spiv 5 years ago.
fix for memory error

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by spiv

Possible fix?

comment:1 Changed 5 years ago by spiv

I wonder if explicitly casting to (const char *) in the OUTPUTCHAR macro would fix those warnings? I've attached a one-liner patch to this ticket that does that; it builds and tests cleanly for me.

comment:2 Changed 5 years ago by spiv

  • Cc spiv added

comment:3 Changed 5 years ago by exarkun

  • Cc exarkun added

On AIX?

comment:4 Changed 5 years ago by spiv

No, sorry for being unclear. What I should have said is that it builds and runs cleanly for me on Linux (Ubuntu Karmic, x86), so it is at least not a regression.

We still need someone with AIX, perhaps the reporter, to try it there.

comment:5 Changed 5 years ago by aprilmay

The memory error still persists after applying the proposed patch (however the compilation errors have disappeared).

comment:6 Changed 5 years ago by spiv

  • Keywords review added

Oh, I bet I know what this is. On some platforms malloc(0) fails.

So probably the fix for the MemoryError is:

if (length == 0) {
    return PyString_FromStringAndSize("", 0);
}

This would need to happen just before the call to NewOutput. Patch attached.

Changed 5 years ago by spiv

fix for memory error

comment:7 Changed 5 years ago by aprilmay

Yes, this new patch fixed the issue:

>>> from twisted.protocols._c_urlarg import unquote
>>> unquote("")
''

Web server runs smoothly on AIX now. Thanks !!!

A test case could be also added?

comment:8 Changed 5 years ago by spiv

Thanks very much for testing that and reporting the results so quickly.

By the way, twisted.web.test.test_http.QueryArgumentsTestCase.testUnquote already tests this case:

        # Empty string
        self.failUnlessEqual(urllib.unquote(""), _c_urlarg.unquote(""))

You should be able to run that test yourself with trial twisted.web.test.test_http.QueryArgumentsTestCase.testUnquote, or even better just run the whole test suite (trial twisted) and file bugs for any other failures :)

comment:9 Changed 5 years ago by spiv

  • Author set to spiv
  • Branch set to branches/portable-c-4142

comment:10 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner changed from glyph to spiv
  1. Warnings are gone on Ubuntu 8.10. This is good, right? Or is it? This kind of C fix always bothers me. I looked up s# in the Python docs (http://python.org/doc/1.5.2p2/ext/parseTuple.html) and I see that it's actually declared as a char *. I wonder why the variable in this function is declared as a unsigned char *. If it were just char * then the cast wouldn't be necessary, right? Maybe there's some other good reason it's an unsigned char *, though.
  2. The tests pass (again on Ubuntu 8.10). They did before, of course. Also, strange that the tests are in Twisted Web, when the code is in Twisted Core.
  3. As I mentioned on IRC, I can't use combinator to merge the branch. The revision doesn't get picked up. Not a big deal here, since you're going to merge it and I suppose you know how. It'd be nice to avoid this case in the future, though. :)
  4. Please also add a news entry to the topfiles directory. (ReviewProcess#Newsfiles)

It'd be great to hear that the unit test also passes on AIX before giving the ok to merge this (and if the unsigned char * thing changes at all, it'd be great to re-confirm that it still works on AIX).

Thanks!

comment:11 Changed 5 years ago by spiv

  1. char * would probably make more sense, except for the is_hexdigit function. It relies on an array defined as unsigned char hexdigits[256], and the unsigned chars are used as indices. There's probably a more elegant solution that avoids the ugly cast, but a trivial change to char * in unquote would simply transfer the ugly cast to is_hexdigit. Given that warnings are gone and this appears to be functioning correctly (tests pass), I think it's ok as is. I'll happily review patches to improve this, though :)
  2. I filed #4162 about the misplaced test (or misplaced module?).
  3. Yes, noted for future. I'll happily take the burden of doing the merge for this one.
  4. I've added an entry with this text: “The optional _c_urlarg extension now handles unquote("") correctly on platforms where malloc(0) returns NULL, such as AIX. It also compiles with less warnings.”

comment:12 Changed 5 years ago by spiv

  • Resolution set to fixed
  • Status changed from new to closed

(In [27898]) Merge portable-c-4142: Make _c_urlarg work on AIX.

Author: spiv
Reviewer: exarkun
Fixes: #4142

The optional _c_urlarg extension now handles unquote("") correctly on
platforms where malloc(0) returns NULL, such as AIX. It also compiles with
less warnings on all platforms.

Note: See TracTickets for help on using tickets.