Skip to content

Commit

Permalink
api.go.tmpl: Split Read() and Write() into MustX and X variants.
Browse files Browse the repository at this point in the history
Use the non-must form to handle context cancellation correctly in hand written code.

Fixes #1123
  • Loading branch information
ben-clayton committed Oct 10, 2017
1 parent 88fe53e commit 0a79f3f
Show file tree
Hide file tree
Showing 20 changed files with 292 additions and 229 deletions.
4 changes: 2 additions & 2 deletions gapis/api/gles/compat.go
Original file line number Diff line number Diff line change
Expand Up @@ -413,10 +413,10 @@ func compat(ctx context.Context, device *device.Instance) (transform.Transformer

case *GlVertexAttrib4fv:
if oldAttrib, ok := c.Vertex.Attributes[cmd.Location]; ok {
oldValue := oldAttrib.Value.Read(ctx, cmd, s, nil /* builder */)
oldValue := oldAttrib.Value.MustRead(ctx, cmd, s, nil /* builder */)
cmd.Mutate(ctx, id, s, nil /* no builder, just mutate */)
newAttrib := c.Vertex.Attributes[cmd.Location]
newValue := newAttrib.Value.Read(ctx, cmd, s, nil /* builder */)
newValue := newAttrib.Value.MustRead(ctx, cmd, s, nil /* builder */)
if reflect.DeepEqual(oldValue, newValue) {
// Ignore the call if it is redundant.
// Some applications iterate over all arrays and explicitly initialize them.
Expand Down
7 changes: 5 additions & 2 deletions gapis/api/gles/compat_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,11 @@ func (c glShaderSourceCompatTest) run(t *testing.T) {
s := newState(ctx)
api.MutateCmds(ctx, s, nil, mw.Cmds...)

srcPtr := cmd.Source.Read(ctx, cmd, s, nil) // 0'th glShaderSource string pointer
got := strings.TrimRight(string(memory.CharToBytes(srcPtr.StringSlice(ctx, s).Read(ctx, cmd, s, nil))), "\x00")
srcPtr, err := cmd.Source.Read(ctx, cmd, s, nil) // 0'th glShaderSource string pointer
if err != nil {
t.Errorf("cmd.Source.Read returned: %v", err)
}
got := strings.TrimRight(string(memory.CharToBytes(srcPtr.StringSlice(ctx, s).MustRead(ctx, cmd, s, nil))), "\x00")

expected, err := glslCompat(ctx, c.source, c.lang, nil, dev)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion gapis/api/gles/custom_replay.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ func (ω *CGLCreateContext) Mutate(ctx context.Context, id api.CmdID, s *api.Glo
if b == nil || err != nil {
return err
}
ctxID := uint32(GetState(s).CGLContexts[ω.Ctx.Read(ctx, ω, s, b)].Identifier)
ctxID := uint32(GetState(s).CGLContexts[ω.Ctx.MustRead(ctx, ω, s, b)].Identifier)
cb := CommandBuilder{Thread: ω.Thread()}
return cb.ReplayCreateRenderer(ctxID).Mutate(ctx, id, s, b)
}
Expand Down
2 changes: 1 addition & 1 deletion gapis/api/gles/dependency_graph_behaviour_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ func getAllUsedTextureData(ctx context.Context, cmd api.Cmd, id api.CmdID, s *ap
}
for i := 0; i < int(activeUniform.ArraySize); i++ {
uniform := prog.Uniforms[activeUniform.Location+UniformLocation(i)]
units := AsU32ˢ(uniform.Value, s.MemoryLayout).Read(ctx, cmd, s, nil)
units := AsU32ˢ(uniform.Value, s.MemoryLayout).MustRead(ctx, cmd, s, nil)
if len(units) == 0 {
units = []uint32{0} // The uniform was not set, so use default value.
}
Expand Down
5 changes: 4 additions & 1 deletion gapis/api/gles/draw_call_mesh.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,10 @@ func vertexStreamData(

// Only read as much data as we actually have.
size := u64.Min(uint64(compactSize+ /* total size of gaps */ gap*(vectorCount-1)), slice.count-base)
data := slice.Slice(base, base+size, s.MemoryLayout).Read(ctx, nil, s, nil)
data, err := slice.Slice(base, base+size, s.MemoryLayout).Read(ctx, nil, s, nil)
if err != nil {
return nil, err
}
if gap > 0 {
// Adjust vectorCount to the number of complete vectors found in data.
vectorCount := sint.Min((gap+len(data))/vectorStride, vectorCount)
Expand Down
48 changes: 26 additions & 22 deletions gapis/api/gles/markers.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,48 +22,52 @@ import (
"github.com/google/gapid/gapis/memory"
)

func readString(ϟctx context.Context, ϟa api.Cmd, ϟs *api.GlobalState, at memory.Pointer, length GLsizei) string {
ptr := NewCharᵖ(at)
if length > 0 {
chars, err := ptr.Slice(0, uint64(length), ϟs.MemoryLayout).Read(ϟctx, ϟa, ϟs, nil)
if err != nil {
return ""
}
return string(memory.CharToBytes(chars))
}
chars, err := ptr.StringSlice(ϟctx, ϟs).Read(ϟctx, ϟa, ϟs, nil)
if err != nil {
return ""
}
return strings.TrimRight(string(memory.CharToBytes(chars)), "\x00")
}

// Label returns the user maker name.
func (ϟa *GlPushGroupMarkerEXT) Label(ϟctx context.Context, ϟs *api.GlobalState) string {
ptr := Charᵖ(ϟa.Marker)
if ϟa.Length > 0 {
return string(memory.CharToBytes(ptr.Slice(0, uint64(ϟa.Length), ϟs.MemoryLayout).Read(ϟctx, ϟa, ϟs, nil)))
}
return strings.TrimRight(string(memory.CharToBytes(ptr.StringSlice(ϟctx, ϟs).Read(ϟctx, ϟa, ϟs, nil))), "\x00")
return readString(ϟctx, ϟa, ϟs, ϟa.Marker, ϟa.Length)
}

// Label returns the user maker name.
func (ϟa *GlInsertEventMarkerEXT) Label(ϟctx context.Context, ϟs *api.GlobalState) string {
ptr := Charᵖ(ϟa.Marker)
if ϟa.Length > 0 {
return string(memory.CharToBytes(ptr.Slice(0, uint64(ϟa.Length), ϟs.MemoryLayout).Read(ϟctx, ϟa, ϟs, nil)))
}
return strings.TrimRight(string(memory.CharToBytes(ptr.StringSlice(ϟctx, ϟs).Read(ϟctx, ϟa, ϟs, nil))), "\x00")
return readString(ϟctx, ϟa, ϟs, ϟa.Marker, ϟa.Length)
}

// Label returns the user maker name.
func (ϟa *GlPushDebugGroup) Label(ϟctx context.Context, ϟs *api.GlobalState) string {
ptr := Charᵖ(ϟa.Message)
// This is incorrect, fudging for a bug in Unity which has been fixed but not
// rolled out.
// See https://github.com/google/gapid/issues/459 for reference.
//
// ϟa.Length should only be treated as null-terminated if ϟa.Length is < 0.
//
// TODO: Consider removing once the fixed version is mainstream.
// if ϟa.Length >= 0 {
if ϟa.Length > 0 {
return string(memory.CharToBytes(ptr.Slice(0, uint64(ϟa.Length), ϟs.MemoryLayout).Read(ϟctx, ϟa, ϟs, nil)))
}
return strings.TrimRight(string(memory.CharToBytes(ptr.StringSlice(ϟctx, ϟs).Read(ϟctx, ϟa, ϟs, nil))), "\x00")
return readString(ϟctx, ϟa, ϟs, ϟa.Message, ϟa.Length)
}

// Label returns the user maker name.
func (ϟa *GlPushDebugGroupKHR) Label(ϟctx context.Context, ϟs *api.GlobalState) string {
ptr := Charᵖ(ϟa.Message)
// This is incorrect, fudging for a bug in Unity which has been fixed but not
// rolled out.
// See https://github.com/google/gapid/issues/459 for reference.
//
// ϟa.Length should only be treated as null-terminated if ϟa.Length is < 0.
//
// TODO: Consider removing once the fixed version is mainstream.
// if ϟa.Length >= 0 {
if ϟa.Length > 0 {
return string(memory.CharToBytes(ptr.Slice(0, uint64(ϟa.Length), ϟs.MemoryLayout).Read(ϟctx, ϟa, ϟs, nil)))
}
return strings.TrimRight(string(memory.CharToBytes(ptr.StringSlice(ϟctx, ϟs).Read(ϟctx, ϟa, ϟs, nil))), "\x00")
return readString(ϟctx, ϟa, ϟs, ϟa.Message, ϟa.Length)
}
5 changes: 4 additions & 1 deletion gapis/api/gvr/framebindings.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ func (r *FrameBindingsResolvable) Resolve(ctx context.Context) (interface{}, err
// just so it can be nullified before returning. To avoid another
// state mutation just to get the pointer, cache them here.
cmd.extras.Observations().ApplyReads(s.Memory.ApplicationPool())
frame := cmd.Frame.Read(ctx, cmd, s, nil)
frame, err := cmd.Frame.Read(ctx, cmd, s, nil)
if err != nil {
return err
}
out.submitBuffer[id] = frameToBuffer[frame]
case *Gvr_frame_get_framebuffer_object:
frameToBuffer[GvrFrameᵖ(cmd.Frame)] = gles.FramebufferId(cmd.Result)
Expand Down
59 changes: 47 additions & 12 deletions gapis/api/templates/api.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@
// New{{$ptr_ty}} returns a {{$ptr_ty}} that points to addr in the application pool.
func New{{$ptr_ty}}(p ϟmem.Pointer) {{$ptr_ty}} {
if p == nil {
return {{$ptr_ty}}{ 0, ϟmem.ApplicationPool }
return {{$ptr_ty}}{ 0, ϟmem.ApplicationPool }
}
return {{$ptr_ty}}{ p.Address(), p.Pool() }
}
Expand Down Expand Up @@ -251,13 +251,30 @@

{{if not $el_is_void}}
// Read reads and returns the {{$el_ty}} element at the pointer.
func (p {{$ptr_ty}}) Read(ϟctx context.Context, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) {{$el_ty}} {
return p.Slice(0, 1, ϟs.MemoryLayout).Read(ϟctx, ϟa, ϟs, ϟb)[0]
func (p {{$ptr_ty}}) Read(ϟctx context.Context, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) ({{$el_ty}}, error) {
vals, err := p.Slice(0, 1, ϟs.MemoryLayout).Read(ϟctx, ϟa, ϟs, ϟb)
if err != nil {
return {{Template "Go.Null" (TypeOf $p.To)}}, err
}
return vals[0], nil
}

// MustRead calls and returns Read, panicing if there was an error.
func (p {{$ptr_ty}}) MustRead(ϟctx context.Context, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) {{$el_ty}} {
val, err := p.Read(ϟctx, ϟa, ϟs, ϟb)
panicOnError(err)
return val
}

// Write writes value to the {{$el_ty}} element at the pointer.
func (p {{$ptr_ty}}) Write(ϟctx context.Context, value {{$el_ty}}, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) {
p.Slice(0, 1, ϟs.MemoryLayout).Write(ϟctx, []{{$el_ty}}{value}, ϟa, ϟs, ϟb)
func (p {{$ptr_ty}}) Write(ϟctx context.Context, value {{$el_ty}}, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) error {
_, err := p.Slice(0, 1, ϟs.MemoryLayout).Write(ϟctx, []{{$el_ty}}{value}, ϟa, ϟs, ϟb)
return err
}

// MustWrite calls Write, panicing if there was an error.
func (p {{$ptr_ty}}) MustWrite(ϟctx context.Context, value {{$el_ty}}, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) {
panicOnError(p.Write(ϟctx, value, ϟa, ϟs, ϟb))
}
{{end}}

Expand Down Expand Up @@ -499,23 +516,41 @@
}

// Read reads and returns all the {{$el_ty}} elements in this {{$slice_ty}}.
func (s {{$slice_ty}}) Read(ϟctx context.Context, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) []{{$el_ty}} {
func (s {{$slice_ty}}) Read(ϟctx context.Context, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) ([]{{$el_ty}}, error) {
s.OnRead(ϟctx, ϟa, ϟs, ϟb)
res := make([]{{$el_ty}}, s.count)
d := s.Decoder(ϟctx, ϟs)
ϟmem.Read(d, &res)
panicOnError(d.Error())
return res
if err := d.Error(); err != nil {
return nil, err
}
return res, nil
}

// MustRead calls and returns Read, panicing if there was an error.
func (s {{$slice_ty}}) MustRead(ϟctx context.Context, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) []{{$el_ty}} {
vals, err := s.Read(ϟctx, ϟa, ϟs, ϟb)
panicOnError(err)
return vals
}

// Write copies elements from src to this slice. The number of elements copied is returned
// which is the minimum of s.count and len(src).
func (s {{$slice_ty}}) Write(ϟctx context.Context, src []{{$el_ty}}, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) uint64 {
func (s {{$slice_ty}}) Write(ϟctx context.Context, src []{{$el_ty}}, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) (uint64, error) {
count := u64.Min(s.count, uint64(len(src)))
e := s.Slice(0, count, ϟs.MemoryLayout).Encoder(ϟs)
ϟmem.Write(e, src[:count])
s.OnWrite(ϟctx, ϟa, ϟs, ϟb)
panicOnError(e.Error())
if err := e.Error(); err != nil {
return 0, err
}
return count, nil
}

// MustWrite calls and returns Write, panicing if there was an error.
func (s {{$slice_ty}}) MustWrite(ϟctx context.Context, src []{{$el_ty}}, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) uint64 {
count, err := s.Write(ϟctx, src, ϟa, ϟs, ϟb)
panicOnError(err)
return count
}

Expand All @@ -527,7 +562,7 @@
dst, src = dst.Slice(0, count, ϟs.MemoryLayout), src.Slice(0, count, ϟs.MemoryLayout)
{{if $el_is_ptr}}
if (dst.pool == ϟmem.ApplicationPool) != (src.pool == ϟmem.ApplicationPool) {
dst.Write(ϟctx, src.Read(ϟctx, ϟa, ϟs, ϟb), ϟa, ϟs, ϟb) // Element-wise copy so we can convert u64 <-> {{$ptr_ty}}
dst.MustWrite(ϟctx, src.MustRead(ϟctx, ϟa, ϟs, ϟb), ϟa, ϟs, ϟb) // Element-wise copy so we can convert u64 <-> {{$ptr_ty}}
} else {
{{end}}
src.OnRead(ϟctx, ϟa, ϟs, ϟb)
Expand All @@ -539,7 +574,7 @@

// Contains returns true if the slice contains the specified value.
func (s {{$slice_ty}}) Contains(ϟctx context.Context, val {{$el_ty}}, ϟa api.Cmd, ϟs *api.GlobalState, ϟb *builder.Builder) bool {
for _, e := range s.Read(ϟctx, ϟa, ϟs, ϟb) {
for _, e := range s.MustRead(ϟctx, ϟa, ϟs, ϟb) {
if e == val {
return true
}
Expand Down
4 changes: 2 additions & 2 deletions gapis/api/templates/go_common.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -534,9 +534,9 @@
observed memory which is undesirable, so instead just perform regular
(non-replay-builder) read logic.
*/}}
Read(ϟctx, ϟa, ϟs, nil)
MustRead(ϟctx, ϟa, ϟs, nil)
{{else}}
Read(ϟctx, ϟa, ϟs, ϟb)
MustRead(ϟctx, ϟa, ϟs, ϟb)
{{end}}
{{end}}

Expand Down
6 changes: 3 additions & 3 deletions gapis/api/templates/mutate.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@
{{AssertType $ "SliceAssign"}}

{{if ne $.Operator "="}}{{Error "Compound assignments to pointers are not supported (%s)" $.Operator}}{{end}}
{{Template "Go.Read" $.To.Slice}}.Index({{Template "Go.Read" $.To.Index}}, ϟl).Write(ϟctx, {{Template "Go.Read" $.Value}}, ϟa, ϟs, ϟb)
{{Template "Go.Read" $.To.Slice}}.Index({{Template "Go.Read" $.To.Index}}, ϟl).MustWrite(ϟctx, {{Template "Go.Read" $.Value}}, ϟa, ϟs, ϟb)
{{end}}


Expand Down Expand Up @@ -617,15 +617,15 @@
ϟdst, ϟsrc := {{Template "Go.Read" $.Statement.Dst}}, {{Template "Go.Read" $.Statement.Src}}
ϟcount := u64.Min(ϟdst.Count(), ϟsrc.Count())
ϟdst, ϟsrc = ϟdst.Slice(0, ϟcount, ϟl), ϟsrc.Slice(0, ϟcount, ϟl)
ϟsrcElems := ϟsrc.Read(ϟctx, ϟa, ϟs, ϟb)
ϟsrcElems := ϟsrc.MustRead(ϟctx, ϟa, ϟs, ϟb)
{{end}}

{{/* Perform the call */}}
ϟa{{if $f.Subroutine}}.(callable){{end}}.Call(ϟctx, ϟs, ϟb);

{{if IsCopy $.Statement}}
{{/* Apply the fenced-copy write */}}
ϟdst.Write(ϟctx, ϟsrcElems, ϟa, ϟs, ϟb)
ϟdst.MustWrite(ϟctx, ϟsrcElems, ϟa, ϟs, ϟb)
{{end}}
}
{{if IsCopy $.Statement}}
Expand Down
12 changes: 8 additions & 4 deletions gapis/api/test/intrinsics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ func TestClone(t *testing.T) {
cb.CmdClone(p(0x1234), 10).
AddRead(memory.Store(ctx, s.MemoryLayout, p(0x1234), expected)),
)
got := GetState(s).U8s.Read(ctx, nil, s, nil)
assert.With(ctx).ThatSlice(got).Equals(expected)
got, err := GetState(s).U8s.Read(ctx, nil, s, nil)
if assert.For(ctx, "err").ThatError(err).Succeeded() {
assert.For(ctx, "got").ThatSlice(got).Equals(expected)
}
}

func TestMake(t *testing.T) {
Expand Down Expand Up @@ -70,8 +72,10 @@ func TestCopy(t *testing.T) {
cb.CmdCopy(p(0x1234), 10).
AddRead(memory.Store(ctx, s.MemoryLayout, p(0x1234), expected)),
)
got := GetState(s).U8s.Read(ctx, nil, s, nil)
assert.With(ctx).ThatSlice(got).Equals(expected)
got, err := GetState(s).U8s.Read(ctx, nil, s, nil)
if assert.For(ctx, "err").ThatError(err).Succeeded() {
assert.For(ctx, "got").ThatSlice(got).Equals(expected)
}
}

func TestCharsliceToString(t *testing.T) {
Expand Down
6 changes: 4 additions & 2 deletions gapis/api/test/subroutines_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ func TestSubAdd(t *testing.T) {
cb := CommandBuilder{Thread: 0}
s := api.NewStateWithEmptyAllocator(device.Little32)
api.MutateCmds(ctx, s, nil, cb.CmdAdd(10, 20))
got := GetState(s).Ints.Read(ctx, nil, s, nil)
got, err := GetState(s).Ints.Read(ctx, nil, s, nil)
expected := []memory.Int{30}
assert.With(ctx).ThatSlice(got).Equals(expected)
if assert.For(ctx, "err").ThatError(err).Succeeded() {
assert.For(ctx, "got").ThatSlice(got).Equals(expected)
}
}
Loading

0 comments on commit 0a79f3f

Please sign in to comment.