Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "fixup: remove boehmgc patch" #7679

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 24, 2023

Motivation

The patch is still necessary.

Should fix #7644

Context

The patch is still necessary.
@lheckemann @bb010g @edolstra, please do your research, or just ask the author (which happens to be me, but that doesn't matter).

An evaluator like this is not an environment where "it compiles, so it works" will ever hold.

This reverts commit 1c40182.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
    • n/a
  • agreed on implementation strategy
    • n/a
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests
  • documentation in the manual
    • n/a
  • code and comments are self-explanatory
    • can explain later if desired
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

It is still necessary.
Please do your research, or f ask the author, which happens to be me.

An evaluator like this is not an environment where "it compiles, so
it works" will ever hold.

This reverts commit 1c40182.
@roberth roberth marked this pull request as draft January 24, 2023 13:57
@roberth
Copy link
Member Author

roberth commented Jan 24, 2023

Draft for now because the patch doesn't actually apply completely; the reverted commit doesn't cover the whole change.

@roberth roberth marked this pull request as ready for review January 24, 2023 14:12
@cole-h
Copy link
Member

cole-h commented Jan 24, 2023

I don't know how easy it would be, but is there a way to test this that we could integrate into the test suite (or even as a hydra job)? As in, a way to validate that the patch is still required (not that it ever might not be required, but mostly to just avoid this situation where removing it seems to be fine, but causes issues later on), or that the patch is not properly applied...?

@roberth
Copy link
Member Author

roberth commented Jan 24, 2023

We could run some large evaluations, but Nixpkgs is unlikely to be GC-ed while a source filtering coroutine is active.
So for me personally it's not a done deal and I'd rather work to improve the GC/coroutine integration in a way that works with upstream. That said, upstream is iterating on a redesign of the interfaces to better support such an integration, so for now I'm going to leave it be.
If I'm wrong to be pessimistic about finding a reliable reproducer, I'd be happy to include it in the test suite of course.

@cole-h
Copy link
Member

cole-h commented Jan 24, 2023

Upstream integration would be even better!

@github-actions
Copy link

Successfully created backport PR for 2.13-maintenance:

@Ericson2314
Copy link
Member

Perhaps we could make some sort of unit test that manually triggers a GC. Hard/impossible to do externally I agree, but perhaps possible internally.

This would be a nice thing to have even if we don't need the patch because vanilla Boehm GC handles such things.

@roberth
Copy link
Member Author

roberth commented Jan 24, 2023

Actually I did add a triggerGC primop originally, but Eelco rejected that. An identity function with a side effect, not unlike trace. You could trigger it in a source filter predicate to make sure a coroutine is active during GC.

@Ericson2314
Copy link
Member

Let's have a nix-testing experimental feature so we can make such things for testing purposes without qualms.

yorickvP added a commit to yorickvP/nix that referenced this pull request Jan 31, 2023
yorickvP added a commit to yorickvP/nix that referenced this pull request Mar 1, 2023
yorickvP added a commit to yorickvP/nix that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in libgc.so in Nix 2.13
3 participants