[go] runtime: add race instrumentation for moduleToTypelinksLock
Chressie Himpel (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Cherry Mui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 1 comment![]( "Open in Gerrit")
We don't have to add explicit raceacquire calls for other locks in the runtime. What is different about this one?
Related details
- Cherry Mui
- Chressie Himpel
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 1 comment![]( "Open in Gerrit")
We don't have to add explicit raceacquire calls for other locks in the runtime. What is different about this one?
Or, to put it another way, how can I reproduce the problem? "go test -race reflect" doesn't do it. I don't see any failures on the build dashboard.
Related details
- Cherry Mui
- Chressie Himpel
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
Ian Lance TaylorWe don't have to add explicit raceacquire calls for other locks in the runtime. What is different about this one?
Or, to put it another way, how can I reproduce the problem? "go test -race reflect" doesn't do it. I don't see any failures on the build dashboard.
The reason why we need race instrumentation is tricky. The runtime package is not compiled with race instrumentation, therefore the race detector doesn't know about synchronizations within the runtime, and usually it doesn't care, as it also doesn't know about the concurrent data accesses in the runtime.
However, in this case the concurrent access is in a map. Map accesses are always instrumented https://cs.opensource.google/go/go/+/master:src/internal/runtime/maps/runtime_fast64.go;l=25 , even if the map is defined in the runtime. So the race detector does see the concurrent access. But it doesn't see the synchronization. So this CL adds them.
Go maps are not common in the runtime. But I recall an issue like this did occur. Maybe we could make the compiler/runtime not to instrument map accesses for maps defined in the runtime (which would be a more involved change).
I'll see if I can work out a reproducer.
Thanks.
Related details
- Chressie Himpel
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Lance Taylor
| Code-Review | +2 |
1 comment
Ian Lance TaylorWe don't have to add explicit raceacquire calls for other locks in the runtime. What is different about this one?
Cherry MuiOr, to put it another way, how can I reproduce the problem? "go test -race reflect" doesn't do it. I don't see any failures on the build dashboard.
The reason why we need race instrumentation is tricky. The runtime package is not compiled with race instrumentation, therefore the race detector doesn't know about synchronizations within the runtime, and usually it doesn't care, as it also doesn't know about the concurrent data accesses in the runtime.
However, in this case the concurrent access is in a map. Map accesses are always instrumented https://cs.opensource.google/go/go/+/master:src/internal/runtime/maps/runtime_fast64.go;l=25 , even if the map is defined in the runtime. So the race detector does see the concurrent access. But it doesn't see the synchronization. So this CL adds them.
Go maps are not common in the runtime. But I recall an issue like this did occur. Maybe we could make the compiler/runtime not to instrument map accesses for maps defined in the runtime (which would be a more involved change).
I'll see if I can work out a reproducer.
Thanks.
- Cherry Mui
- Chressie Himpel
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
Ian Lance TaylorWe don't have to add explicit raceacquire calls for other locks in the runtime. What is different about this one?
Cherry MuiOr, to put it another way, how can I reproduce the problem? "go test -race reflect" doesn't do it. I don't see any failures on the build dashboard.
Ian Lance TaylorThe reason why we need race instrumentation is tricky. The runtime package is not compiled with race instrumentation, therefore the race detector doesn't know about synchronizations within the runtime, and usually it doesn't care, as it also doesn't know about the concurrent data accesses in the runtime.
However, in this case the concurrent access is in a map. Map accesses are always instrumented https://cs.opensource.google/go/go/+/master:src/internal/runtime/maps/runtime_fast64.go;l=25 , even if the map is defined in the runtime. So the race detector does see the concurrent access. But it doesn't see the synchronization. So this CL adds them.
Go maps are not common in the runtime. But I recall an issue like this did occur. Maybe we could make the compiler/runtime not to instrument map accesses for maps defined in the runtime (which would be a more involved change).
I'll see if I can work out a reproducer.
Thanks.
I see, thanks.
https://go.dev/play/p/7_kGki3MMJi is a reproducer, which fails in race mode.
Related details
- Chressie Himpel
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +2 |
1 comment
Ian Lance TaylorWe don't have to add explicit raceacquire calls for other locks in the runtime. What is different about this one?
Cherry MuiOr, to put it another way, how can I reproduce the problem? "go test -race reflect" doesn't do it. I don't see any failures on the build dashboard.
Ian Lance TaylorThe reason why we need race instrumentation is tricky. The runtime package is not compiled with race instrumentation, therefore the race detector doesn't know about synchronizations within the runtime, and usually it doesn't care, as it also doesn't know about the concurrent data accesses in the runtime.
However, in this case the concurrent access is in a map. Map accesses are always instrumented https://cs.opensource.google/go/go/+/master:src/internal/runtime/maps/runtime_fast64.go;l=25 , even if the map is defined in the runtime. So the race detector does see the concurrent access. But it doesn't see the synchronization. So this CL adds them.
Go maps are not common in the runtime. But I recall an issue like this did occur. Maybe we could make the compiler/runtime not to instrument map accesses for maps defined in the runtime (which would be a more involved change).
I'll see if I can work out a reproducer.
Thanks.
Cherry MuiI see, thanks.
https://go.dev/play/p/7_kGki3MMJi is a reproducer, which fails in race mode.
Let's add a test. Thanks.
It might need to be a standalone program, as moduleTypelinks is initialized only once. If it is already initialized in the sequential part of the program, it won't race.
Related details
- Chressie Himpel
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chressie Himpel (Gerrit)
Chressie Himpel uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Cherry Mui, +2 by Ian Lance Taylor
Related details
- Cherry Mui
- Chressie Himpel
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chressie Himpel (Gerrit)
Chressie Himpel added 1 comment![]( "Open in Gerrit")
We don't have to add explicit raceacquire calls for other locks in the runtime. What is different about this one?
Cherry MuiOr, to put it another way, how can I reproduce the problem? "go test -race reflect" doesn't do it. I don't see any failures on the build dashboard.
Ian Lance TaylorThe reason why we need race instrumentation is tricky. The runtime package is not compiled with race instrumentation, therefore the race detector doesn't know about synchronizations within the runtime, and usually it doesn't care, as it also doesn't know about the concurrent data accesses in the runtime.
However, in this case the concurrent access is in a map. Map accesses are always instrumented https://cs.opensource.google/go/go/+/master:src/internal/runtime/maps/runtime_fast64.go;l=25 , even if the map is defined in the runtime. So the race detector does see the concurrent access. But it doesn't see the synchronization. So this CL adds them.
Go maps are not common in the runtime. But I recall an issue like this did occur. Maybe we could make the compiler/runtime not to instrument map accesses for maps defined in the runtime (which would be a more involved change).
I'll see if I can work out a reproducer.
Thanks.
Cherry MuiI see, thanks.
Cherry Muihttps://go.dev/play/p/7_kGki3MMJi is a reproducer, which fails in race mode.
Let's add a test. Thanks.
It might need to be a standalone program, as moduleTypelinks is initialized only once. If it is already initialized in the sequential part of the program, it won't race.
Thanks! I added your explanation to the CL description and your reproducer as test/fixedbugs/bug519.go. I tested it with and without the instrumentation.
Please submit, if you think it's in a proper state now. Thanks.
Related details
- Cherry Mui
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |