From 8033af0947f0397b1741da4b443b4c53dc686080 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Mon, 4 Apr 2022 14:46:12 +0000 Subject: [PATCH] dawn.node: Implement the [Clamp] and [EnforceRange] WebIDL attributes. These are implemented by wrapping the integer types in transparent ClampedInteger<> and EnforceRangeInteger<> structures. Some parts of the core needed to be updated after this, either to disambiguate conversions, or because of bugs (u32 vs u64). To make the CTS tests checking for this pass, the errors returned when conversion FromJS failed needed to be updated to TypeError and not just the generic Napi::Error. Bug: dawn:1123 Change-Id: Ife1d0baa7687e43d735a1814ec41883c49ae74a6 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/85640 Reviewed-by: Ben Clayton Commit-Queue: Corentin Wallez --- src/dawn/node/binding/Converter.cpp | 10 +- src/dawn/node/binding/Converter.h | 14 +++ src/dawn/node/binding/GPUBuffer.cpp | 4 +- .../node/binding/GPURenderBundleEncoder.cpp | 4 +- .../node/binding/GPURenderPassEncoder.cpp | 4 +- src/dawn/node/interop/Core.h | 103 ++++++++++++++++++ src/dawn/node/interop/WebGPU.cpp.tmpl | 6 +- src/dawn/node/interop/WebGPU.h.tmpl | 4 + src/dawn/node/interop/WebGPUCommon.tmpl | 24 +++- src/dawn/node/tools/src/cmd/idlgen/main.go | 8 +- 10 files changed, 162 insertions(+), 19 deletions(-) diff --git a/src/dawn/node/binding/Converter.cpp b/src/dawn/node/binding/Converter.cpp index b2bd832234..a70e85ef07 100644 --- a/src/dawn/node/binding/Converter.cpp +++ b/src/dawn/node/binding/Converter.cpp @@ -464,27 +464,27 @@ namespace wgpu::binding { } bool Converter::Convert(wgpu::TextureUsage& out, const interop::GPUTextureUsageFlags& in) { - out = static_cast(in); + out = static_cast(in.value); return true; } bool Converter::Convert(wgpu::ColorWriteMask& out, const interop::GPUColorWriteFlags& in) { - out = static_cast(in); + out = static_cast(in.value); return true; } bool Converter::Convert(wgpu::BufferUsage& out, const interop::GPUBufferUsageFlags& in) { - out = static_cast(in); + out = static_cast(in.value); return true; } bool Converter::Convert(wgpu::MapMode& out, const interop::GPUMapModeFlags& in) { - out = static_cast(in); + out = static_cast(in.value); return true; } bool Converter::Convert(wgpu::ShaderStage& out, const interop::GPUShaderStageFlags& in) { - out = static_cast(in); + out = static_cast(in.value); return true; } diff --git a/src/dawn/node/binding/Converter.h b/src/dawn/node/binding/Converter.h index f0fa75314d..17754cbdeb 100644 --- a/src/dawn/node/binding/Converter.h +++ b/src/dawn/node/binding/Converter.h @@ -281,6 +281,20 @@ namespace wgpu::binding { return true; } + // ClampedInteger + template + inline bool Convert(T& out, const interop::ClampedInteger& in) { + out = in; + return true; + } + + // EnforceRangeInteger + template + inline bool Convert(T& out, const interop::EnforceRangeInteger& in) { + out = in; + return true; + } + template inline bool Convert(OUT& out, const std::variant& in) { return std::visit([&](auto&& i) { return Convert(out, i); }, in); diff --git a/src/dawn/node/binding/GPUBuffer.cpp b/src/dawn/node/binding/GPUBuffer.cpp index dbff2c5f0a..ebafaa5eee 100644 --- a/src/dawn/node/binding/GPUBuffer.cpp +++ b/src/dawn/node/binding/GPUBuffer.cpp @@ -69,7 +69,7 @@ namespace wgpu::binding { auto ctx = new Context{env, interop::Promise(env, PROMISE_INFO), async_, state_}; auto promise = ctx->promise; - uint64_t s = size.has_value() ? size.value() : (desc_.size - offset); + uint64_t s = size.has_value() ? size.value().value : (desc_.size - offset); state_ = State::MappingPending; @@ -114,7 +114,7 @@ namespace wgpu::binding { return {}; } - uint64_t s = size.has_value() ? size.value() : (desc_.size - offset); + uint64_t s = size.has_value() ? size.value().value : (desc_.size - offset); uint64_t start = offset; uint64_t end = offset + s; diff --git a/src/dawn/node/binding/GPURenderBundleEncoder.cpp b/src/dawn/node/binding/GPURenderBundleEncoder.cpp index 9bfededee0..87ecd34745 100644 --- a/src/dawn/node/binding/GPURenderBundleEncoder.cpp +++ b/src/dawn/node/binding/GPURenderBundleEncoder.cpp @@ -156,7 +156,7 @@ namespace wgpu::binding { Converter conv(env); wgpu::Buffer b{}; - uint32_t o = 0; + uint64_t o = 0; if (!conv(b, indirectBuffer) || // !conv(o, indirectOffset)) { @@ -172,7 +172,7 @@ namespace wgpu::binding { Converter conv(env); wgpu::Buffer b{}; - uint32_t o = 0; + uint64_t o = 0; if (!conv(b, indirectBuffer) || // !conv(o, indirectOffset)) { diff --git a/src/dawn/node/binding/GPURenderPassEncoder.cpp b/src/dawn/node/binding/GPURenderPassEncoder.cpp index 694da62bfb..0297d65d66 100644 --- a/src/dawn/node/binding/GPURenderPassEncoder.cpp +++ b/src/dawn/node/binding/GPURenderPassEncoder.cpp @@ -218,7 +218,7 @@ namespace wgpu::binding { Converter conv(env); wgpu::Buffer b{}; - uint32_t o = 0; + uint64_t o = 0; if (!conv(b, indirectBuffer) || // !conv(o, indirectOffset)) { @@ -234,7 +234,7 @@ namespace wgpu::binding { Converter conv(env); wgpu::Buffer b{}; - uint32_t o = 0; + uint64_t o = 0; if (!conv(b, indirectBuffer) || // !conv(o, indirectOffset)) { diff --git a/src/dawn/node/interop/Core.h b/src/dawn/node/interop/Core.h index 1c98036e0b..49ebb40263 100644 --- a/src/dawn/node/interop/Core.h +++ b/src/dawn/node/interop/Core.h @@ -69,6 +69,40 @@ namespace wgpu::interop { template using FrozenArray = std::vector; + // A wrapper class for integers that's as transparent as possible and is used to distinguish + // that the type is tagged with the [Clamp] WebIDL attribute. + template + struct ClampedInteger { + static_assert(std::is_integral_v); + + using IntegerType = T; + ClampedInteger() : value(0) { + } + ClampedInteger(T value) : value(value) { + } + operator T() const { + return value; + } + T value; + }; + + // A wrapper class for integers that's as transparent as possible and is used to distinguish + // that the type is tagged with the [EnforceRange] WebIDL attribute. + template + struct EnforceRangeInteger { + static_assert(std::is_integral_v); + + using IntegerType = T; + EnforceRangeInteger() : value(0) { + } + EnforceRangeInteger(T value) : value(value) { + } + operator T() const { + return value; + } + T value; + }; + //////////////////////////////////////////////////////////////////////////////// // Result //////////////////////////////////////////////////////////////////////////////// @@ -447,6 +481,75 @@ namespace wgpu::interop { static Napi::Value ToJS(Napi::Env, double); }; + // [Clamp]ed integers must convert values outside of the integer range by clamping them. + template + class Converter> { + public: + static Result FromJS(Napi::Env env, Napi::Value value, ClampedInteger& out) { + double doubleValue; + Result res = Converter::FromJS(env, value, doubleValue); + if (!res) { + return res; + } + + // Check for clamping first. + constexpr T kMin = std::numeric_limits::min(); + constexpr T kMax = std::numeric_limits::max(); + if (doubleValue < kMin) { + out = kMin; + return Success; + } + if (doubleValue > kMax) { + out = kMax; + return Success; + } + + // Yay, no clamping! We can convert the integer type as usual. + T correctValue; + res = Converter::FromJS(env, value, correctValue); + if (!res) { + return res; + } + out = correctValue; + return Success; + } + static Napi::Value ToJS(Napi::Env env, const ClampedInteger& value) { + return Converter::ToJS(env, value.value); + } + }; + + // [EnforceRange] integers cause a TypeError when converted from out of range values + template + class Converter> { + public: + static Result FromJS(Napi::Env env, Napi::Value value, EnforceRangeInteger& out) { + double doubleValue; + Result res = Converter::FromJS(env, value, doubleValue); + if (!res) { + return res; + } + + // Check for out of range and throw a type error. + constexpr T kMin = std::numeric_limits::min(); + constexpr T kMax = std::numeric_limits::max(); + if (!(kMin <= doubleValue && doubleValue <= kMax)) { + return Error("Values are out of the range of that integer."); + } + + // Yay, no error! We can convert the integer type as usual. + T correctValue; + res = Converter::FromJS(env, value, correctValue); + if (!res) { + return res; + } + out = correctValue; + return Success; + } + static Napi::Value ToJS(Napi::Env env, const EnforceRangeInteger& value) { + return Converter::ToJS(env, value.value); + } + }; + template <> class Converter { public: diff --git a/src/dawn/node/interop/WebGPU.cpp.tmpl b/src/dawn/node/interop/WebGPU.cpp.tmpl index ba2a81789f..f711b1ecfa 100644 --- a/src/dawn/node/interop/WebGPU.cpp.tmpl +++ b/src/dawn/node/interop/WebGPU.cpp.tmpl @@ -172,7 +172,7 @@ Wrappers* Wrappers::instance = nullptr; if (res) { return ToJS(info.Env(), impl->has(info.Env(), std::get<0>(args))); } - Napi::Error::New(info.Env(), res.error).ThrowAsJavaScriptException(); + Napi::TypeError::New(info.Env(), res.error).ThrowAsJavaScriptException(); return {}; } Napi::Value keys(const Napi::CallbackInfo& info) { @@ -217,7 +217,7 @@ Wrappers* Wrappers::instance = nullptr; error = {{if $overloaded}}"\noverload {{$overload_idx}} failed to match:\n" + {{end}}res.error; } {{- end}} - Napi::Error::New(info.Env(), "no overload matched for {{$m.Name}}:\n" + error).ThrowAsJavaScriptException(); + Napi::TypeError::New(info.Env(), "no overload matched for {{$m.Name}}:\n" + error).ThrowAsJavaScriptException(); return {}; } {{- end}} @@ -235,7 +235,7 @@ Wrappers* Wrappers::instance = nullptr; impl->set{{Title $a.Name}}(info.Env(), std::move(v)); } else { res = res.Append("invalid value to {{$a.Name}}"); - Napi::Error::New(info.Env(), res.error).ThrowAsJavaScriptException(); + Napi::TypeError::New(info.Env(), res.error).ThrowAsJavaScriptException(); } } {{- end}} diff --git a/src/dawn/node/interop/WebGPU.h.tmpl b/src/dawn/node/interop/WebGPU.h.tmpl index 1a1a97040e..628c569d76 100644 --- a/src/dawn/node/interop/WebGPU.h.tmpl +++ b/src/dawn/node/interop/WebGPU.h.tmpl @@ -146,7 +146,11 @@ public: -------------------------------------------------------------------------------- */ -}} {{- define "Typedef"}} +{{- if HasAnnotation $ "EnforceRange"}} +using {{$.Name}} = EnforceRangeInteger<{{template "Type" $.Type}}>; +{{- else}} using {{$.Name}} = {{template "Type" $.Type}}; +{{- end}} {{end}} diff --git a/src/dawn/node/interop/WebGPUCommon.tmpl b/src/dawn/node/interop/WebGPUCommon.tmpl index 1311c906be..bf11709e0a 100644 --- a/src/dawn/node/interop/WebGPUCommon.tmpl +++ b/src/dawn/node/interop/WebGPUCommon.tmpl @@ -39,6 +39,8 @@ See: {{- if IsUndefinedType $}}void {{- else if IsTypeName $}} {{- if eq $.Name "boolean" }}bool +{{- else if eq $.Name "short" }}int16_t +{{- else if eq $.Name "unsigned short" }}uint16_t {{- else if eq $.Name "long" }}int32_t {{- else if eq $.Name "unsigned long" }}uint32_t {{- else if eq $.Name "long long" }}int64_t @@ -66,13 +68,29 @@ See: -------------------------------------------------------------------------------- */ -}} {{- define "AttributeType" -}} -{{- if $.Required }}{{template "Type" $.Type}} -{{- else if $.Init }}{{template "Type" $.Type}} -{{- else }}std::optional<{{template "Type" $.Type}}> +{{- if $.Required }}{{template "AttributeClampHelper" $}} +{{- else if $.Init }}{{template "AttributeClampHelper" $}} +{{- else }}std::optional<{{template "AttributeClampHelper" $}}> {{- end}} {{- end }} +{{- /* + A helper for AttributeType that wraps integer types if necessary for WebIDL attributes. + Note that [Clamp] and [EnforceRange] are supposed to be an annotation on the type and not + the attribute, but webidlparser doesn't parse this correctly. +*/ -}} +{{- define "AttributeClampHelper" -}} +{{- if HasAnnotation $ "Clamp" }} +ClampedInteger<{{template "Type" $.Type}}> +{{- else if HasAnnotation $ "EnforceRange" }} +EnforceRangeInteger<{{template "Type" $.Type}}> +{{- else}} +{{template "Type" $.Type}} +{{- end }} +{{- end }} + + {{- /* -------------------------------------------------------------------------------- -- Literal generates a C++ literal value using the following arguments: diff --git a/src/dawn/node/tools/src/cmd/idlgen/main.go b/src/dawn/node/tools/src/cmd/idlgen/main.go index 5661f00166..9985203dff 100644 --- a/src/dawn/node/tools/src/cmd/idlgen/main.go +++ b/src/dawn/node/tools/src/cmd/idlgen/main.go @@ -519,12 +519,16 @@ func findAnnotation(list []*ast.Annotation, name string) *ast.Annotation { func hasAnnotation(obj interface{}, name string) bool { switch obj := obj.(type) { - case *ast.Member: - return findAnnotation(obj.Annotations, name) != nil case *ast.Interface: return findAnnotation(obj.Annotations, name) != nil + case *ast.Member: + return findAnnotation(obj.Annotations, name) != nil case *ast.Namespace: return findAnnotation(obj.Annotations, name) != nil + case *ast.Parameter: + return findAnnotation(obj.Annotations, name) != nil + case *ast.Typedef: + return findAnnotation(obj.Annotations, name) != nil || findAnnotation(obj.TypeAnnotations, name) != nil } panic("Unhandled AST node type in hasAnnotation") }