From e8bd26e1f83580fd483808f2a95ec36f4bceb787 Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Thu, 28 Jan 2021 12:57:35 -0500 Subject: [PATCH] Set correct scope and line info for DebugValue (#4125) The existing spirv-opt `DebugInfoManager::AddDebugValueForDecl()` sets the scope and line info of the new added DebugValue using the scope and line of DebugDeclare. This is wrong because only a single DebugDeclare must exist under a scope while we have to add DebugValue for all the places where the variable's value is updated. Therefore, we have to set the scope and line of DebugValue based on the places of the variable updates. This bug makes https://github.com/google/amber/blob/main/tests/cases/debugger_hlsl_shadowed_vars.amber fail. This commit fixes the bug. --- source/opt/debug_info_manager.cpp | 8 ++- source/opt/debug_info_manager.h | 11 +-- source/opt/local_single_store_elim_pass.cpp | 2 +- source/opt/scalar_replacement_pass.cpp | 3 +- source/opt/ssa_rewrite_pass.cpp | 9 +-- test/opt/debug_info_manager_test.cpp | 77 +++++++++++++++++++++ test/opt/local_single_store_elim_test.cpp | 10 +-- test/opt/local_ssa_elim_test.cpp | 14 ++++ 8 files changed, 117 insertions(+), 17 deletions(-) diff --git a/source/opt/debug_info_manager.cpp b/source/opt/debug_info_manager.cpp index 11b5131d..e782641f 100644 --- a/source/opt/debug_info_manager.cpp +++ b/source/opt/debug_info_manager.cpp @@ -500,14 +500,15 @@ bool DebugInfoManager::AddDebugValueIfVarDeclIsVisible( insert_before->opcode() == SpvOpVariable) { insert_before = insert_before->NextNode(); } - modified |= AddDebugValueForDecl(dbg_decl_or_val, value_id, - insert_before) != nullptr; + modified |= AddDebugValueForDecl(dbg_decl_or_val, value_id, insert_before, + scope_and_line) != nullptr; } return modified; } Instruction* DebugInfoManager::AddDebugValueForDecl( - Instruction* dbg_decl, uint32_t value_id, Instruction* insert_before) { + Instruction* dbg_decl, uint32_t value_id, Instruction* insert_before, + Instruction* scope_and_line) { if (dbg_decl == nullptr || !IsDebugDeclare(dbg_decl)) return nullptr; std::unique_ptr dbg_val(dbg_decl->Clone(context())); @@ -517,6 +518,7 @@ Instruction* DebugInfoManager::AddDebugValueForDecl( dbg_val->SetOperand(kDebugDeclareOperandVariableIndex, {value_id}); dbg_val->SetOperand(kDebugValueOperandExpressionIndex, {GetEmptyDebugExpression()->result_id()}); + dbg_val->UpdateDebugInfoFrom(scope_and_line); auto* added_dbg_val = insert_before->InsertBefore(std::move(dbg_val)); AnalyzeDebugInst(added_dbg_val); diff --git a/source/opt/debug_info_manager.h b/source/opt/debug_info_manager.h index 92b38183..776e9baa 100644 --- a/source/opt/debug_info_manager.h +++ b/source/opt/debug_info_manager.h @@ -152,11 +152,14 @@ class DebugInfoManager { std::unordered_set* invisible_decls); // Creates a DebugValue for DebugDeclare |dbg_decl| and inserts it before - // |insert_before|. The new DebugValue has the same line, scope, and - // operands with DebugDeclare but it uses |value_id| for value. Returns - // the added DebugValue, or nullptr if it does not add a DebugValue. + // |insert_before|. The new DebugValue has the same line and scope as + // |scope_and_line|, or no scope and line information if |scope_and_line| + // is nullptr. The new DebugValue has the same operands as DebugDeclare + // but it uses |value_id| for the value. Returns the created DebugValue, + // or nullptr if fails to create one. Instruction* AddDebugValueForDecl(Instruction* dbg_decl, uint32_t value_id, - Instruction* insert_before); + Instruction* insert_before, + Instruction* scope_and_line); // Erases |instr| from data structures of this class. void ClearDebugInfo(Instruction* instr); diff --git a/source/opt/local_single_store_elim_pass.cpp b/source/opt/local_single_store_elim_pass.cpp index b8d9091a..d322a2fb 100644 --- a/source/opt/local_single_store_elim_pass.cpp +++ b/source/opt/local_single_store_elim_pass.cpp @@ -175,7 +175,7 @@ bool LocalSingleStoreElimPass::RewriteDebugDeclares(Instruction* store_inst, for (auto* decl : invisible_decls) { if (dominator_analysis->Dominates(store_inst, decl)) { context()->get_debug_info_mgr()->AddDebugValueForDecl(decl, value_id, - decl); + decl, store_inst); modified = true; } } diff --git a/source/opt/scalar_replacement_pass.cpp b/source/opt/scalar_replacement_pass.cpp index c8e0da5b..ba2d0675 100644 --- a/source/opt/scalar_replacement_pass.cpp +++ b/source/opt/scalar_replacement_pass.cpp @@ -175,7 +175,8 @@ bool ScalarReplacementPass::ReplaceWholeDebugDeclare( Instruction* added_dbg_value = context()->get_debug_info_mgr()->AddDebugValueForDecl( dbg_decl, /*value_id=*/var->result_id(), - /*insert_before=*/var->NextNode()); + /*insert_before=*/var->NextNode(), /*scope_and_line=*/dbg_decl); + if (added_dbg_value == nullptr) return false; added_dbg_value->AddOperand( {SPV_OPERAND_TYPE_ID, diff --git a/source/opt/ssa_rewrite_pass.cpp b/source/opt/ssa_rewrite_pass.cpp index 0489f03a..81770d77 100644 --- a/source/opt/ssa_rewrite_pass.cpp +++ b/source/opt/ssa_rewrite_pass.cpp @@ -658,8 +658,8 @@ Pass::Status SSARewriter::AddDebugValuesForInvisibleDebugDecls(Function* fp) { // a = 3; // foo(3); // After inlining: - // a = 3; // we want to specify "DebugValue: %x = %int_3" - // foo and x disappeared! + // a = 3; + // foo and x disappeared but we want to specify "DebugValue: %x = %int_3". // // We want to specify the value for the variable using |defs_at_block_[bb]|, // where |bb| is the basic block contains the decl. @@ -681,16 +681,17 @@ Pass::Status SSARewriter::AddDebugValuesForInvisibleDebugDecls(Function* fp) { if (value && (pass_->context()->get_instr_block(value) == nullptr || dom_tree->Dominates(value, decl))) { if (pass_->context()->get_debug_info_mgr()->AddDebugValueForDecl( - decl, value->result_id(), decl) == nullptr) { + decl, value->result_id(), decl, value) == nullptr) { return Pass::Status::Failure; } } else { // If |value| in the same basic block does not dominate |decl|, we can // assign the value in the immediate dominator. value_id = GetValueAtBlock(var_id, dom_tree->ImmediateDominator(bb)); + if (value_id) value = pass_->get_def_use_mgr()->GetDef(value_id); if (value_id && pass_->context()->get_debug_info_mgr()->AddDebugValueForDecl( - decl, value_id, decl) == nullptr) { + decl, value_id, decl, value) == nullptr) { return Pass::Status::Failure; } } diff --git a/test/opt/debug_info_manager_test.cpp b/test/opt/debug_info_manager_test.cpp index 911331a7..49407fd5 100644 --- a/test/opt/debug_info_manager_test.cpp +++ b/test/opt/debug_info_manager_test.cpp @@ -31,6 +31,9 @@ static const uint32_t kDebugFunctionOperandFunctionIndex = 13; static const uint32_t kDebugInlinedAtOperandLineIndex = 4; static const uint32_t kDebugInlinedAtOperandScopeIndex = 5; static const uint32_t kDebugInlinedAtOperandInlinedIndex = 6; +static const uint32_t kOpLineInOperandFileIndex = 0; +static const uint32_t kOpLineInOperandLineIndex = 1; +static const uint32_t kOpLineInOperandColumnIndex = 2; namespace spvtools { namespace opt { @@ -609,6 +612,80 @@ void main(float in_var_color : COLOR) { EXPECT_FALSE(dbg_info_mgr->IsVariableDebugDeclared(100)); } +TEST(DebugInfoManager, AddDebugValueForDecl) { + const std::string text = R"( + OpCapability Shader + %1 = OpExtInstImport "OpenCL.DebugInfo.100" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %in_var_COLOR + OpExecutionMode %main OriginUpperLeft + %5 = OpString "ps.hlsl" + %14 = OpString "#line 1 \"ps.hlsl\" +void main(float in_var_color : COLOR) { + float color = in_var_color; +} +" + %17 = OpString "float" + %21 = OpString "main" + %24 = OpString "color" + OpName %in_var_COLOR "in.var.COLOR" + OpName %main "main" + OpDecorate %in_var_COLOR Location 0 + %uint = OpTypeInt 32 0 + %uint_32 = OpConstant %uint 32 + %float = OpTypeFloat 32 +%_ptr_Input_float = OpTypePointer Input %float + %void = OpTypeVoid + %27 = OpTypeFunction %void +%_ptr_Function_float = OpTypePointer Function %float +%in_var_COLOR = OpVariable %_ptr_Input_float Input + %13 = OpExtInst %void %1 DebugExpression + %15 = OpExtInst %void %1 DebugSource %5 %14 + %16 = OpExtInst %void %1 DebugCompilationUnit 1 4 %15 HLSL + %18 = OpExtInst %void %1 DebugTypeBasic %17 %uint_32 Float + %20 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %18 %18 + %22 = OpExtInst %void %1 DebugFunction %21 %20 %15 1 1 %16 %21 FlagIsProtected|FlagIsPrivate 1 %main + %12 = OpExtInst %void %1 DebugInfoNone + %25 = OpExtInst %void %1 DebugLocalVariable %24 %18 %15 1 20 %22 FlagIsLocal 0 + %main = OpFunction %void None %27 + %28 = OpLabel + %100 = OpVariable %_ptr_Function_float Function + %31 = OpLoad %float %in_var_COLOR + %101 = OpExtInst %void %1 DebugScope %22 + OpLine %5 13 7 + OpStore %100 %31 + OpNoLine + %102 = OpExtInst %void %1 DebugNoScope + %36 = OpExtInst %void %1 DebugDeclare %25 %100 %13 + OpReturn + OpFunctionEnd + )"; + + std::unique_ptr context = + BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text, + SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); + auto* def_use_mgr = context->get_def_use_mgr(); + auto* dbg_decl = def_use_mgr->GetDef(36); + EXPECT_EQ(dbg_decl->GetOpenCL100DebugOpcode(), + OpenCLDebugInfo100DebugDeclare); + + auto* dbg_info_mgr = context->get_debug_info_mgr(); + Instruction* store = dbg_decl->PreviousNode(); + auto* dbg_value = + dbg_info_mgr->AddDebugValueForDecl(dbg_decl, 100, dbg_decl, store); + + EXPECT_EQ(dbg_value->GetOpenCL100DebugOpcode(), OpenCLDebugInfo100DebugValue); + EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand( + kOpLineInOperandFileIndex), + 5); + EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand( + kOpLineInOperandLineIndex), + 13); + EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand( + kOpLineInOperandColumnIndex), + 7); +} + } // namespace } // namespace analysis } // namespace opt diff --git a/test/opt/local_single_store_elim_test.cpp b/test/opt/local_single_store_elim_test.cpp index d015dfba..c94ff372 100644 --- a/test/opt/local_single_store_elim_test.cpp +++ b/test/opt/local_single_store_elim_test.cpp @@ -1403,18 +1403,20 @@ TEST_F(LocalSingleStoreElimTest, AddDebugValueforStoreOutOfDebugDeclareScope) { %param_var_pos = OpVariable %_ptr_Function_v4float Function %param_var_color = OpVariable %_ptr_Function_v4float Function %55 = OpLoad %v4float %in_var_POSITION + OpLine %7 6 23 OpStore %param_var_pos %55 + OpNoLine %56 = OpLoad %v4float %in_var_COLOR ;CHECK: DebugNoScope ;CHECK-NOT: OpLine ;CHECK: [[pos:%\w+]] = OpLoad %v4float %in_var_POSITION ;CHECK: [[color:%\w+]] = OpLoad %v4float %in_var_COLOR - OpStore %param_var_color %56 - %93 = OpExtInst %void %1 DebugScope %48 - OpLine %7 6 23 - %73 = OpExtInst %void %1 DebugDeclare %53 %param_var_pos %52 OpLine %7 7 23 + OpStore %param_var_color %56 + OpNoLine + %93 = OpExtInst %void %1 DebugScope %48 + %73 = OpExtInst %void %1 DebugDeclare %53 %param_var_pos %52 %74 = OpExtInst %void %1 DebugDeclare %51 %param_var_color %52 ;CHECK: OpLine [[file:%\w+]] 6 23 ;CHECK-NEXT: {{%\w+}} = OpExtInst %void {{%\w+}} DebugValue [[dbg_pos]] [[pos]] [[empty_expr:%\w+]] diff --git a/test/opt/local_ssa_elim_test.cpp b/test/opt/local_ssa_elim_test.cpp index 9baf8da8..ca9aba33 100644 --- a/test/opt/local_ssa_elim_test.cpp +++ b/test/opt/local_ssa_elim_test.cpp @@ -2261,9 +2261,13 @@ TEST_F(LocalSSAElimTest, AddDebugValueForFunctionParameterWithPhi) { %69 = OpExtInst %void %1 DebugLexicalBlock %60 7 21 %68 %70 = OpExtInst %void %1 DebugLocalVariable %11 %59 %60 6 26 %67 FlagIsLocal 2 +; CHECK: [[color_name:%\w+]] = OpString "color" +; CHECK: [[pos_name:%\w+]] = OpString "pos" ; CHECK: [[i_name:%\w+]] = OpString "i" ; CHECK: [[null_expr:%\w+]] = OpExtInst %void [[ext:%\w+]] DebugExpression ; CHECK: [[dbg_i:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[i_name]] {{%\w+}} {{%\w+}} 6 16 {{%\w+}} FlagIsLocal 1 +; CHECK: [[dbg_color:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[color_name]] {{%\w+}} {{%\w+}} 15 23 +; CHECK: [[dbg_pos:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[pos_name]] {{%\w+}} {{%\w+}} 14 23 %71 = OpExtInst %void %1 DebugLocalVariable %15 %65 %60 6 16 %67 FlagIsLocal 1 %72 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %62 %59 %59 %73 = OpExtInst %void %1 DebugFunction %16 %72 %60 14 1 %61 %14 FlagIsProtected|FlagIsPrivate 15 %156 @@ -2282,13 +2286,23 @@ TEST_F(LocalSSAElimTest, AddDebugValueForFunctionParameterWithPhi) { %169 = OpExtInst %void %1 DebugNoScope %param_var_pos = OpVariable %_ptr_Function_v4float Function %param_var_color = OpVariable %_ptr_Function_v4float Function + OpLine %7 100 105 %80 = OpLoad %v4float %in_var_POSITION OpStore %param_var_pos %80 + OpNoLine + OpLine %7 200 205 %81 = OpLoad %v4float %in_var_COLOR OpStore %param_var_color %81 + OpNoLine %170 = OpExtInst %void %1 DebugScope %73 + +; CHECK: OpLine {{%\w+}} 100 105 +; CHECK: DebugValue [[dbg_pos]] %124 = OpExtInst %void %1 DebugDeclare %78 %param_var_pos %77 +; CHECK: OpLine {{%\w+}} 200 205 +; CHECK: DebugValue [[dbg_color]] %125 = OpExtInst %void %1 DebugDeclare %76 %param_var_color %77 + %171 = OpExtInst %void %1 DebugScope %74 OpLine %7 17 18