Handle decoration groups with no decorations. (#1921)

In DecorationManager::RemoveDecorationsFrom, we do not remove the id
from a decoration group if the group has no decorations.  This causes
problems because KillNamesAndDecorates is suppose to remove all
references to the id, but in this case, there is still a reference.

This is fixed by adding a special case.

Also, there is the possibility of a double free because
RemoveDecorationsFrom will delete the instructions defining |id| when
|id| is a decoration group.  Later, KillInst would later write to memory
that has been deleted when trying to turn it into a Nop.  To fix this,
we will only remove the decorations that use |id| and not its definition
in RemoveDecorationsFrom.

source/opt/decoration_manager.cpp

@@ -29,7 +29,9 @@ namespace analysis {
 void DecorationManager::RemoveDecorationsFrom(
     uint32_t id, std::function<bool(const Instruction&)> pred) {
   const auto ids_iter = id_to_decoration_insts_.find(id);
-  if (ids_iter == id_to_decoration_insts_.end()) return;
+  if (ids_iter == id_to_decoration_insts_.end()) {
+    return;
+  }
 
   TargetData& decorations_info = ids_iter->second;
   auto context = module_->context();
@@ -59,8 +61,14 @@ void DecorationManager::RemoveDecorationsFrom(
       if (!pred(*decoration)) group_decorations_to_keep.push_back(decoration);
     }
 
-    // If all decorations should be kept, move to the next group
-    if (group_decorations_to_keep.size() == group_decorations.size()) continue;
+    // If all decorations should be kept, then we can keep |id| part of the
+    // group.  However, if the group itself has no decorations, we should remove
+    // the id from the group.  This is needed to make |KillNameAndDecorate| work
+    // correctly when a decoration group has no decorations.
+    if (group_decorations_to_keep.size() == group_decorations.size() &&
+        group_decorations.size() != 0) {
+      continue;
+    }
 
     // Otherwise, remove |id| from the targets of |group_id|
     const uint32_t stride = inst->opcode() == SpvOpGroupDecorate ? 1u : 2u;
@@ -136,9 +144,6 @@ void DecorationManager::RemoveDecorationsFrom(
       decorations_info.indirect_decorations.empty() &&
       decorations_info.decorate_insts.empty()) {
     id_to_decoration_insts_.erase(ids_iter);
-
-    // Remove the OpDecorationGroup defining this group.
-    if (is_group) context->KillInst(context->get_def_use_mgr()->GetDef(id));
   }
 }
 

source/opt/decoration_manager.h

@@ -36,11 +36,22 @@ class DecorationManager {
   }
   DecorationManager() = delete;
 
-  // Removes all decorations from |id| (either directly or indirectly) for
-  // which |pred| returns true.
-  // If |id| is a group ID, OpGroupDecorate and OpGroupMemberDecorate will be
-  // removed if they have no targets left, and OpDecorationGroup will be
-  // removed if the group is not applied to anyone and contains no decorations.
+  // Changes all of the decorations (direct and through groups) where |pred| is
+  // true and that apply to |id| so that they no longer apply to |id|.
+  //
+  // If |id| is part of a group, it will be removed from the group if it
+  // does not use all of the group's decorations, or, if there are no
+  // decorations that apply to the group.
+  //
+  // If decoration groups become empty, the |OpGroupDecorate| and
+  // |OpGroupMemberDecorate| instructions will be killed.
+  //
+  // Decoration instructions that apply directly to |id| will be killed.
+  //
+  // If |id| is a decoration group and all of the group's decorations are
+  // removed, then the |OpGroupDecorate| and
+  // |OpGroupMemberDecorate| for the group will be killed, but not the defining
+  // |OpDecorationGroup| instruction.
   void RemoveDecorationsFrom(uint32_t id,
                              std::function<bool(const Instruction&)> pred =
                                  [](const Instruction&) { return true; });

test/opt/decoration_manager_test.cpp

@@ -422,6 +422,7 @@ OpGroupDecorate %2 %1 %3
 OpCapability Linkage
 OpMemoryModel Logical GLSL450
 OpDecorate %1 Constant
+%2 = OpDecorationGroup
 %4 = OpTypeInt 32 0
 %1 = OpVariable %4 Uniform
 %3 = OpVariable %4 Uniform

test/opt/ir_context_test.cpp

@@ -282,6 +282,94 @@ TEST_F(IRContextTest, TakeNextUniqueIdIncrementing) {
     EXPECT_EQ(i, localContext.TakeNextUniqueId());
 }
 
+TEST_F(IRContextTest, KillGroupDecorationWitNoDecorations) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %2 "main"
+               OpExecutionMode %2 OriginUpperLeft
+               OpSource GLSL 430
+          %3 = OpDecorationGroup
+               OpGroupDecorate %3 %4 %5
+          %6 = OpTypeFloat 32
+          %7 = OpTypePointer Function %6
+          %8 = OpTypeStruct %6
+          %9 = OpTypeVoid
+         %10 = OpTypeFunction %9
+          %2 = OpFunction %9 None %10
+         %11 = OpLabel
+          %4 = OpVariable %7 Function
+          %5 = OpVariable %7 Function
+               OpReturn
+               OpFunctionEnd
+)";
+
+  std::unique_ptr<IRContext> context =
+      BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
+
+  // Build the decoration manager.
+  context->get_decoration_mgr();
+
+  // Delete the second variable.
+  context->KillDef(5);
+
+  // The two decoration instructions should still be there.  The first one
+  // should be the same, but the second should have %5 removed.
+
+  // Check the OpDecorationGroup Instruction
+  auto inst = context->annotation_begin();
+  EXPECT_EQ(inst->opcode(), SpvOpDecorationGroup);
+  EXPECT_EQ(inst->result_id(), 3);
+
+  // Check that %5 is no longer part of the group.
+  ++inst;
+  EXPECT_EQ(inst->opcode(), SpvOpGroupDecorate);
+  EXPECT_EQ(inst->NumInOperands(), 2);
+  EXPECT_EQ(inst->GetSingleWordInOperand(0), 3);
+  EXPECT_EQ(inst->GetSingleWordInOperand(1), 4);
+
+  // Check that we are at the end.
+  ++inst;
+  EXPECT_EQ(inst, context->annotation_end());
+}
+
+TEST_F(IRContextTest, KillDecorationGroup) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %2 "main"
+               OpExecutionMode %2 OriginUpperLeft
+               OpSource GLSL 430
+          %3 = OpDecorationGroup
+               OpGroupDecorate %3 %4 %5
+          %6 = OpTypeFloat 32
+          %7 = OpTypePointer Function %6
+          %8 = OpTypeStruct %6
+          %9 = OpTypeVoid
+         %10 = OpTypeFunction %9
+          %2 = OpFunction %9 None %10
+         %11 = OpLabel
+          %4 = OpVariable %7 Function
+          %5 = OpVariable %7 Function
+               OpReturn
+               OpFunctionEnd
+)";
+
+  std::unique_ptr<IRContext> context =
+      BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
+
+  // Build the decoration manager.
+  context->get_decoration_mgr();
+
+  // Delete the second variable.
+  context->KillDef(3);
+
+  // Check the OpDecorationGroup Instruction is still there.
+  EXPECT_TRUE(context->annotations().empty());
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools