Emit tint warning when creating shader module

Add the formatted tint messages in OwnedCompilationMessages, which will be emit after creating the shader module succeeds of fails.
This patch also change the compilation messages handling in creating shader modules. Now the compilation messages are separated from sparseResult, and should be handled manually. A new method, ShaderModuleBase::InjectCompilationMessages, is introduced to move a given OwnedCompilationMessages into the shader module, and emit the formatted tint errors and warnings. This method should be called explicitly on a valid or error shader module.

Bug: dawn:753
Change-Id: I5825186c6d9c4aa7725aebd0c302bfce5e1f37cf
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/53890
Commit-Queue: Zhaoming Jiang <zhaoming.jiang@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/CompilationMessages.cpp

@@ -41,12 +41,12 @@ namespace dawn_native {
         mCompilationInfo.messages = nullptr;
     }
 
-    void OwnedCompilationMessages::AddMessage(std::string message,
+    void OwnedCompilationMessages::AddMessageForTesting(std::string message,
-                                              wgpu::CompilationMessageType type,
+                                                        wgpu::CompilationMessageType type,
-                                              uint64_t lineNum,
+                                                        uint64_t lineNum,
-                                              uint64_t linePos,
+                                                        uint64_t linePos,
-                                              uint64_t offset,
+                                                        uint64_t offset,
-                                              uint64_t length) {
+                                                        uint64_t length) {
         // Cannot add messages after GetCompilationInfo has been called.
         ASSERT(mCompilationInfo.messages == nullptr);
 
@@ -111,6 +111,8 @@ namespace dawn_native {
         for (const auto& diag : diagnostics) {
             AddMessage(diag);
         }
+
+        AddFormattedTintMessages(diagnostics);
     }
 
     void OwnedCompilationMessages::ClearMessages() {
@@ -136,4 +138,50 @@ namespace dawn_native {
         return &mCompilationInfo;
     }
 
+    const std::vector<std::string>& OwnedCompilationMessages::GetFormattedTintMessages() {
+        return mFormattedTintMessages;
+    }
+
+    void OwnedCompilationMessages::AddFormattedTintMessages(const tint::diag::List& diagnostics) {
+        tint::diag::List messageList;
+        size_t warningCount = 0;
+        size_t errorCount = 0;
+        for (auto& diag : diagnostics) {
+            switch (diag.severity) {
+                case (tint::diag::Severity::Fatal):
+                case (tint::diag::Severity::Error):
+                case (tint::diag::Severity::InternalCompilerError): {
+                    errorCount++;
+                    messageList.add(tint::diag::Diagnostic(diag));
+                    break;
+                }
+                case (tint::diag::Severity::Warning): {
+                    warningCount++;
+                    messageList.add(tint::diag::Diagnostic(diag));
+                    break;
+                }
+                default:
+                    break;
+            }
+        }
+        if (errorCount == 0 && warningCount == 0) {
+            return;
+        }
+        tint::diag::Formatter::Style style;
+        style.print_newline_at_end = false;
+        std::ostringstream t;
+        if (errorCount > 0) {
+            t << errorCount << " error(s) ";
+            if (warningCount > 0) {
+                t << "and ";
+            }
+        }
+        if (warningCount > 0) {
+            t << warningCount << " warning(s) ";
+        }
+        t << "generated while compiling the shader:" << std::endl
+          << tint::diag::Formatter{style}.format(messageList);
+        mFormattedTintMessages.push_back(t.str());
+    }
+
 }  // namespace dawn_native

src/dawn_native/CompilationMessages.h

@@ -34,22 +34,27 @@ namespace dawn_native {
         OwnedCompilationMessages();
         ~OwnedCompilationMessages() = default;
 
-        void AddMessage(std::string message,
+        void AddMessageForTesting(
-                        wgpu::CompilationMessageType type = wgpu::CompilationMessageType::Info,
+            std::string message,
-                        uint64_t lineNum = 0,
+            wgpu::CompilationMessageType type = wgpu::CompilationMessageType::Info,
-                        uint64_t linePos = 0,
+            uint64_t lineNum = 0,
-                        uint64_t offset = 0,
+            uint64_t linePos = 0,
-                        uint64_t length = 0);
+            uint64_t offset = 0,
-        void AddMessage(const tint::diag::Diagnostic& diagnostic);
+            uint64_t length = 0);
         void AddMessages(const tint::diag::List& diagnostics);
         void ClearMessages();
 
         const WGPUCompilationInfo* GetCompilationInfo();
+        const std::vector<std::string>& GetFormattedTintMessages();
 
       private:
+        void AddMessage(const tint::diag::Diagnostic& diagnostic);
+        void AddFormattedTintMessages(const tint::diag::List& diagnostics);
+
         WGPUCompilationInfo mCompilationInfo;
         std::vector<std::string> mMessageStrings;
         std::vector<WGPUCompilationMessage> mMessages;
+        std::vector<std::string> mFormattedTintMessages;
     };
 
 }  // namespace dawn_native

src/dawn_native/Device.cpp

@@ -649,7 +649,8 @@ namespace dawn_native {
 
     ResultOrError<Ref<ShaderModuleBase>> DeviceBase::GetOrCreateShaderModule(
         const ShaderModuleDescriptor* descriptor,
-        ShaderModuleParseResult* parseResult) {
+        ShaderModuleParseResult* parseResult,
+        OwnedCompilationMessages* compilationMessages) {
         ASSERT(parseResult != nullptr);
 
         ShaderModuleBase blueprint(this, descriptor);
@@ -668,7 +669,8 @@ namespace dawn_native {
                 // now, so call validate. Most of |ValidateShaderModuleDescriptor| is parsing, but
                 // we can consider splitting it if additional validation is added.
                 ASSERT(!IsValidationEnabled());
-                DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, parseResult));
+                DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, parseResult,
+                                                        compilationMessages));
             }
             DAWN_TRY_ASSIGN(result, CreateShaderModuleImpl(descriptor, parseResult));
             result->SetIsCachedReference();
