Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#22810 closed defect (fixed)

Pari initialization segfaults in Cygwin since #22633

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.0
Component: porting: Cygwin Keywords: windows cygwin pari mmap
Cc: jdemeyer Merged in:
Authors: Erik Bray Reviewers: Travis Scrimshaw, Jeroen Demeyer
Report Upstream: Not yet reported upstream; Will do shortly. Work issues:
Branch: 00a5d73 (Commits) Commit:
Dependencies: #22675 Stopgaps:

Description

Sorry, I thought I had tested the last version of the patch in #22633. But I think I just tested it in PARI/GP, but not in Sage itself, since my Sage build was broken at the time for other reasons.

The offending line is here. It seems that MAP_FIXED is not working correctly for some reason, but the resulting call to pari_err segfaults because the pari mainstack is left in an invalid state.

I'm not sure if this is a bug in Cygwin or what, but I'm trying to put together a simplified test case.

Change History (24)

comment:1 in reply to: ↑ description Changed 3 years ago by jdemeyer

Replying to embray:

It seems that MAP_FIXED is not working correctly for some reason

The question now becomes: why?

Does everything work fine if you remove the line if (res != addr) pari_err(e_MEM);

For debugging, it would be useful to print the values of addr, s and res after that mmap() call.

comment:2 Changed 3 years ago by embray

Yes, that's what I'm trying to do now.

comment:3 Changed 3 years ago by jdemeyer

I guess that MAP_FIXED is not commonly used, so it wouldn't be so surprising that it is buggy in Cygwin.

comment:4 Changed 3 years ago by embray

The problem is not so much MAP_FIXED itself--it works in a different context.

In this case the mmap in pari_mainstack_mreset is failing unless I call munmap(addr, s) first. If I understand correctly that should be fine, because we're not using that memory anyways right now if we're setting it to PROT_NONE.

comment:5 follow-up: Changed 3 years ago by embray

I also found some relevant discussion here: http://cygwin.1069669.n5.nabble.com/mmap-MAP-FIXED-vs-mprotect-td98125.html But I think it's fine as long as you munmap() first--that will set those pages to unused/unresreved, and then they can be re-reserved. The only problem is the munmap followed by mmap will not be atomic. But how likely is that to be a problem in practice?

comment:6 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

  • Dependencies set to #22675

Replying to embray:

The only problem is the munmap followed by mmap will not be atomic. But how likely is that to be a problem in practice?

With multi-threading, there is a non-zero chance of things going wrong.

For the record, calling mmap() with MAP_FIXED on a already-mapped region is specified by POSIX to replace an existing mapping. So if Cygwin does not support that, I would consider that a bug.

comment:7 Changed 3 years ago by embray

Right, I think that would be a bug too--the thread I linked to above has some comment about that.

Honestly, I might just add a patch that reverts to the old mprotect-based approach on Cygwin.

comment:8 Changed 3 years ago by embray

  • Priority changed from major to blocker

comment:9 Changed 3 years ago by embray

I spent some more time poking at this, and what I found is that it suffices on Cygwin to mprotect(..., PROT_NONE) rather than re-mmap with PROT_NONE and MAP_FIXED, which is in fact buggy (although a known issue).

This means, unfortunately, having to put an #ifdef __CYGWIN__ in pari_mainstack_mreset, to have it use mprotect instead of mmap, but other than that I think that is the only change needed. In Cygwin, mprotect(..., PROT_NONE) will actually decommit the memory, which is what we want.

comment:10 Changed 3 years ago by jdemeyer

Thanks for the analysis. Do you have a patch ready?

comment:11 Changed 3 years ago by embray

Incoming.

comment:12 Changed 3 years ago by jdemeyer

Remember to base this ticket on top of #22675.

comment:13 Changed 3 years ago by embray

Yep, that's the plan.

comment:14 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/cygwin/ticket-22810
  • Commit set to ab20b1b912075256314c7ec8c94bf799a8a76428

Still waiting on some testing.


New commits:

543c4a1Upgrade PARI to version 2.9.2
ab20b1bAnother patch to Pari's stack allocation to fix a bug on Cygwin: https://trac.sagemath.org/ticket/22810

comment:15 Changed 3 years ago by git

  • Commit changed from ab20b1b912075256314c7ec8c94bf799a8a76428 to 00a5d73376817860b85c1abcb1a10203e28d897a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

00a5d73Another patch to Pari's stack allocation to fix a bug on Cygwin: https://trac.sagemath.org/ticket/22810

comment:16 Changed 3 years ago by embray

  • Status changed from new to needs_review

Seems to work now.

comment:17 follow-up: Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Jeroen, any more comments on this? Since this worked for me on Cygwin, I would set this to a positive review. However, you know much more about Pari than I do.

comment:18 in reply to: ↑ 17 Changed 3 years ago by jdemeyer

Replying to tscrim:

Since this worked for me on Cygwin

Define "worked". Did you run the PARI testsuite? And the Sage doctests in src/sage/libs/*pari*?

If both of these pass, then it's fine for me.

comment:19 Changed 3 years ago by embray

I did all of the above, but Travis should confirm if possible.

comment:20 Changed 3 years ago by embray

Please by my CI, Travis (sorry, you probably get that all the time...)

comment:21 Changed 3 years ago by tscrim

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer
  • Status changed from needs_review to positive_review

I will be your CI. :P

Built okay and tests pass.

comment:22 Changed 3 years ago by vbraun

  • Branch changed from u/embray/cygwin/ticket-22810 to 00a5d73376817860b85c1abcb1a10203e28d897a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:23 Changed 3 years ago by embray

  • Commit 00a5d73376817860b85c1abcb1a10203e28d897a deleted
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

This still needs to be reported upstream, however.

comment:24 Changed 3 years ago by jdemeyer

Right. Should I?

Note: See TracTickets for help on using tickets.