Skip to content

New C reader crashes in Windows #2952

New issue
New issue
Closed
Closed
New C reader crashes in Windows#2952
Labels
Affects-devPRs and issues that do not impact an existing Astropy releaseBugEffort-mediumPackage-expertWindowsio.ascii

Description

@embray
embray
opened on Sep 16, 2014

Unfortunately, when I run test_c_reader.py on Windows the Python interpreter crashes with no specified error.

I'll update this more once I have more details, but I just wanted to make sure to note it before I forget (this is important though as it basically makes Windows testing impossible until it's resolved (or those tests are skipped)).

Activity

  1. embray
    added
    io.ascii
    Bug
    Affects-devPRs and issues that do not impact an existing Astropy release
    Windows
    Package-expert
    Effort-medium
    on Sep 16, 2014

  2. embray commented on Sep 16, 2014

    @embray
    embray
    on Sep 16, 2014
    MemberAuthor

    (suggested "Effort-medium" for now, until and unless I figure out what the actual problem is)

  3. embray commented on Sep 16, 2014

    @embray
    embray
    on Sep 16, 2014
    MemberAuthor

    Relatedly, tokenizer.c does not compile with MSVC. When I sent this report I had compiled it with gcc (via mingw32), but I just thought I'd see if compiling with MSVC made any difference, and it didn't compile at all:

    c:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\BIN\cl.exe /c /nologo /Ox
/MD /W3 /GS- /DNDEBUG -Ic:\Python27\lib\site-packages\numpy\core\include -Ic:\Python27\include -Ic:\Python27\PC /Tcastropy\io\ascii\src\tokenizer.c /Fobuild\temp.win32-2.7\Release\astropy\io\ascii\src\tokenizer.obj
tokenizer.c
astropy\io\ascii\src\tokenizer.c(92) : error C2054: expected '(' to follow 'inline'
astropy\io\ascii\src\tokenizer.c(93) : error C2085: 'push' : not in formal parameter list
astropy\io\ascii\src\tokenizer.c(93) : error C2143: syntax error : missing ';' before '{'
astropy\io\ascii\src\tokenizer.c(121) : error C2054: expected '(' to follow 'inline'
astropy\io\ascii\src\tokenizer.c(122) : error C2085: 'end_field' : not in formal parameter list
astropy\io\ascii\src\tokenizer.c(122) : error C2143: syntax error : missing ';'
before '{'
astropy\io\ascii\src\tokenizer.c(160) : error C2054: expected '(' to follow 'inline'
astropy\io\ascii\src\tokenizer.c(162) : error C2085: 'end_line' : not in formal
parameter list
astropy\io\ascii\src\tokenizer.c(162) : error C2143: syntax error : missing ';'
before '{'
astropy\io\ascii\src\tokenizer.c(323) : warning C4013: 'end_field' undefined; assuming extern returning int
astropy\io\ascii\src\tokenizer.c(381) : warning C4013: 'push' undefined; assuming extern returning int
astropy\io\ascii\src\tokenizer.c(389) : warning C4013: 'end_line' undefined; assuming extern returning int
astropy\io\ascii\src\tokenizer.c(683) : warning C4554: '&' : check operator precedence for possible error; use parentheses to clarify precedence
astropy\io\ascii\src\tokenizer.c(783) : warning C4018: '>=' : signed/unsigned mismatch
error: command '"c:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\BIN\cl.exe"' failed with exit status 2

    Probably something related to what dialect of C it's using compared to the default for gcc (or clang for that matter).

  4. astrofrog commented on Sep 17, 2014

    @astrofrog
    astrofrog
    on Sep 17, 2014
    Member

    cc @AmrAS1 @taldcroft

  5. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    I tracked down the MSVC issue. No big deal there.

    The test still crashes when compiled with MSVC though--I didn't think it was the compiler anyways. Apparently this is something pretty basic because it crashes right in the first test on:

    read_basic("A B C\n1 2 3\n4 5 6")

  6. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    Digging a little deeper, it's specifically in parallel=True where it blows up.

  7. mdmueller commented on Sep 17, 2014

    @mdmueller
    mdmueller
    on Sep 17, 2014
    Contributor

    Hm...do you have the full error output available, or is it a segfault?

  8. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    I'm checking on that now. It looks like it's a segfault.

  9. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    I don't have a debug build of Python on Windows right now, but this is what I got:

    Program received signal SIGSEGV, Segmentation fault.
0x1e00fee2 in python27!PyString_FromString ()
   from C:\Windows\SysWOW64\python27.dll
(gdb) where
#0  0x1e00fee2 in python27!PyString_FromString ()
   from C:\Windows\SysWOW64\python27.dll
#1  0x680470d4 in __pyx_pf_7astropy_2io_5ascii_7cparser_7CParser_16__reduce__
    (__pyx_v_self=0x35d7e68) at astropy\io\ascii\cparser.c:12221
#2  __pyx_pw_7astropy_2io_5ascii_7cparser_7CParser_17__reduce__ (
    __pyx_v_self=0x35d7e68, unused=0x0) at astropy\io\ascii\cparser.c:12189
#3  0x1e015d93 in python27!PyCFunction_Call ()
   from C:\Windows\SysWOW64\python27.dll

    The segfault is in PyString_FromString, but it's being called from CParser.__reduce__. Specifically, the line from the generated C code is:

      __pyx_t_2 = __Pyx_PyBytes_FromString(__pyx_v_self->source_ptr); if (unlikely(!__pyx_t_2)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 723; __pyx_clineno = __LINE__; goto __pyx_L1_error;

    so it looks like maybe source_ptr contains a bad address.

  10. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    (Indeed, self.source_ptr is NULL here)

  11. 2 remaining items

  12. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    I'm not convinced it's anything so deep as that though. That is, I don't think it's necessary to make any zany patches to multiprocessing or anything. We just need to get __reduce__ working properly. That could be tested even without Windows (just make a CParser object and manually call __reduce__ on it).

  13. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    That would explain though why this is only showing up on Windows.

  14. mdmueller commented on Sep 17, 2014

    @mdmueller
    mdmueller
    on Sep 17, 2014
    Contributor

    Oh, that patch isn't mine 😃 That's just a newly added convenience to replicate Windows behavior on Linux/Mac. You're right, it's actually not strictly necessary to test this out.

  15. mdmueller commented on Sep 17, 2014

    @mdmueller
    mdmueller
    on Sep 17, 2014
    Contributor

    @embray - Does this work? It should fix the segfault but I haven't been able to check yet if the tests will still pass.

  16. embray commented on Sep 17, 2014

    @embray
    embray
    on Sep 17, 2014
    MemberAuthor

    Unfortunately that does not fix it--but I didn't expect it would. The problem occurs even before _cparser_copy is called. Even if _cparser_copy expects a Python bytes object the char * needs to be converted to one first via PyBytes_FromString (or the equivalent) and that's where the segfault is occurring.

    I found you can reproduce this easily on Linux:

    CParser(None, 0, 0).__reduce__()

  17. mdmueller commented on Sep 17, 2014

    @mdmueller
    mdmueller
    on Sep 17, 2014
    Contributor

    @embray - CParser(None, 0, 0).__reduce__() works for me with the altered version. The relevant difference is this: https://github.com/amras1/astropy/compare/fix-cparser-reduce#diff-46d96e60712ec8a415ed95737bd279d3R723

  18. embray commented on Sep 18, 2014

    @embray
    embray
    on Sep 18, 2014
    MemberAuthor

    Oh, somehow I missed that change. That looks feasible. I'll give it a try.

  19. embray commented on Sep 22, 2014

    @embray
    embray
    on Sep 22, 2014
    MemberAuthor

    @AmrAS1 I tried out your fix on Windows and it no longer segfaults, so that's a start. Unfortunately the multiprocessing still doesn't work properly on Windows. It just hangs for a very long time--I'm not sure what it's doing. But that gets back to what you were saying about sketchy multiprocessing support on Windows.

    I think that until we can get to the bottom of this and work out a good solution we should just disable the multiprocessing support on Windows and mark any tests that use it as xfail on Windows, because in the meantime it's impossible to run the tests on Windows because of this :(

  20. mdmueller commented on Sep 22, 2014

    @mdmueller
    mdmueller
    on Sep 22, 2014
    Contributor

    @embray - I agree, that makes sense. I'll try to figure out what's going on (hopefully pretty soon), but for now I'll just open a PR to disable multiprocessing testing on Windows. All fast-reading tests use multiprocessing, so I think I should just alter those instead of marking them as xfail.

  21. taldcroft commented on Sep 22, 2014

    @taldcroft
    taldcroft
    on Sep 22, 2014
    Member

    @AmrAS1 @embray - as part of disabling the multiprocessing testing on windows, should we also make the parallel option emit a warning (on windows only) that windows support for parallel may have problems? This could be backed out later if a good solution is found, but maybe it's better to not have a serious known problem lurking in master.

  22. mdmueller commented on Sep 22, 2014

    @mdmueller
    mdmueller
    on Sep 22, 2014
    Contributor

    @taldcroft - That sounds good to me. I'll add this as a commit in #2969.

  23. embray commented on Sep 22, 2014

    @embray
    embray
    on Sep 22, 2014
    MemberAuthor

    @taldcroft @AmrAS1 I was suggesting that in addition to the tests, the parallel option itself should be disabled entirely on Windows, emmitting a NotImplemented exception or such. Anything I try to do it just hangs my system.

  24. taldcroft commented on Dec 20, 2014

    @taldcroft
    taldcroft
    on Dec 20, 2014
    Member

    @embray - can this be closed?

  25. embray commented on Dec 22, 2014

    @embray
    embray
    on Dec 22, 2014
    MemberAuthor

    I think so--I still have a lot of test failures for io.ascii on Windows, but at least it's not segfaulting or otherwise crashing the Python interpreter.

  26. embray
    closed this as completedon Dec 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Affects-devPRs and issues that do not impact an existing Astropy releaseBugEffort-mediumPackage-expertWindowsio.ascii

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions