Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uint8 array support #143

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions array_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package v8go_test

import (
"errors"
"fmt"
"testing"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"testing"
"testing"
v8 "rogchap.com/v8go"


v8 "rogchap.com/v8go"
)

func reverseUint8ArrayFunctionCallback(info *v8.FunctionCallbackInfo) *v8.Value {
iso := info.Context().Isolate()
args := info.Args()

if len(args) != 1 {
e, _ := v8.NewValue(iso, "Function ReverseUint8Array expects 1 parameter")
return iso.ThrowException(e)
}
if !args[0].IsUint8Array() {
e, _ := v8.NewValue(iso, "Function ReverseUint8Array expects Uint8Array parameter")
return iso.ThrowException(e)
}
array := args[0].Uint8Array()
length := len(array)
reversed := make([]uint8, length)
for i := 0; i < length; i++ {
reversed[i] = array[length-i-1]
}
val, err := v8.NewValue(iso, reversed)
if err != nil {
e, _ := v8.NewValue(iso, fmt.Sprintf("Could not get value for array: %v\n", err))
return iso.ThrowException(e)
}
return val
}

func injectUint8ArrayTester(ctx *v8.Context) error {
if ctx == nil {
return errors.New("injectUint8ArrayTester: ctx is required")
}

iso := ctx.Isolate()

con := v8.NewObjectTemplate(iso)

reverseFn := v8.NewFunctionTemplate(iso, reverseUint8ArrayFunctionCallback)

if err := con.Set("reverseUint8Array", reverseFn, v8.ReadOnly); err != nil {
return fmt.Errorf("injectUint8ArrayTester: %v", err)
}

nativeObj, err := con.NewInstance(ctx)
if err != nil {
return fmt.Errorf("injectUint8ArrayTester: %v", err)
}

global := ctx.Global()

if err := global.Set("native", nativeObj); err != nil {
return fmt.Errorf("injectUint8ArrayTester: %v", err)
}

return nil
}

// Test that a script can call a go function to reverse a []uint8 array
func TestUint8Array(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it is testing both Value.IsUint8Array and NewValue with a byte array. These could be tested separately with much simpler tests

t.Parallel()

iso := v8.NewIsolate()
ctx := v8.NewContext(iso)

if err := injectUint8ArrayTester(ctx); err != nil {
t.Error(err)
}

if val, err := ctx.RunScript("native.reverseUint8Array(new Uint8Array([0,1,2,3,4,5,6,7,8,9]))", ""); err != nil {
t.Error(err)
} else {
Comment on lines +77 to +79
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use fatal errors to avoid needing to deeply nest the non-error paths. There is a test helper method for this

Suggested change
if val, err := ctx.RunScript("native.reverseUint8Array(new Uint8Array([0,1,2,3,4,5,6,7,8,9]))", ""); err != nil {
t.Error(err)
} else {
val, err := ctx.RunScript("native.reverseUint8Array(new Uint8Array([0,1,2,3,4,5,6,7,8,9]))", "")
failIf(t, err)

if !val.IsUint8Array() {
t.Errorf("Expected uint8 array return value")
}
arr := val.Uint8Array()
if len(arr) != 10 {
t.Errorf("Got wrong array length %d, expected 10", len(arr))
}
for i := 0; i < 10; i++ {
if arr[i] != uint8(10-i-1) {
Comment on lines +84 to +88
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be simplified using bytes.Equal?

t.Errorf("Incorrect byte at index %d (whole array: %v)", i, arr)
}
}
}
}

// Test that a native go function can throw exceptions that make it back to the script runner
func TestUint8ArrayException(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be testing IsUint8Array, but in a very indirect way. Why not just test it more directly.

t.Parallel()

iso := v8.NewIsolate()
ctx := v8.NewContext(iso)

if err := injectUint8ArrayTester(ctx); err != nil {
t.Error(err)
}

if _, err := ctx.RunScript("native.reverseUint8Array(\"notanarray\")", ""); err != nil {
t.Logf("Got expected error from script: %v", err)
} else {
t.Errorf("Should have received an error from the script")
}
}
30 changes: 30 additions & 0 deletions arraybuffer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package v8go

// #include <stdlib.h>
// #include "v8go.h"
import "C"
import "unsafe"

type ArrayBuffer struct {
*Value
}

func NewArrayBuffer(ctx *Context, len int64) *ArrayBuffer {
return &ArrayBuffer{&Value{C.NewArrayBuffer(ctx.iso.ptr, C.size_t(len)), ctx}}
}

func (ab *ArrayBuffer) ByteLength() int64 {
return int64(C.ArrayBufferByteLength(ab.ptr))
}

func (ab *ArrayBuffer) GetBytes() []uint8 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefixing methods with Get isn't idiomatic for Go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dylanahsmith can you expand on this? I want to make sure I do this correctly in the future. Do you mean that it should just be Bytes()?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that it should just be Bytes()?

Yes

len := C.ArrayBufferByteLength(ab.ptr)
cbytes := unsafe.Pointer(C.GetArrayBufferBytes(ab.ptr)) // points into BackingStore
return C.GoBytes(cbytes, C.int(len))
Comment on lines +21 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't safely just expose the C++ data. However, we could expose more primitive operations. For instance, if we exposed a method to append the data to a Go slice, then we could implement a method that does this in pure Go using that on an empty slice.

Another possible primitive would be something like v8::ArrayBufferView::CopyContents which could be used to copy as much as it could into a slice without growing it for the fixed size buffer case. It might also be useful to be able to get something like a V8::ArrayBufferView without having to allocate the V8 object for it, in order to specify the part of the ArrayBuffer to copy/append into or out of a Go slice.

I suppose more primitive operations could always be added later though.

}

