Skip to content

Commit 132159f

Browse files
DavePearceDavePearce
authored
fix: race condition in pkg/test/util/framework.go (#1197)
* use race detector for CI tests This updates the CI tests to use the race detector by default, meaning that race condition will prevent merging. * lower point of schema cloning in testing framework This lowers the point where the schema is cloned within the testing framework. The purpose of this is to ensure every trace has a unique schema, thereby avoiding sharing of the `io.Executor` across traces. * separtate SchemaStacker from a SchemaStack This allows the configuration for a SchemaStack to behave in a functional manner. Thus, a SchemaStacker is simply a configurable builder for a schema stack. * separate out specific racer tests Since using the race detector on all tests is quite expensive in terms of time, this splits out some specific tests using the race detector. * Fix #1198 This fixes a race condition in io.Executor where the read lock was released slightly too early. This additionally tweaks the set of tests being run to improve CI performance.
1 parent 12fbeda commit 132159f

File tree

19 files changed

+622
-551
lines changed

19 files changed

+622
-551
lines changed

‎.github/workflows/test.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ jobs:
88
test:
99
strategy:
1010
matrix:
11-
test: [asm-bench-test,asm-util-test,asm-unit-test,corset-test,corset-bench,unit-test]
11+
test: [asm-bench,asm-racer,asm-util,asm-unit,corset-bench,corset-racer,corset-test,unit-test]
1212
fail-fast: false
1313
runs-on: gha-runner-scale-set-ubuntu-22.04-amd64-large
1414
steps:

‎Makefile‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,30 @@ test:
2323
@echo ">>> Running All Tests..."
2424
go test --timeout 0 ./...
2525

26-
asm-bench-test:
26+
asm-racer:
27+
@echo ">>> Running Assembly Racer Tests..."
28+
go test -race --timeout 0 -run "Test_AsmUtil_FillBytes" ./...
29+
30+
asm-bench:
2731
@echo ">>> Running Assembly Benchmark Tests..."
2832
go test --timeout 0 -run "Test_AsmBench" ./...
2933

30-
asm-util-test:
34+
asm-util:
3135
@echo ">>> Running Assembly Util Tests..."
3236
go test --timeout 0 -run "Test_AsmUtil" ./...
3337

34-
asm-unit-test:
38+
asm-unit:
3539
@echo ">>> Running Assembly Unit Tests..."
36-
go test --timeout 0 -run "Test_AsmUnit" ./...
40+
go test -race --timeout 0 -run "Test_AsmUnit" ./...
3741

3842
corset-test:
3943
@echo ">>> Running Corset Tests..."
4044
go test --timeout 0 -run "Test_Agnostic|Test_Valid|Test_Invalid" ./...
4145

46+
corset-racer:
47+
@echo ">>> Running Corset Racer Tests..."
48+
go test -race --timeout 0 -run "Test_Bench_Bin|Test_Bench_Euc|Test_Bench_Mul" ./...
49+
4250
corset-bench:
4351
@echo ">>> Running Corset Benchmark Tests..."
4452
go test --timeout 0 -run "Test_Bench" ./...

‎pkg/asm/io/executor.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,13 +110,17 @@ func (p *FunctionTrace[T]) Call(inputs []big.Int, iomap Map) FunctionInstance {
110110
p.mux.RLock()
111111
// Look for cached instance
112112
index := p.instances.Find(iostate)
113-
// Release read lock
114-
p.mux.RUnlock()
115113
// Check for cache hit.
116114
if index != math.MaxUint {
117115
// Yes, therefore return precomputed outputs
118-
return p.instances[index]
116+
instance := p.instances[index]
117+
// Release read lock
118+
p.mux.RUnlock()
119+
//
120+
return instance
119121
}
122+
// Release read lock
123+
p.mux.RUnlock()
120124
// Execute function to determine new outputs.
121125
return p.executeCall(inputs, iomap)
122126
}

‎pkg/cmd/check.go‎

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
"github.com/consensys/go-corset/pkg/cmd/check"
2525
cmd_util "github.com/consensys/go-corset/pkg/cmd/util"
2626
"github.com/consensys/go-corset/pkg/corset"
27-
"github.com/consensys/go-corset/pkg/ir"
2827
sc "github.com/consensys/go-corset/pkg/schema"
2928
"github.com/consensys/go-corset/pkg/schema/constraint"
3029
"github.com/consensys/go-corset/pkg/schema/constraint/lookup"
@@ -165,7 +164,7 @@ type checkConfig struct {
165164

166165
// Check raw constraints using the legacy pipeline.
167166
func checkWithLegacyPipeline[F field.Element[F]](cfg checkConfig, batched bool, tracefile string,
168-
schemas cmd_util.SchemaStack[F]) {
167+
schemas cmd_util.SchemaStacker[F]) {
169168
//
170169
var (
171170
errors []error
@@ -195,10 +194,7 @@ func checkWithLegacyPipeline[F field.Element[F]](cfg checkConfig, batched bool,
195194
}
196195
// Go!
197196
if len(errors) == 0 {
198-
for i, schema := range schemas.ConcreteSchemas() {
199-
ir := schemas.ConcreteIrName(uint(i))
200-
ok = checkTrace(ir, traces, schema, schemas.TraceBuilder(), cfg) && ok
201-
}
197+
ok = checkTraces(traces, schemas, cfg) && ok
202198
}
203199
// Handle errors
204200
if !ok || len(errors) > 0 {
@@ -210,31 +206,39 @@ func checkWithLegacyPipeline[F field.Element[F]](cfg checkConfig, batched bool,
210206
}
211207
}
212208

213-
func checkTrace[F field.Element[F]](ir string, traces []lt.TraceFile, schema sc.AnySchema[F],
214-
builder ir.TraceBuilder[F], cfg checkConfig) bool {
209+
func checkTraces[F field.Element[F]](traces []lt.TraceFile, stacker cmd_util.SchemaStacker[F], cfg checkConfig) bool {
215210
//
216211
for _, tf := range traces {
212+
//
217213
for n := cfg.padding.Left; n <= cfg.padding.Right; n++ {
218-
//
219-
stats := util.NewPerfStats()
220-
trace, errs := builder.WithPadding(n).Build(schema, tf)
221-
// Log cost of expansion
222-
stats.Log("Expanding trace columns")
223-
// Report any errors
224-
reportErrors(ir, errs)
225-
// Check whether considered unrecoverable
226-
if trace == nil || len(errs) > 0 {
227-
return false
228-
}
229-
//
230-
stats = util.NewPerfStats()
231-
// Check constraints
232-
if errs := sc.Accepts(builder.Parallelism(), builder.BatchSize(), schema, trace); len(errs) > 0 {
233-
reportFailures(ir, errs, trace, cfg)
234-
return false
235-
}
214+
// Configure stack. This is important to ensure true separation
215+
// between runs (e.g. for the io.Executor).
216+
stack := stacker.Build()
217+
// configure trace builder
218+
builder := stack.TraceBuilder().WithPadding(n)
219+
// Run each concrete schema separately
220+
for i, schema := range stack.ConcreteSchemas() {
221+
ir := stack.ConcreteIrName(uint(i))
222+
stats := util.NewPerfStats()
223+
trace, errs := builder.Build(schema, tf)
224+
// Log cost of expansion
225+
stats.Log("Expanding trace columns")
226+
// Report any errors
227+
reportErrors(ir, errs)
228+
// Check whether considered unrecoverable
229+
if trace == nil || len(errs) > 0 {
230+
return false
231+
}
232+
//
233+
stats = util.NewPerfStats()
234+
// Check constraints
235+
if errs := sc.Accepts(builder.Parallelism(), builder.BatchSize(), schema, trace); len(errs) > 0 {
236+
reportFailures(ir, errs, trace, cfg)
237+
return false
238+
}
236239

237-
stats.Log("Checking constraints")
240+
stats.Log("Checking constraints")
241+
}
238242
}
239243
}
240244
// Done

‎pkg/cmd/debug.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ import (
1616
"fmt"
1717
"os"
1818

19+
"github.com/consensys/go-corset/pkg/binfile"
1920
"github.com/consensys/go-corset/pkg/cmd/debug"
20-
cmd_util "github.com/consensys/go-corset/pkg/cmd/util"
2121
"github.com/consensys/go-corset/pkg/corset"
2222
sc "github.com/consensys/go-corset/pkg/schema"
2323
"github.com/consensys/go-corset/pkg/util/field"
@@ -65,30 +65,31 @@ func runDebugCmd[F field.Element[F]](cmd *cobra.Command, args []string) {
6565
spillage := GetFlag(cmd, "spillage")
6666
textWidth := GetUint(cmd, "textwidth")
6767
// Read in constraint files
68-
schemas := *getSchemaStack[F](cmd, SCHEMA_DEFAULT_MIR, args...)
68+
stacker := *getSchemaStack[F](cmd, SCHEMA_DEFAULT_MIR, args...)
69+
stack := stacker.Build()
6970
// Print constant info (if requested)
7071
if constants {
71-
debug.PrintExternalisedConstants(schemas)
72+
debug.PrintExternalisedConstants(stack.BinaryFile())
7273
}
7374
// Print spillage info (if requested)
7475
if spillage {
75-
printSpillage(schemas, true)
76+
printSpillage(stack.BinaryFile(), true)
7677
}
7778
// Print meta-data (if requested)
7879
if metadata {
79-
printBinaryFileHeader(schemas)
80+
printBinaryFileHeader(stack.BinaryFile())
8081
}
8182
// Print stats (if requested)
8283
if stats {
83-
debug.PrintStats(schemas)
84+
debug.PrintStats(stack)
8485
}
8586
// Print embedded attributes (if requested
8687
if attrs {
87-
printAttributes(schemas)
88+
printAttributes(stack.BinaryFile())
8889
}
8990
//
9091
if !stats && !attrs {
91-
debug.PrintSchemas(schemas, textWidth)
92+
debug.PrintSchemas(stack, textWidth)
9293
}
9394
}
9495

@@ -102,10 +103,9 @@ func init() {
102103
debugCmd.Flags().Uint("textwidth", 130, "Set maximum textwidth to use")
103104
}
104105

105-
func printAttributes[F field.Element[F]](schemas cmd_util.SchemaStack[F]) {
106-
binfile := schemas.BinaryFile()
106+
func printAttributes(binf *binfile.BinaryFile) {
107107
// Print attributes
108-
for _, attr := range binfile.Attributes {
108+
for _, attr := range binf.Attributes {
109109
fmt.Printf("attribute \"%s\":\n", attr.AttributeName())
110110
//
111111
if attr.AttributeName() == "CorsetSourceMap" {
@@ -114,7 +114,7 @@ func printAttributes[F field.Element[F]](schemas cmd_util.SchemaStack[F]) {
114114
}
115115
}
116116

117-
func printSpillage[F field.Element[F]](schemas cmd_util.SchemaStack[F], defensive bool) {
117+
func printSpillage(binf *binfile.BinaryFile, defensive bool) {
118118
// fmt.Println("Spillage:")
119119
// // Compute spillage for optimisation level
120120
// spillage := determineSpillage(&binf.Schema, defensive, optConfig)
@@ -135,8 +135,8 @@ func printSpillage[F field.Element[F]](schemas cmd_util.SchemaStack[F], defensiv
135135
panic("todo")
136136
}
137137

138-
func printBinaryFileHeader[F field.Element[F]](schemas cmd_util.SchemaStack[F]) {
139-
header := schemas.BinaryFile().Header
138+
func printBinaryFileHeader(binf *binfile.BinaryFile) {
139+
header := binf.Header
140140
//
141141
fmt.Printf("Format: %d.%d\n", header.MajorVersion, header.MinorVersion)
142142
// Attempt to parse metadata

‎pkg/cmd/debug/constants.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,12 @@ import (
1717
"math"
1818

1919
"github.com/consensys/go-corset/pkg/binfile"
20-
cmd_util "github.com/consensys/go-corset/pkg/cmd/util"
2120
"github.com/consensys/go-corset/pkg/corset"
22-
"github.com/consensys/go-corset/pkg/util/field"
2321
)
2422

2523
// PrintExternalisedConstants is responsible for printing any externalised
2624
// constants contained within the given binary file.
27-
func PrintExternalisedConstants[F field.Element[F]](schemas cmd_util.SchemaStack[F]) {
28-
binf := schemas.BinaryFile()
25+
func PrintExternalisedConstants(binf *binfile.BinaryFile) {
2926
//
3027
fmt.Println("External constants:")
3128
// Sanity check debug information is available.

‎pkg/cmd/generate.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func runGenerateCmd[F field.Element[F]](cmd *cobra.Command, args []string) {
6464
intrface := GetString(cmd, "interface")
6565
// Parse constraints
6666
files := splitConstraintSets(args)
67-
schemas := make([]cmd_util.SchemaStack[F], len(files))
67+
schemas := make([]cmd_util.SchemaStacker[F], len(files))
6868
//
6969
for i := range schemas {
7070
schemas[i] = *getSchemaStack[F](cmd, SCHEMA_DEFAULT_AIR, files[i]...)
@@ -87,11 +87,13 @@ func runGenerateCmd[F field.Element[F]](cmd *cobra.Command, args []string) {
8787
super = strings.TrimSuffix(filename, ".java")
8888
}
8989
//
90-
for i, stack := range schemas {
90+
for i, stacker := range schemas {
9191
var (
9292
filename = outputs[i]
93-
binf = stack.BinaryFile()
93+
binf = stacker.BinaryFile()
9494
)
95+
// build schema stack
96+
stack := stacker.Build()
9597
// NOTE: assume defensive padding is enabled.
9698
spillage := determineSpillage(stack.LowestConcreteSchema(), true)
9799
// Generate appropriate Java source

‎pkg/cmd/generate/interface.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,13 @@ import (
2828
// JavaTraceInterfaceUnion generates a suitable interface capturing the given schema,
2929
// as outlined in the source map.
3030
func JavaTraceInterfaceUnion[F field.Element[F]](filename string, pkgname string,
31-
stacks []cmd_util.SchemaStack[F]) (string, error) {
31+
stacks []cmd_util.SchemaStacker[F]) (string, error) {
3232
//
3333
return javaTraceInterface(filename, pkgname, true, stacks)
3434
}
3535

3636
func javaTraceInterface[F field.Element[F]](filename string, pkgname string, union bool,
37-
stacks []cmd_util.SchemaStack[F]) (string, error) {
37+
stacks []cmd_util.SchemaStacker[F]) (string, error) {
3838
//
3939
var root corset.SourceModule
4040
// Combine roots to determine set of common functionality.

‎pkg/cmd/inspect.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,19 +61,20 @@ func runInspectCmd[F field.Element[F]](cmd *cobra.Command, args []string) {
6161
os.Exit(1)
6262
}
6363
// Read in constraint files
64-
schemas := *getSchemaStack[F](cmd, SCHEMA_DEFAULT_MIR, args[1:]...)
64+
stacker := *getSchemaStack[F](cmd, SCHEMA_DEFAULT_MIR, args[1:]...)
65+
stack := stacker.Build()
6566
//
6667
stats := util.NewPerfStats()
6768
// Parse constraints
68-
binf := schemas.BinaryFile()
69+
binf := stacker.BinaryFile()
6970
// Determine whether expansion is being performed
70-
expanding := schemas.TraceBuilder().Expanding()
71+
expanding := stack.TraceBuilder().Expanding()
7172
// Sanity check debug information is available.
7273
srcmap, srcmap_ok := binfile.GetAttribute[*corset.SourceMap](binf)
7374
//
7475
if !srcmap_ok {
7576
fmt.Printf("binary file \"%s\" missing source map", args[1])
76-
} else if !schemas.HasUniqueSchema() {
77+
} else if !stack.HasUniqueSchema() {
7778
fmt.Println("must specify exactly one of --air/mir/uasm/asm")
7879
os.Exit(2)
7980
}
@@ -82,7 +83,7 @@ func runInspectCmd[F field.Element[F]](cmd *cobra.Command, args []string) {
8283
// Parse trace file
8384
tracefile := ReadTraceFile(args[0])
8485
// Extract scheam
85-
schema := schemas.UniqueConcreteSchema()
86+
schema := stack.UniqueConcreteSchema()
8687
//
8788
stats.Log("Reading trace file")
8889
//
@@ -92,7 +93,7 @@ func runInspectCmd[F field.Element[F]](cmd *cobra.Command, args []string) {
9293
}
9394
// Apply trace expansion
9495
if len(errors) == 0 {
95-
trace, errors = schemas.TraceBuilder().Build(schema, tracefile)
96+
trace, errors = stack.TraceBuilder().Build(schema, tracefile)
9697
}
9798
//
9899
if len(errors) == 0 {

‎pkg/cmd/root.go‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,9 @@ func runFieldAgnosticCmd(cmd *cobra.Command, args []string, cmds []FieldAgnostic
106106
os.Exit(2)
107107
}
108108

109-
func getSchemaStack[F field.Element[F]](cmd *cobra.Command, mode uint, filenames ...string) *cmd_util.SchemaStack[F] {
109+
func getSchemaStack[F field.Element[F]](cmd *cobra.Command, mode uint, filenames ...string) *cmd_util.SchemaStacker[F] {
110110
var (
111-
schemaStack cmd_util.SchemaStack[F]
111+
stacker cmd_util.SchemaStacker[F]
112112
corsetConfig corset.CompilationConfig
113113
asmConfig asm.LoweringConfig
114114
field = GetString(cmd, "field")
@@ -171,37 +171,38 @@ func getSchemaStack[F field.Element[F]](cmd *cobra.Command, mode uint, filenames
171171
WithParallelism(parallel).
172172
WithBatchSize(batchSize)
173173
// Configure the stack
174-
schemaStack.WithAssemblyConfig(asmConfig)
175-
schemaStack.WithCorsetConfig(corsetConfig)
176-
schemaStack.WithOptimisationConfig(mir.OPTIMISATION_LEVELS[optimisation])
177-
schemaStack.WithConstantDefinitions(externs)
174+
stacker = stacker.
175+
WithAssemblyConfig(asmConfig).
176+
WithCorsetConfig(corsetConfig).
177+
WithOptimisationConfig(mir.OPTIMISATION_LEVELS[optimisation]).
178+
WithConstantDefinitions(externs)
178179
//
179180
if asmEnable {
180-
schemaStack.WithLayer(cmd_util.MACRO_ASM_LAYER)
181+
stacker = stacker.WithLayer(cmd_util.MACRO_ASM_LAYER)
181182
}
182183
//
183184
if uasmEnable {
184-
schemaStack.WithLayer(cmd_util.MICRO_ASM_LAYER)
185+
stacker = stacker.WithLayer(cmd_util.MICRO_ASM_LAYER)
185186
}
186187
//
187188
if mirEnable {
188-
schemaStack.WithLayer(cmd_util.MIR_LAYER)
189+
stacker = stacker.WithLayer(cmd_util.MIR_LAYER)
189190
}
190191
//
191192
if airEnable {
192-
schemaStack.WithLayer(cmd_util.AIR_LAYER)
193+
stacker = stacker.WithLayer(cmd_util.AIR_LAYER)
193194
}
194195
// Read / compile given source files.
195196
if mode != SCHEMA_OPTIONAL || len(filenames) > 0 {
196-
schemaStack.Read(filenames...)
197+
stacker = stacker.Read(filenames...)
197198
} else {
198199
// In this situation, we cannot perform trace expansion.
199200
builder = builder.WithExpansion(false)
200201
}
201202
// Configure builder
202-
schemaStack.WithTraceBuilder(builder)
203+
stacker = stacker.WithTraceBuilder(builder)
203204
// Done
204-
return &schemaStack
205+
return &stacker
205206
}
206207

207208
func init() {

0 commit comments

Comments
 (0)