Remove functions/methods that work on the global context #54
Conversation
This usually works by chance, but leads to crashes. So we should never ever do this. I'm pretty sure this is the crash behind this issue: #3894 It may also have caused this crash: #3874 I have a suspicion this is also behind the rather crash-prone CircleCI jobs, that we haven't been able to find the source of. But we'll find out soon enough once this fix is merged. To avoid hitting this issue again in the future, I've created a PR to remove these dangerous functions altogether from the go-llvm API: tinygo-org/go-llvm#54
The global context should almost never be used, but it's very easy to use accidentally. Therefore, I'd like to remove all convenience functions that use the global context. All functionality that got removed is still accessible as methods on llvm.Context, and the global context can be obtained using llvm.GlobalContext(). So no actual functionality is lost, just the very easy to misuse "convenience" functions.
aykevl
force-pushed
the
remove-global-context
branch
from
1b703af
to
3c9a573
Compare
This usually works by chance, but leads to crashes. So we should never ever do this. I'm pretty sure this is the crash behind this issue: #3894 It may also have caused this crash: #3874 I have a suspicion this is also behind the rather crash-prone CircleCI jobs, that we haven't been able to find the source of. But we'll find out soon enough once this fix is merged. To avoid hitting this issue again in the future, I've created a PR to remove these dangerous functions altogether from the go-llvm API: tinygo-org/go-llvm#54
|
@aykevl I agree with you on this one. If I am using any of these functions it's a bug I am happy to fix. I am trying to have my test suite run in parallel and I am not sure if Go's race detector would find any bugs where I have accidentally used the global context. I believe I am currently experiencing some flakes due to data races and I don't think the race detector found them so this could very well be the cause. So thank you! |
It does not. The only symptom I know is weird crashes, and that could have lots of causes (not just from using the global context). In my experience, segmentation faults are one of the more common crashes.
Yeah, the global context is inherently not thread safe, so it should never be used from multiple goroutines at the same time. The only way to make the compiler work in parallel is by using separate contexts (and never use the same context from more than one goroutine or thread). Anyway, thank you for your feedback! |
The global context should almost never be used, but it's very easy to use accidentally. Therefore, I'd like to remove all convenience functions that use the global context.
All functionality that got removed is still accessible as methods on llvm.Context, and the global context can be obtained using llvm.GlobalContext(). So no actual functionality is lost, just the very easy to misuse "convenience" functions.
@rj45 any opinion on this? Because I suspect you're the only other user of this package and this is a breaking API change.