func (ab *ArrayBuffer) PutBytes(bytes []uint8) {
cbytes := C.CBytes(bytes) //FIXME is there really no way to avoid this malloc+memcpy?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to pass in unsafe.Pointer(&bytes[0]) and len(bytes) to the C function to avoid the extra allocation and copy.

defer C.free(cbytes)
C.PutArrayBufferBytes(ab.ptr, 0, (*C.char)(cbytes), C.size_t(len(bytes)))
}
152 changes: 152 additions & 0 deletions arraybuffer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
package v8go_test

import (
"errors"
"fmt"
"testing"

v8 "rogchap.com/v8go"
)

func reverseArrayBufferFunctionCallback(info *v8.FunctionCallbackInfo) *v8.Value {
iso := info.Context().Isolate()
args := info.Args()
if len(args) != 1 {
e, _ := v8.NewValue(iso, "Function ReverseArrayBuffer expects 1 parameter")
return iso.ThrowException(e)
}
if !args[0].IsArrayBuffer() {
e, _ := v8.NewValue(iso, "Function ReverseArrayBuffer expects ArrayBuffer parameter")
return iso.ThrowException(e)
}
ab := args[0].ArrayBuffer() // "cast" to ArrayBuffer
length := int(ab.ByteLength())
bytes := ab.GetBytes() // get a copy of the bytes from the ArrayBuffer
reversed := make([]uint8, length)
for i := 0; i < length; i++ {
reversed[i] = bytes[length-i-1]
}
ab.PutBytes(reversed) // update the bytes in the ArrayBuffer (length must match!)
return nil
}

func createArrayBufferFunctionCallback(info *v8.FunctionCallbackInfo) *v8.Value {
iso := info.Context().Isolate()
args := info.Args()
if len(args) != 1 {
e, _ := v8.NewValue(iso, "Function CreateArrayBuffer expects 1 parameter")
return iso.ThrowException(e)
}
if !args[0].IsInt32() {
e, _ := v8.NewValue(iso, "Function CreateArrayBuffer expects Int32 parameter")
return iso.ThrowException(e)
}
length := args[0].Int32()
ab := v8.NewArrayBuffer(info.Context(), int64(length)) // create ArrayBuffer object of given length
bytes := make([]uint8, length)
for i := uint8(0); i < uint8(length); i++ {
bytes[i] = i
}
ab.PutBytes(bytes) // copy these bytes into it. Caller is responsible for avoiding overruns!
return ab.Value // return the ArrayBuffer to javascript
}

