[SwiftREPL] Fix a wrong copy-constructor assignment causing crashes. #345
Conversation
<rdar://problem/57123561>
|
Thanks to Adrian for helping me finding the root cause of this! |
|
@swift-ci test |
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
Thanks, there are a few obvious improvements that we can make while we're here.
Also: Can you = delete the copy constructor now?
| m_swift_ast = target_swift_ast; | ||
| } | ||
| SwiftASTContext *swift_ast = m_swift_ast_sp.get(); | ||
| SwiftASTContext *swift_ast = m_swift_ast; |
There was a problem hiding this comment.
I think we should get rid of m_swift_ast and only use the local copy, because it's not safe to hold on to the pointer without also holding on to the lock.
| //---------------------------------------------------------------------- | ||
| Status error; | ||
| if (!m_swift_ast_sp) { | ||
| if (!m_swift_ast) { |
There was a problem hiding this comment.
here we also need to acquire the scratch context lock. That's very easy if you just call target.GetSwiftScratchContext().
|
@swift-ci test |
|
How do we make sure this keeps working? I know the crash didn't happen in our test environment, but is there a way to check that the setup is correct by looking at some side effect maybe? |
|
Is there a separate pull request for acquiring the lock that I missed? |
|
@fredriss We already have a test for this, but it only reproduces in a particular enviroment. If we end up testing that specific environment, we'll get coverage for free. @adrian-prantl patch coming :) |
3 participants
rdar://problem/57123561