@@ -839,10 +841,15 @@ namespace dawn_native {
     }
     ShaderModuleBase* DeviceBase::APICreateShaderModule(const ShaderModuleDescriptor* descriptor) {
         Ref<ShaderModuleBase> result;
-        ShaderModuleParseResult parseResult = {};
+        std::unique_ptr<OwnedCompilationMessages> compilationMessages(
-        if (ConsumedError(CreateShaderModule(descriptor, &parseResult), &result)) {
+            std::make_unique<OwnedCompilationMessages>());
-            return ShaderModuleBase::MakeError(this, std::move(parseResult.compilationMessages));
+        if (ConsumedError(CreateShaderModule(descriptor, compilationMessages.get()), &result)) {
+            result = ShaderModuleBase::MakeError(this);
         }
+        // Move compilation messages into ShaderModuleBase and emit tint errors and warnings
+        // after all other operations are finished successfully.
+        result->InjectCompilationMessages(std::move(compilationMessages));
+
         return result.Detach();
     }
     SwapChainBase* DeviceBase::APICreateSwapChain(Surface* surface,
@@ -1228,22 +1235,20 @@ namespace dawn_native {
 
     ResultOrError<Ref<ShaderModuleBase>> DeviceBase::CreateShaderModule(
         const ShaderModuleDescriptor* descriptor,
-        ShaderModuleParseResult* parseResult) {
+        OwnedCompilationMessages* compilationMessages) {
         DAWN_TRY(ValidateIsAlive());
 
-        // ShaderModule can be called from inside dawn_native. If that's the case handle the error
+        // CreateShaderModule can be called from inside dawn_native. If that's the case handle the
-        // directly in Dawn and don't need the parse results since there should be no validation
+        // error directly in Dawn and no compilationMessages held in the shader module. It is ok as
-        // errors.
+        // long as dawn_native don't use the compilationMessages of these internal shader modules.
-        ShaderModuleParseResult ignoredResults;
+        ShaderModuleParseResult parseResult;
-        if (parseResult == nullptr) {
-            parseResult = &ignoredResults;
-        }
 
         if (IsValidationEnabled()) {
-            DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, parseResult));
+            DAWN_TRY(ValidateShaderModuleDescriptor(this, descriptor, &parseResult,
+                                                    compilationMessages));
         }
 
-        return GetOrCreateShaderModule(descriptor, parseResult);
+        return GetOrCreateShaderModule(descriptor, &parseResult, compilationMessages);
     }
 
     ResultOrError<Ref<SwapChainBase>> DeviceBase::CreateSwapChain(

src/dawn_native/Device.h

@@ -133,7 +133,8 @@ namespace dawn_native {
 
         ResultOrError<Ref<ShaderModuleBase>> GetOrCreateShaderModule(
             const ShaderModuleDescriptor* descriptor,
-            ShaderModuleParseResult* parseResult);
+            ShaderModuleParseResult* parseResult,
+            OwnedCompilationMessages* compilationMessages);
         void UncacheShaderModule(ShaderModuleBase* obj);
 
         Ref<AttachmentState> GetOrCreateAttachmentState(AttachmentStateBlueprint* blueprint);
@@ -166,7 +167,7 @@ namespace dawn_native {
         ResultOrError<Ref<SamplerBase>> CreateSampler(const SamplerDescriptor* descriptor);
         ResultOrError<Ref<ShaderModuleBase>> CreateShaderModule(
             const ShaderModuleDescriptor* descriptor,
-            ShaderModuleParseResult* parseResult = nullptr);
+            OwnedCompilationMessages* compilationMessages = nullptr);
         ResultOrError<Ref<SwapChainBase>> CreateSwapChain(Surface* surface,
                                                           const SwapChainDescriptor* descriptor);
         ResultOrError<Ref<TextureBase>> CreateTexture(const TextureDescriptor* descriptor);

src/dawn_native/ShaderModule.cpp

@@ -1042,9 +1042,7 @@ namespace dawn_native {
         }
     }  // anonymous namespace
 
-    ShaderModuleParseResult::ShaderModuleParseResult()
+    ShaderModuleParseResult::ShaderModuleParseResult() = default;
-        : compilationMessages(new OwnedCompilationMessages()) {
-    }
     ShaderModuleParseResult::~ShaderModuleParseResult() = default;
 
     ShaderModuleParseResult::ShaderModuleParseResult(ShaderModuleParseResult&& rhs) = default;
@@ -1069,7 +1067,8 @@ namespace dawn_native {
 
     MaybeError ValidateShaderModuleDescriptor(DeviceBase* device,
                                               const ShaderModuleDescriptor* descriptor,
-                                              ShaderModuleParseResult* parseResult) {
+                                              ShaderModuleParseResult* parseResult,
+                                              OwnedCompilationMessages* outMessages) {
         ASSERT(parseResult != nullptr);
 
         const ChainedStruct* chainedDescriptor = descriptor->nextInChain;
@@ -1080,8 +1079,6 @@ namespace dawn_native {
         DAWN_TRY(ValidateSingleSType(chainedDescriptor, wgpu::SType::ShaderModuleSPIRVDescriptor,
                                      wgpu::SType::ShaderModuleWGSLDescriptor));
 
-        OwnedCompilationMessages* outMessages = parseResult->compilationMessages.get();
-
         ScopedTintICEHandler scopedICEHandler(device);
 
         const ShaderModuleSPIRVDescriptor* spirvDesc = nullptr;
@@ -1236,13 +1233,8 @@ namespace dawn_native {
         }
     }
 
-    ShaderModuleBase::ShaderModuleBase(
+    ShaderModuleBase::ShaderModuleBase(DeviceBase* device, ObjectBase::ErrorTag tag)
-        DeviceBase* device,
+        : CachedObject(device, tag), mType(Type::Undefined) {
-        ObjectBase::ErrorTag tag,
-        std::unique_ptr<OwnedCompilationMessages> compilationMessages)
-        : CachedObject(device, tag),
-          mType(Type::Undefined),
-          mCompilationMessages(std::move(compilationMessages)) {
     }
 
     ShaderModuleBase::~ShaderModuleBase() {
@@ -1252,10 +1244,8 @@ namespace dawn_native {
     }
 
     // static
-    ShaderModuleBase* ShaderModuleBase::MakeError(
+    ShaderModuleBase* ShaderModuleBase::MakeError(DeviceBase* device) {
-        DeviceBase* device,
+        return new ShaderModuleBase(device, ObjectBase::kError);
-        std::unique_ptr<OwnedCompilationMessages> compilationMessages) {
-        return new ShaderModuleBase(device, ObjectBase::kError, std::move(compilationMessages));
     }
 
     bool ShaderModuleBase::HasEntryPoint(const std::string& entryPoint) const {
@@ -1301,6 +1291,42 @@ namespace dawn_native {
                  mCompilationMessages->GetCompilationInfo(), userdata);
     }
 
+    void ShaderModuleBase::InjectCompilationMessages(
+        std::unique_ptr<OwnedCompilationMessages> compilationMessages) {
+        // TODO(zhaoming.jiang@intel.com): ensure the InjectCompilationMessages is properly
+        // handled for shader module returned from cache.
+        // InjectCompilationMessages should be called only once for a shader module, after
+        // it is created. However currently InjectCompilationMessages may be called on a
+        // shader module returned from cache rather than newly created, and violate the rule.
+        // We just skip the injection in this case for now, but a proper solution including
+        // ensure the cache goes before the validation is required.
+        if (mCompilationMessages != nullptr) {
+            return;
+        }
+        // Move the compilationMessages into the shader module and emit the tint errors and warnings
+        mCompilationMessages = std::move(compilationMessages);
+
+        // Emit the formatted Tint errors and warnings within the moved compilationMessages
+        const std::vector<std::string>& formattedTintMessages =
+            mCompilationMessages->GetFormattedTintMessages();
+        if (formattedTintMessages.empty()) {
+            return;
+        }
+        std::ostringstream t;
+        for (auto pMessage = formattedTintMessages.begin(); pMessage != formattedTintMessages.end();
+             pMessage++) {
+            if (pMessage != formattedTintMessages.begin()) {
+                t << std::endl;
+            }
+            t << *pMessage;
+        }
+        this->GetDevice()->EmitLog(WGPULoggingType_Warning, t.str().c_str());
+    }
+
+    OwnedCompilationMessages* ShaderModuleBase::GetCompilationMessages() const {
+        return mCompilationMessages.get();
+    }
+
     ResultOrError<std::vector<uint32_t>> ShaderModuleBase::GeneratePullingSpirv(
         const std::vector<uint32_t>& spirv,
         const VertexState& vertexState,
@@ -1358,7 +1384,6 @@ namespace dawn_native {
         mTintProgram = std::move(parseResult->tintProgram);
         mTintSource = std::move(parseResult->tintSource);
         mSpirv = std::move(parseResult->spirv);
-        mCompilationMessages = std::move(parseResult->compilationMessages);
 
         if (GetDevice()->IsToggleEnabled(Toggle::UseTintGenerator)) {
             DAWN_TRY_ASSIGN(mEntryPoints, ReflectShaderUsingTint(GetDevice(), mTintProgram.get()));

src/dawn_native/ShaderModule.h

@@ -75,12 +75,12 @@ namespace dawn_native {
         std::unique_ptr<tint::Program> tintProgram;
         std::unique_ptr<TintSource> tintSource;
         std::vector<uint32_t> spirv;
-        std::unique_ptr<OwnedCompilationMessages> compilationMessages;
     };
 
     MaybeError ValidateShaderModuleDescriptor(DeviceBase* device,
                                               const ShaderModuleDescriptor* descriptor,
-                                              ShaderModuleParseResult* parseResult);
+                                              ShaderModuleParseResult* parseResult,
+                                              OwnedCompilationMessages* outMessages);
     MaybeError ValidateCompatibilityWithPipelineLayout(DeviceBase* device,
                                                        const EntryPointMetadata& entryPoint,
                                                        const PipelineLayoutBase* layout);
@@ -142,9 +142,7 @@ namespace dawn_native {
         ShaderModuleBase(DeviceBase* device, const ShaderModuleDescriptor* descriptor);
         ~ShaderModuleBase() override;
 
-        static ShaderModuleBase* MakeError(
+        static ShaderModuleBase* MakeError(DeviceBase* device);
-            DeviceBase* device,
-            std::unique_ptr<OwnedCompilationMessages> compilationMessages);
 
         // Return true iff the program has an entrypoint called `entryPoint`.
         bool HasEntryPoint(const std::string& entryPoint) const;
@@ -165,6 +163,11 @@ namespace dawn_native {
 
         void APIGetCompilationInfo(wgpu::CompilationInfoCallback callback, void* userdata);
 
+        void InjectCompilationMessages(
+            std::unique_ptr<OwnedCompilationMessages> compilationMessages);
+
+        OwnedCompilationMessages* GetCompilationMessages() const;
+
         ResultOrError<std::vector<uint32_t>> GeneratePullingSpirv(
             const std::vector<uint32_t>& spirv,
             const VertexState& vertexState,
@@ -177,10 +180,6 @@ namespace dawn_native {
             const std::string& entryPoint,
             BindGroupIndex pullingBufferBindingSet) const;
 
-        OwnedCompilationMessages* GetCompilationMessages() {
-            return mCompilationMessages.get();
-        }
-
       protected:
         MaybeError InitializeBase(ShaderModuleParseResult* parseResult);
         static ResultOrError<EntryPointMetadataTable> ReflectShaderUsingSPIRVCross(
@@ -188,9 +187,7 @@ namespace dawn_native {
             const std::vector<uint32_t>& spirv);
 
       private:
-        ShaderModuleBase(DeviceBase* device,
+        ShaderModuleBase(DeviceBase* device, ObjectBase::ErrorTag tag);
-                         ObjectBase::ErrorTag tag,
-                         std::unique_ptr<OwnedCompilationMessages> compilationMessages);
 
         // The original data in the descriptor for caching.
         enum class Type { Undefined, Spirv, Wgsl };

src/dawn_native/vulkan/ShaderModuleVk.cpp

@@ -108,7 +108,8 @@ namespace dawn_native { namespace vulkan {
             tint::Program program;
             DAWN_TRY_ASSIGN(program,
                             RunTransforms(&transformManager, parseResult->tintProgram.get(),
-                                          transformInputs, nullptr, GetCompilationMessages()));
+                                          transformInputs, nullptr, nullptr));
+            // We will miss the messages generated in this RunTransforms.
 
             tint::writer::spirv::Generator generator(&program);
             if (!generator.Generate()) {
@@ -119,12 +120,11 @@ namespace dawn_native { namespace vulkan {
             spirv = generator.result();
             spirvPtr = &spirv;
 
-            ShaderModuleParseResult transformedParseResult;
+            // Rather than use a new ParseResult object, we just reuse the original parseResult
-            transformedParseResult.tintProgram =
+            parseResult->tintProgram = std::make_unique<tint::Program>(std::move(program));
-                std::make_unique<tint::Program>(std::move(program));
+            parseResult->spirv = spirv;
-            transformedParseResult.spirv = spirv;
 
-            DAWN_TRY(InitializeBase(&transformedParseResult));
+            DAWN_TRY(InitializeBase(parseResult));
         } else {
             DAWN_TRY(InitializeBase(parseResult));
             spirvPtr = &GetSpirv();

src/tests/unittests/validation/ShaderModuleValidationTests.cpp

@@ -171,9 +171,9 @@ TEST_F(ShaderModuleValidationTest, GetCompilationMessages) {
         reinterpret_cast<dawn_native::ShaderModuleBase*>(shaderModule.Get());
     dawn_native::OwnedCompilationMessages* messages = shaderModuleBase->GetCompilationMessages();
     messages->ClearMessages();
-    messages->AddMessage("Info Message");
+    messages->AddMessageForTesting("Info Message");
-    messages->AddMessage("Warning Message", wgpu::CompilationMessageType::Warning);
+    messages->AddMessageForTesting("Warning Message", wgpu::CompilationMessageType::Warning);
-    messages->AddMessage("Error Message", wgpu::CompilationMessageType::Error, 3, 4);
+    messages->AddMessageForTesting("Error Message", wgpu::CompilationMessageType::Error, 3, 4);
 
     auto callback = [](WGPUCompilationInfoRequestStatus status, const WGPUCompilationInfo* info,
                        void* userdata) {