Nuke Builders part 4: headers, proctable and miscreferences

BUG=dawn:125

Change-Id: If1b2feb1b0880eb8ad168cfabb9d67ece4f02190
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/6220
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

dawn.json

@@ -136,18 +136,6 @@
             {"value": 2, "name": "opaque white"}
         ]
     },
-    "builder error status": {
-        "category": "enum",
-        "values": [
-            {"value": 0, "name": "success"},
-            {"value": 1, "name": "error", "TODO": "cwallez@chromium.org: recoverable errors like GPU OOM"},
-            {"value": 2, "name": "unknown"},
-            {"value": 3, "name": "context lost"}
-        ]
-    },
-    "builder error callback": {
-        "category": "natively defined"
-    },
     "buffer": {
         "category": "object",
         "methods": [

generator/common.py

@@ -49,7 +49,6 @@ class Type:
         self.dict_name = name
         self.name = Name(name, native=native)
         self.category = json_data['category']
-        self.is_builder = self.name.canonical_case().endswith(" builder")
 
 EnumValue = namedtuple('EnumValue', ['name', 'value'])
 class EnumType(Type):

generator/main.py

@@ -39,14 +39,6 @@ def link_object(obj, types):
     obj.methods = [method for method in methods if not is_native_method(method)]
     obj.native_methods = [method for method in methods if is_native_method(method)]
 
-    # Compute the built object type for builders
-    if obj.is_builder:
-        for method in obj.methods:
-            if method.name.canonical_case() == "get result":
-                obj.built_type = method.return_type
-                break
-        assert(obj.built_type != None)
-
 def link_structure(struct, types):
     struct.members = common.linked_record_members(struct.json_data['members'], types)
 
@@ -276,9 +268,6 @@ def as_cProc(type_name, method_name):
 
 def as_frontendType(typ):
     if typ.category == 'object':
-        if typ.is_builder:
-            return typ.name.CamelCase() + '*'
-        else:
         return typ.name.CamelCase() + 'Base*'
     elif typ.category in ['bitmask', 'enum']:
         return 'dawn::' + typ.name.CamelCase()
@@ -288,16 +277,7 @@ def as_frontendType(typ):
         return as_cType(typ.name)
 
 def cpp_native_methods(types, typ):
-    methods = typ.methods + typ.native_methods
+    return typ.methods + typ.native_methods
-
-    if typ.is_builder:
-        methods.append(common.Method(Name('set error callback'), types['void'], [
-            common.RecordMember(Name('callback'), types['builder error callback'], 'value', False, False),
-            common.RecordMember(Name('userdata1'), types['callback userdata'], 'value', False, False),
-            common.RecordMember(Name('userdata2'), types['callback userdata'], 'value', False, False),
-        ]))
-
-    return methods
 
 def c_native_methods(types, typ):
     return cpp_native_methods(types, typ) + [
@@ -346,7 +326,6 @@ def get_renders_for_targets(api_params, wire_json, targets):
     if 'dawn_headers' in targets:
         renders.append(FileRender('api.h', 'dawn/dawn.h', [base_params, api_params, c_params]))
         renders.append(FileRender('apicpp.h', 'dawn/dawncpp.h', [base_params, api_params, cpp_params]))
-        renders.append(FileRender('apicpp_traits.h', 'dawn/dawncpp_traits.h', [base_params, api_params, cpp_params]))
 
     if 'libdawn' in targets:
         additional_params = {'native_methods': lambda typ: cpp_native_methods(api_params['types'], typ)}

generator/templates/api.h

@@ -50,7 +50,6 @@
 // Custom types depending on the target language
 typedef uint64_t DawnCallbackUserdata;
 typedef void (*DawnDeviceErrorCallback)(const char* message, DawnCallbackUserdata userdata);
-typedef void (*DawnBuilderErrorCallback)(DawnBuilderErrorStatus status, const char* message, DawnCallbackUserdata userdata1, DawnCallbackUserdata userdata2);
 typedef void (*DawnBufferMapReadCallback)(DawnBufferMapAsyncStatus status, const void* data, uint32_t dataLength, DawnCallbackUserdata userdata);
 typedef void (*DawnBufferMapWriteCallback)(DawnBufferMapAsyncStatus status, void* data, uint32_t dataLength, DawnCallbackUserdata userdata);
 typedef void (*DawnFenceOnCompletionCallback)(DawnFenceCompletionStatus status,

generator/templates/apicpp.cpp

@@ -58,12 +58,7 @@ namespace dawn {
 
         {% macro render_cpp_method_declaration(type, method) %}
             {% set CppType = as_cppType(type.name) %}
-            {% if method.return_type.name.concatcase() == "void" and type.is_builder -%}
+            {{as_cppType(method.return_type.name)}} {{CppType}}::{{method.name.CamelCase()}}(
-                {{CppType}} const&
-            {%- else -%}
-                {{as_cppType(method.return_type.name)}}
-            {%- endif -%}
-            {{" "}}{{CppType}}::{{method.name.CamelCase()}}(
                 {%- for arg in method.arguments -%}
                     {%- if not loop.first %}, {% endif -%}
                     {%- if arg.type.category == "object" and arg.annotation == "value" -%}
@@ -99,9 +94,6 @@ namespace dawn {
             {{render_cpp_method_declaration(type, method)}} {
                 {% if method.return_type.name.concatcase() == "void" %}
                     {{render_cpp_to_c_method_call(type, method)}};
-                    {% if type.is_builder %}
-                        return *this;
-                    {% endif %}
                 {% else %}
                     auto result = {{render_cpp_to_c_method_call(type, method)}};
                     {% if method.return_type.category == "native" %} 

generator/templates/apicpp.h

@@ -129,12 +129,7 @@ namespace dawn {
 
     {% macro render_cpp_method_declaration(type, method) %}
         {% set CppType = as_cppType(type.name) %}
-        {% if method.return_type.name.concatcase() == "void" and type.is_builder -%}
+        DAWN_EXPORT {{as_cppType(method.return_type.name)}} {{method.name.CamelCase()}}(
-            DAWN_EXPORT {{CppType}} const&
-        {%- else -%}
-            DAWN_EXPORT {{as_cppType(method.return_type.name)}}
-        {%- endif -%}
-        {{" "}}{{method.name.CamelCase()}}(
             {%- for arg in method.arguments -%}
                 {%- if not loop.first %}, {% endif -%}
                 {%- if arg.type.category == "object" and arg.annotation == "value" -%}

generator/templates/apicpp_traits.h

@@ -1,43 +0,0 @@
-//* Copyright 2017 The Dawn Authors
-//*
-//* Licensed under the Apache License, Version 2.0 (the "License");
-//* you may not use this file except in compliance with the License.
-//* You may obtain a copy of the License at
-//*
-//*     http://www.apache.org/licenses/LICENSE-2.0
-//*
-//* Unless required by applicable law or agreed to in writing, software
-//* distributed under the License is distributed on an "AS IS" BASIS,
-//* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-//* See the License for the specific language governing permissions and
-//* limitations under the License.
-
-#ifndef DAWN_DAWNCPP_TRAITS_H_
-#define DAWN_DAWNCPP_TRAITS_H_
-
-#include "dawncpp.h"
-
-#include <type_traits>
-
-namespace dawn {
-
-    template<typename Builder>
-    using BuiltObject = decltype(std::declval<Builder>().GetResult());
-
-    template<typename BuiltObject>
-    struct BuiltObjectTrait {
-    };
-
-    {% for type in by_category["object"] if type.is_builder %}
-        template<>
-        struct BuiltObjectTrait<BuiltObject<{{as_cppType(type.name)}}>> {
-            using Builder = {{as_cppType(type.name)}};
-        };
-    {% endfor %}
-
-    template<typename BuiltObject>
-    using Builder = typename BuiltObjectTrait<BuiltObject>::Builder;
-
-}
-
-#endif // DAWN_DAWNCPP_TRAITS_H_

generator/templates/dawn_native/ProcTable.cpp

@@ -20,7 +20,7 @@
 #include "dawn_native/ValidationUtils_autogen.h"
 
 {% for type in by_category["object"] %}
-    {% if not type.is_builder and type.name.canonical_case() not in ["texture view"] %}
+    {% if type.name.canonical_case() not in ["texture view"] %}
         #include "dawn_native/{{type.name.CamelCase()}}.h"
     {% endif %}
 {% endfor %}
@@ -28,60 +28,11 @@
 namespace dawn_native {
 
     namespace {
-
-        {% set methodsWithExtraValidation = (
-            "CommandBufferBuilderGetResult",
-        ) %}
-
         {% for type in by_category["object"] %}
             {% for method in native_methods(type) %}
                 {% set suffix = as_MethodSuffix(type.name, method.name) %}
 
-                //* Autogenerated part of the entry point validation
+                {{as_cType(method.return_type.name)}} CToCpp{{suffix}}(
-                //*  - Check that enum and bitmaks are in the correct range
-                //*  - Check that builders have not been consumed already
-                bool ValidateBase{{suffix}}(
-                    {{-as_frontendType(type)}} self
-                    {%- for arg in method.arguments -%}
-                        , {{as_annotated_frontendType(arg)}}
-                    {%- endfor -%}
-                ) {
-                    {% if type.is_builder and method.name.canonical_case() not in ("release", "reference") %}
-                        if (!self->CanBeUsed()) {
-                            self->GetDevice()->HandleError("Builder cannot be used after GetResult");
-                            return false;
-                        }
-                    {% else %}
-                        DAWN_UNUSED(self);
-                    {% endif %}
-                    bool error = false;
-                    DAWN_UNUSED(error);
-                    {% for arg in method.arguments %}
-                        {% set cppType = as_cppType(arg.type.name) %}
-                        {% set argName = as_varName(arg.name) %}
-                        {% if arg.type.category in ["enum", "bitmask"] %}
-                            MaybeError {{argName}}Valid = Validate{{cppType}}(static_cast<dawn::{{cppType}}>({{argName}}));
-                            if ({{argName}}Valid.IsError()) {
-                                delete {{argName}}Valid.AcquireError();
-                                error = true;
-                            }
-                        {% else %}
-                            DAWN_UNUSED({{argName}});
-                        {% endif %}
-                        if (error) {
-                            {% if type.is_builder %}
-                                self->HandleError("Bad value in {{suffix}}");
-                            {% else %}
-                                self->GetDevice()->HandleError("Bad value in {{suffix}}");
-                            {% endif %}
-                            return false;
-                        }
-                    {% endfor %}
-                    return true;
-                }
-
-                //* Entry point with validation
-                {{as_cType(method.return_type.name)}} Validating{{suffix}}(
                     {{-as_cType(type.name)}} cSelf
                     {%- for arg in method.arguments -%}
                         , {{as_annotated_cType(arg)}}
@@ -101,55 +52,7 @@ namespace dawn_native {
                         {% endif %}
                     {%- endfor-%}
 
-                    //* Do the autogenerated checks
+                    {% if method.return_type.name.canonical_case() != "void" %}
-                    bool valid = ValidateBase{{suffix}}(self
-                        {%- for arg in method.arguments -%}
-                            , {{as_varName(arg.name)}}_
-                        {%- endfor -%}
-                    );
-
-                    //* Some function have very heavy checks in a separate method, so that they
-                    //* can be skipped in the NonValidatingEntryPoints.
-                    {% if suffix in methodsWithExtraValidation %}
-                        if (valid) {
-                            MaybeError error = self->Validate{{method.name.CamelCase()}}(
-                                {%- for arg in method.arguments -%}
-                                    {% if not loop.first %}, {% endif %}{{as_varName(arg.name)}}_
-                                {%- endfor -%}
-                            );
-                            //* Builders want to handle error themselves, unpack the error and make
-                            //* the builder handle it.
-                            {% if type.is_builder %}
-                                if (error.IsError()) {
-                                    ErrorData* errorData = error.AcquireError();
-                                    self->HandleError(errorData->GetMessage().c_str());
-                                    delete errorData;
-                                    valid = false;
-                                }
-                            {% else %}
-                                //* Non-builder errors are handled by the device
-                                valid = !self->GetDevice()->ConsumedError(std::move(error));
-                            {% endif %}
-                        }
-                    {% endif %}
-
-                    //* HACK(cwallez@chromium.org): special casing GetResult so that the error callback
-                    //* is called if needed. Without this, no call to HandleResult would happen, and the
-                    //* error callback would always get called with an Unknown status
-                    {% if type.is_builder and method.name.canonical_case() == "get result" %}
-                        if (!valid) {
-                            {{as_frontendType(method.return_type)}} fakeResult = nullptr;
-                            bool shouldBeFalse = self->HandleResult(fakeResult);
-                            ASSERT(shouldBeFalse == false);
-                        }
-                    {% endif %}
-
-                    {% if method.return_type.name.canonical_case() == "void" %}
-                        if (!valid) return;
-                    {% else %}
-                        if (!valid) {
-                            return {};
-                        }
                         auto result =
                     {%- endif %}
                     self->{{method.name.CamelCase()}}(
@@ -174,9 +77,10 @@ namespace dawn_native {
         DawnProcTable table;
         {% for type in by_category["object"] %}
             {% for method in native_methods(type) %}
-                table.{{as_varName(type.name, method.name)}} = Validating{{as_MethodSuffix(type.name, method.name)}};
+                table.{{as_varName(type.name, method.name)}} = CToCpp{{as_MethodSuffix(type.name, method.name)}};
             {% endfor %}
         {% endfor %}
         return table;
     }
+
 }

src/dawn/BUILD.gn

@@ -30,7 +30,6 @@ dawn_generator("dawn_headers_gen") {
   outputs = [
     "dawn/dawncpp.h",
     "dawn/dawn.h",
-    "dawn/dawncpp_traits.h",
   ]
 }
 

src/dawn_native/CommandEncoder.h

@@ -27,11 +27,7 @@
 namespace dawn_native {
 
     struct BeginRenderPassCmd;
-    class CommandBufferBuilder;
 
-    // CommandEncoder is temporarily a wrapper around CommandBufferBuilder so the two can coexist
-    // while code is migrated to the new shiny CommandEncoder interface. It captures any command
-    // buffer builder error and defers to trigger a device error when "Finish" is called.
     class CommandEncoderBase : public ObjectBase {
       public:
         CommandEncoderBase(DeviceBase* device);

src/dawn_native/Device.h

@@ -66,7 +66,7 @@ namespace dawn_native {
         virtual void TickImpl() = 0;
 
         // Many Dawn objects are completely immutable once created which means that if two
-        // builders are given the same arguments, they can return the same object. Reusing
+        // creations are given the same arguments, they can return the same object. Reusing
         // objects will help make comparisons between objects by a single pointer comparison.
         //
         // Technically no object is immutable as they have a reference count, and an
@@ -75,10 +75,10 @@ namespace dawn_native {
         // the client-server wire every creation will get a different proxy object, with a
         // different reference count.
         //
-        // When trying to create an object, we give both the builder and an example of what
+        // When trying to create an object, we give both the descriptor and an example of what
-        // the built object will be, the "blueprint". The blueprint is just a FooBase object
+        // the created object will be, the "blueprint". The blueprint is just a FooBase object
         // instead of a backend Foo object. If the blueprint doesn't match an object in the
-        // cache, then the builder is used to make a new object.
+        // cache, then the descriptor is used to make a new object.
         ResultOrError<BindGroupLayoutBase*> GetOrCreateBindGroupLayout(
             const BindGroupLayoutDescriptor* descriptor);
         void UncacheBindGroupLayout(BindGroupLayoutBase* obj);

src/dawn_native/ProgrammablePassEncoder.cpp

@@ -18,6 +18,7 @@
 #include "dawn_native/CommandBuffer.h"
 #include "dawn_native/Commands.h"
 #include "dawn_native/Device.h"
+#include "dawn_native/ValidationUtils_autogen.h"
 
 #include <string.h>
 
@@ -115,6 +116,10 @@ namespace dawn_native {
             return;
         }
 
+        if (mTopLevelEncoder->ConsumedError(ValidateShaderStageBit(stages))) {
+            return;
+        }
+
         // TODO(cwallez@chromium.org): check for overflows
         if (offset + count > kMaxPushConstants) {
             mTopLevelEncoder->HandleError("Setting too many push constants");

src/dawn_native/Queue.cpp

@@ -60,12 +60,6 @@ namespace dawn_native {
         DAWN_TRY(GetDevice()->ValidateObject(this));
 
         for (uint32_t i = 0; i < commandCount; ++i) {
-            // TODO(cwallez@chromium.org): Remove this once CommandBufferBuilder doesn't use the
-            // builder mechanism anymore.
-            if (commands[i] == nullptr) {
-                return DAWN_VALIDATION_ERROR("Command buffers cannot be null");
-            }
-
             DAWN_TRY(GetDevice()->ValidateObject(commands[i]));
 
             const CommandBufferResourceUsage& usages = commands[i]->GetResourceUsages();

src/dawn_native/ShaderModule.cpp

@@ -82,7 +82,7 @@ namespace dawn_native {
         ASSERT(!IsError());
 
         DeviceBase* device = GetDevice();
-        // TODO(cwallez@chromium.org): make errors here builder-level
+        // TODO(cwallez@chromium.org): make errors here creation errors
         // currently errors here do not prevent the shadermodule from being used
         const auto& resources = compiler.get_shader_resources();