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

Protect shared JIT variables from being modified unsafely #44914

Merged
merged 6 commits into from
Apr 19, 2022

Conversation

pchintalapudi
Copy link
Member

@pchintalapudi pchintalapudi commented Apr 8, 2022

Depends on #44912 Depends on #45016 to fix the macos segfaults

@pchintalapudi pchintalapudi changed the base branch from master to pc/jit-pool April 8, 2022 19:54
@pchintalapudi pchintalapudi force-pushed the pc/jit-lockdown branch 2 times, most recently from ade0463 to 0672563 Compare April 11, 2022 21:41
Base automatically changed from pc/jit-pool to master April 12, 2022 01:59
@pchintalapudi pchintalapudi marked this pull request as ready for review April 12, 2022 02:26
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 12, 2022

seems related?

libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument
  |  
  | signal (6): Abort trap: 6
  | in expression starting at none:0
  |  
  | signal (11): Segmentation fault: 11
  | in expression starting at none:0
  | threads                                   (1) \|         failed at 2022-04-12T03:35:31.109
 

@pchintalapudi
Copy link
Member Author

The pthread documentation suggests that this is caused by attempting to operate on an uninitialized mutex, which could probably happen during setup or teardown (I think the mutex should be initialized once the constructor of EE finishes). Is adding an initialized boolean flag that we assert on every operation too expensive to detect such errors?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 12, 2022

I don't think you can solve race conditions by adding more data races. Oft times, I simply try to leak the memory to avoid cleanup problems, if that is the issue here.

@pchintalapudi pchintalapudi force-pushed the pc/jit-lockdown branch 2 times, most recently from 9bdafdb to c9294ea Compare April 18, 2022 21:33
@pchintalapudi pchintalapudi marked this pull request as draft April 18, 2022 21:37
@pchintalapudi pchintalapudi changed the base branch from master to pc/debuginfo April 19, 2022 06:15
Base automatically changed from pc/debuginfo to master April 19, 2022 07:02
@pchintalapudi pchintalapudi marked this pull request as ready for review April 19, 2022 07:13
@pchintalapudi
Copy link
Member Author

@vtjnash Looks like the macos mutex errors are fixed, does this look good to merge as-is?

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 19, 2022
@pchintalapudi pchintalapudi merged commit ab11173 into master Apr 19, 2022
@pchintalapudi pchintalapudi deleted the pc/jit-lockdown branch April 19, 2022 22:32
@oscardssmith oscardssmith removed the merge me PR is reviewed. Merge when all tests are passing label Apr 19, 2022
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.

3 participants