Changeset 249369 in webkit


Ignore:
Timestamp:
Sep 1, 2019 4:08:38 PM (5 years ago)
Author:
mmaxfield@apple.com
Message:

[WHLSL] Resources don't work when only a subset of a bind group is referenced by a shader
​https://bugs.webkit.org/show_bug.cgi?id=201383

Reviewed by Dean Jackson.

Source/WebCore:

Bind groups correspond to argument buffers in Metal. Both the Metal API and Metal Shading Language
have to agree on the layout of exactly which resources lie at which byte offsets within an argument
buffer.

Before this patch, we only emitted code for the items in the argument buffer that were actually
referenced by the shader source code. However, because these items are held inside a struct, if
we omit one item from the middle of the struct, the byte offets of all the successive items would
be wrong. This means that the Metal API and the shader would disagree about how to access these
resources, making the resources inaccessible (and causing security problems).

Tests: webgpu/whlsl/sparse-bind-group-2.html

webgpu/whlsl/sparse-bind-group-3.html
webgpu/whlsl/sparse-bind-group.html

  • Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:

(WebCore::WHLSL::Metal::EntryPointScaffolding::emitResourceHelperTypes):
(WebCore::WHLSL::Metal::VertexEntryPointScaffolding::emitHelperTypes):
(WebCore::WHLSL::Metal::FragmentEntryPointScaffolding::emitHelperTypes):
(WebCore::WHLSL::Metal::ComputeEntryPointScaffolding::emitHelperTypes):

  • Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.h:
  • Modules/webgpu/WHLSL/WHLSLSemanticMatcher.cpp:

(WebCore::WHLSL::matchResources):
(WebCore::WHLSL::matchVertexAttributes):
(WebCore::WHLSL::matchColorAttachments):

LayoutTests:

  • webgpu/whlsl/compute.html:
  • webgpu/whlsl/sparse-bind-group-2-expected.txt: Added.
  • webgpu/whlsl/sparse-bind-group-2.html: Added.
  • webgpu/whlsl/sparse-bind-group-3-expected.txt: Added.
  • webgpu/whlsl/sparse-bind-group-3.html: Added.
  • webgpu/whlsl/sparse-bind-group-expected.txt: Added.
  • webgpu/whlsl/sparse-bind-group.html: Added.
Location:
trunk
Files:
6 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r249368 r249369  
     12019-09-01  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        [WHLSL] Resources don't work when only a subset of a bind group is referenced by a shader
     4        https://bugs.webkit.org/show_bug.cgi?id=201383
     5
     6        Reviewed by Dean Jackson.
     7
     8        * webgpu/whlsl/compute.html:
     9        * webgpu/whlsl/sparse-bind-group-2-expected.txt: Added.
     10        * webgpu/whlsl/sparse-bind-group-2.html: Added.
     11        * webgpu/whlsl/sparse-bind-group-3-expected.txt: Added.
     12        * webgpu/whlsl/sparse-bind-group-3.html: Added.
     13        * webgpu/whlsl/sparse-bind-group-expected.txt: Added.
     14        * webgpu/whlsl/sparse-bind-group.html: Added.
     15
    1162019-09-01  Wenson Hsieh  <wenson_hsieh@apple.com>
    217

  • trunk/LayoutTests/webgpu/whlsl/compute.html

    r249131 r249369  
    7575    resultsBuffer.unmap();
    7676}
     77
    7778window.jsTestIsAsync = true;
    7879getBasicDevice().then(function(device) {

  • trunk/Source/WebCore/ChangeLog

    r249367 r249369  
     12019-09-01  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        [WHLSL] Resources don't work when only a subset of a bind group is referenced by a shader
     4        https://bugs.webkit.org/show_bug.cgi?id=201383
     5
     6        Reviewed by Dean Jackson.
     7
     8        Bind groups correspond to argument buffers in Metal. Both the Metal API and Metal Shading Language
     9        have to agree on the layout of exactly which resources lie at which byte offsets within an argument
     10        buffer.
     11
     12        Before this patch, we only emitted code for the items in the argument buffer that were actually
     13        referenced by the shader source code. However, because these items are held inside a struct, if
     14        we omit one item from the middle of the struct, the byte offets of all the successive items would
     15        be wrong. This means that the Metal API and the shader would disagree about how to access these
     16        resources, making the resources inaccessible (and causing security problems).
     17
     18        Tests: webgpu/whlsl/sparse-bind-group-2.html
     19               webgpu/whlsl/sparse-bind-group-3.html
     20               webgpu/whlsl/sparse-bind-group.html
     21
     22        * Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:
     23        (WebCore::WHLSL::Metal::EntryPointScaffolding::emitResourceHelperTypes):
     24        (WebCore::WHLSL::Metal::VertexEntryPointScaffolding::emitHelperTypes):
     25        (WebCore::WHLSL::Metal::FragmentEntryPointScaffolding::emitHelperTypes):
     26        (WebCore::WHLSL::Metal::ComputeEntryPointScaffolding::emitHelperTypes):
     27        * Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.h:
     28        * Modules/webgpu/WHLSL/WHLSLSemanticMatcher.cpp:
     29        (WebCore::WHLSL::matchResources):
     30        (WebCore::WHLSL::matchVertexAttributes):
     31        (WebCore::WHLSL::matchColorAttachments):
     32
    1332019-09-01  Said Abou-Hallawa  <sabouhallawa@apple.com>
    234

  • trunk/Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp

    r248892 r249369  
    140140}
    141141
    142 void EntryPointScaffolding::emitResourceHelperTypes(StringBuilder& stringBuilder, Indentation<4> indent)
     142void EntryPointScaffolding::emitResourceHelperTypes(StringBuilder& stringBuilder, Indentation<4> indent, ShaderStage shaderStage)
    143143{
    144144    for (size_t i = 0; i < m_layout.size(); ++i) {
    … …  
    148148            Vector<std::pair<unsigned, String>> structItems;
    149149            for (size_t j = 0; j < m_layout[i].bindings.size(); ++j) {
     150                auto& binding = m_layout[i].bindings[j];
     151                if (!binding.visibility.contains(shaderStage))
     152                    continue;
     153
     154                auto elementName = m_namedBindGroups[i].namedBindings[j].elementName;
     155                auto index = m_namedBindGroups[i].namedBindings[j].index;
     156                if (auto lengthInformation = m_namedBindGroups[i].namedBindings[j].lengthInformation)
     157                    structItems.append(std::make_pair(lengthInformation->index, makeString("uint2 ", lengthInformation->elementName, " [[id(", lengthInformation->index, ")]];")));
     158
    150159                auto iterator = m_resourceMap.find(&m_layout[i].bindings[j]);
    151                 if (iterator == m_resourceMap.end())
    152                     continue;
    153                 auto& type = m_entryPointItems.inputs[iterator->value].unnamedType->unifyNode();
    154                 if (is<AST::UnnamedType>(type) && is<AST::ReferenceType>(downcast<AST::UnnamedType>(type))) {
    155                     auto& referenceType = downcast<AST::ReferenceType>(downcast<AST::UnnamedType>(type));
    156                     auto mangledTypeName = m_typeNamer.mangledNameForType(referenceType.elementType());
    157                     auto addressSpace = toString(referenceType.addressSpace());
    158                     auto elementName = m_namedBindGroups[i].namedBindings[j].elementName;
    159                     auto index = m_namedBindGroups[i].namedBindings[j].index;
    160                     structItems.append(std::make_pair(index, makeString(addressSpace, " ", mangledTypeName, "* ", elementName, " [[id(", index, ")]];")));
    161                     if (auto lengthInformation = m_namedBindGroups[i].namedBindings[j].lengthInformation)
    162                         structItems.append(std::make_pair(lengthInformation->index, makeString("uint2 ", lengthInformation->elementName, " [[id(", lengthInformation->index, ")]];")));
    163                 } else if (is<AST::NamedType>(type) && is<AST::NativeTypeDeclaration>(downcast<AST::NamedType>(type))) {
    164                     auto& namedType = downcast<AST::NativeTypeDeclaration>(downcast<AST::NamedType>(type));
    165                     auto mangledTypeName = m_typeNamer.mangledNameForType(namedType);
    166                     auto elementName = m_namedBindGroups[i].namedBindings[j].elementName;
    167                     auto index = m_namedBindGroups[i].namedBindings[j].index;
    168                     structItems.append(std::make_pair(index, makeString(mangledTypeName, ' ', elementName, " [[id(", index, ")]];")));
     160                if (iterator != m_resourceMap.end()) {
     161                    auto& type = m_entryPointItems.inputs[iterator->value].unnamedType->unifyNode();
     162                    if (is<AST::UnnamedType>(type) && is<AST::ReferenceType>(downcast<AST::UnnamedType>(type))) {
     163                        auto& referenceType = downcast<AST::ReferenceType>(downcast<AST::UnnamedType>(type));
     164                        auto mangledTypeName = m_typeNamer.mangledNameForType(referenceType.elementType());
     165                        auto addressSpace = toString(referenceType.addressSpace());
     166                        structItems.append(std::make_pair(index, makeString(addressSpace, " ", mangledTypeName, "* ", elementName, " [[id(", index, ")]];")));
     167                    } else if (is<AST::NamedType>(type) && is<AST::NativeTypeDeclaration>(downcast<AST::NamedType>(type))) {
     168                        auto& namedType = downcast<AST::NativeTypeDeclaration>(downcast<AST::NamedType>(type));
     169                        auto mangledTypeName = m_typeNamer.mangledNameForType(namedType);
     170                        structItems.append(std::make_pair(index, makeString(mangledTypeName, ' ', elementName, " [[id(", index, ")]];")));
     171                    }
     172                } else {
     173                    // The binding doesn't appear in the shader source.
     174                    // However, we must still emit a placeholder, so successive items in the argument buffer struct have the correct offset.
     175                    // Because the binding doesn't appear in the shader source, we don't know which exact type the bind point should have.
     176                    // Therefore, we must synthesize a type out of thin air.
     177                    WTF::visit(WTF::makeVisitor([&](UniformBufferBinding) {
     178                        structItems.append(std::make_pair(index, makeString("constant void* ", elementName, " [[id(", index, ")]];")));
     179                    }, [&](SamplerBinding) {
     180                        structItems.append(std::make_pair(index, makeString("sampler ", elementName, " [[id(", index, ")]];")));
     181                    }, [&](TextureBinding) {
     182                        // FIXME: https://bugs.webkit.org/show_bug.cgi?id=201384 We don't know which texture type the binding represents. This is no good very bad.
     183                        structItems.append(std::make_pair(index, makeString("texture2d<float4> ", elementName, " [[id(", index, ")]];")));
     184                    }, [&](StorageBufferBinding) {
     185                        structItems.append(std::make_pair(index, makeString("device void* ", elementName, " [[id(", index, ")]];")));
     186                    }), binding.binding);
    169187                }
    170188            }
    … …  
    426444    stringBuilder.append(indent, "};\n\n");
    427445   
    428     emitResourceHelperTypes(stringBuilder, indent);
     446    emitResourceHelperTypes(stringBuilder, indent, ShaderStage::Vertex);
    429447}
    430448
    … …  
    531549    stringBuilder.append(indent, "};\n\n");
    532550
    533     emitResourceHelperTypes(stringBuilder, indent);
     551    emitResourceHelperTypes(stringBuilder, indent, ShaderStage::Fragment);
    534552}
    535553
    … …  
    581599void ComputeEntryPointScaffolding::emitHelperTypes(StringBuilder& stringBuilder, Indentation<4> indent)
    582600{
    583     emitResourceHelperTypes(stringBuilder, indent);
     601    emitResourceHelperTypes(stringBuilder, indent, ShaderStage::Compute);
    584602}
    585603

  • trunk/Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.h

    r248892 r249369  
    6565    EntryPointScaffolding(AST::FunctionDefinition&, Intrinsics&, TypeNamer&, EntryPointItems&, HashMap<Binding*, size_t>& resourceMap, Layout&, std::function<MangledVariableName()>&& generateNextVariableName);
    6666
    67     void emitResourceHelperTypes(StringBuilder&, Indentation<4>);
     67    void emitResourceHelperTypes(StringBuilder&, Indentation<4>, ShaderStage);
    6868
    6969    enum class IncludePrecedingComma {

  • trunk/Source/WebCore/Modules/webgpu/WHLSL/WHLSLSemanticMatcher.cpp

    r248993 r249369  
    6262{
    6363    HashMap<Binding*, size_t> result;
    64     HashSet<size_t> itemIndices;
    65     if (entryPointItems.size() == std::numeric_limits<size_t>::max())
    66         return WTF::nullopt; // Work around the fact that HashSet's keys are restricted.
     64    HashSet<size_t, DefaultHash<size_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<size_t>> itemIndices;
    6765    for (auto& bindGroup : layout) {
    6866        auto space = bindGroup.name;
    … …  
    8381                    continue;
    8482                result.add(&binding, i);
    85                 itemIndices.add(i + 1); // Work around the fact that HashSet's keys are restricted.
     83                itemIndices.add(i);
    8684            }
    8785        }
    … …  
    9391        if (!WTF::holds_alternative<AST::ResourceSemantic>(semantic))
    9492            continue;
    95         if (!itemIndices.contains(i + 1))
     93        if (!itemIndices.contains(i))
    9694            return WTF::nullopt;
    9795    }
    … …  
    143141{
    144142    HashMap<VertexAttribute*, size_t> result;
    145     HashSet<size_t> itemIndices;
    146     if (vertexInputs.size() == std::numeric_limits<size_t>::max())
    147         return WTF::nullopt; // Work around the fact that HashSet's keys are restricted.
     143    HashSet<size_t, DefaultHash<size_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<size_t>> itemIndices;
    148144    for (auto& vertexAttribute : vertexAttributes) {
    149145        for (size_t i = 0; i < vertexInputs.size(); ++i) {
    … …  
    158154                return WTF::nullopt;
    159155            result.add(&vertexAttribute, i);
    160             itemIndices.add(i + 1); // Work around the fact that HashSet's keys are restricted.
     156            itemIndices.add(i);
    161157        }
    162158    }
    … …  
    167163        if (!WTF::holds_alternative<AST::StageInOutSemantic>(semantic))
    168164            continue;
    169         if (!itemIndices.contains(i + 1))
     165        if (!itemIndices.contains(i))
    170166            return WTF::nullopt;
    171167    }
    … …  
    231227{
    232228    HashMap<AttachmentDescriptor*, size_t> result;
    233     HashSet<size_t> itemIndices;
    234     if (attachmentDescriptors.size() == std::numeric_limits<size_t>::max())
    235         return WTF::nullopt; // Work around the fact that HashSet's keys are restricted.
     229    HashSet<size_t, DefaultHash<size_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<size_t>> itemIndices;
    236230    for (auto& attachmentDescriptor : attachmentDescriptors) {
    237231        for (size_t i = 0; i < fragmentOutputs.size(); ++i) {
    … …  
    246240                return WTF::nullopt;
    247241            result.add(&attachmentDescriptor, i);
    248             itemIndices.add(i + 1); // Work around the fact that HashSet's keys are restricted.
     242            itemIndices.add(i);
    249243        }
    250244    }
    … …  
    255249        if (!WTF::holds_alternative<AST::StageInOutSemantic>(semantic))
    256250            continue;
    257         if (!itemIndices.contains(i + 1))
     251        if (!itemIndices.contains(i))
    258252            return WTF::nullopt;
    259253    }

  • trunk/Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm

    r246628 r249369  
    8585    END_BLOCK_OBJC_EXCEPTIONS;
    8686
     87    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=201384 This needs to set the "textureType" field
    8788    [mtlArgument setDataType:dataType];
    8889    [mtlArgument setIndex:index];
Note: See TracChangeset for help on using the changeset viewer.