func injectArrayBufferTester(ctx *v8.Context, funcName string, funcCb v8.FunctionCallback) error {
if ctx == nil {
return errors.New("injectArrayBufferTester: ctx is required")
}

iso := ctx.Isolate()

con := v8.NewObjectTemplate(iso)
funcTempl := v8.NewFunctionTemplate(iso, funcCb)

if err := con.Set(funcName, funcTempl, v8.ReadOnly); err != nil {
return fmt.Errorf("injectArrayBufferTester: %v", err)
}

nativeObj, err := con.NewInstance(ctx)
if err != nil {
return fmt.Errorf("injectArrayBufferTester: %v", err)
}

global := ctx.Global()

if err := global.Set("native", nativeObj); err != nil {
return fmt.Errorf("injectArrayBufferTester: %v", err)
}

return nil
}

// Test that a script can call a go function to reverse an ArrayBuffer.
// The function reverses the ArrayBuffer in-place, i.e. this is a call-by-reference.
func TestModifyArrayBuffer(t *testing.T) {
t.Parallel()

iso := v8.NewIsolate()
ctx := v8.NewContext(iso)
if err := injectArrayBufferTester(ctx, "reverseArrayBuffer", reverseArrayBufferFunctionCallback); err != nil {
t.Error(err)
}

js := `
let ab = new ArrayBuffer(10);
let view = new Uint8Array(ab);
for (let i = 0; i < 10; i++) view[i] = i;
native.reverseArrayBuffer(ab);
ab;
`

if val, err := ctx.RunScript(js, ""); err != nil {
t.Error(err)
} else {
if !val.IsArrayBuffer() {
t.Errorf("Expected ArrayBuffer return value")
}
ab := val.ArrayBuffer()
if ab.ByteLength() != 10 {
t.Errorf("Got wrong ArrayBuffer length %d, expected 10", ab.ByteLength())
}
bytes := ab.GetBytes()
fmt.Printf("Got reversed ArrayBuffer from script: %v\n", bytes)
for i := int64(0); i < ab.ByteLength(); i++ {
if bytes[i] != uint8(10-i-1) {
t.Errorf("Incorrect byte at index %d (whole array: %v)", i, ab)
}
}
}
}

func TestCreateArrayBuffer(t *testing.T) {
t.Parallel()

iso := v8.NewIsolate()
ctx := v8.NewContext(iso)
if err := injectArrayBufferTester(ctx, "createArrayBuffer", createArrayBufferFunctionCallback); err != nil {
t.Error(err)
}

js := `
native.createArrayBuffer(16);
`

if val, err := ctx.RunScript(js, ""); err != nil {
t.Error(err)
} else {
if !val.IsArrayBuffer() {
t.Errorf("Expected ArrayBuffer return value")
}
ab := val.ArrayBuffer()
if ab.ByteLength() != 16 {
t.Errorf("Got wrong ArrayBuffer length %d, expected 16", ab.ByteLength())
}
bytes := ab.GetBytes()
fmt.Printf("Got ArrayBuffer from script: %v\n", bytes)
for i := int64(0); i < ab.ByteLength(); i++ {
if bytes[i] != uint8(i) {
t.Errorf("Incorrect byte at index %d (whole array: %v)", i, bytes)
}
}
}
}
7 changes: 5 additions & 2 deletions deps/include/OWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@ [email protected]

per-file *DEPS=file:../COMMON_OWNERS
per-file v8-internal.h=file:../COMMON_OWNERS
per-file v8-inspector.h=file:../src/inspector/OWNERS
per-file v8-inspector-protocol.h=file:../src/inspector/OWNERS

per-file v8-debug.h=file:../src/debug/OWNERS

per-file js_protocol.pdl=file:../src/inspector/OWNERS
per-file v8-inspector*=file:../src/inspector/OWNERS
per-file v8-inspector*=file:../src/inspector/OWNERS

# Needed by the auto_tag builder
per-file v8-version.h=v8-ci-autoroll-builder@chops-service-accounts.iam.gserviceaccount.com
Expand Down
Loading