Correct metadata load for NodeRecordType for robustness (microsoft#6277)

Metadata loading for node record properties assumed fixed positions for
values in a (key,value) property list, skipping and ignoring the keys.

This was not correct, fragile, and not the intended implementation of
the key,value property pattern.

There is no functional change, but this makes the loader robust to
additional properties instead of crashing or loading the wrong thing.

Fixes microsoft#6276

Author: tex3d

include/dxc/DXIL/DxilMetadataHelper.h

@@ -625,6 +625,7 @@ class DxilMDHelper {
 
   llvm::MDTuple *EmitDxilNodeIOState(const NodeIOProperties &Node);
   hlsl::NodeIOProperties LoadDxilNodeIOState(const llvm::MDOperand &MDO);
+  hlsl::NodeRecordType LoadDxilNodeRecordType(const llvm::MDOperand &MDO);
 
   void EmitDxilNodeState(std::vector<llvm::Metadata *> &MDVals,
                          const DxilFunctionProps &props);

lib/DXIL/DxilMetadataHelper.cpp

@@ -2824,6 +2824,46 @@ DxilMDHelper::EmitDxilNodeIOState(const hlsl::NodeIOProperties &Node) {
   return MDNode::get(m_Ctx, MDVals);
 }
 
+NodeRecordType
+DxilMDHelper::LoadDxilNodeRecordType(const llvm::MDOperand &MDO) {
+  const MDTuple *pTupleMD = dyn_cast<MDTuple>(MDO.get());
+  IFTBOOL(pTupleMD != nullptr, DXC_E_INCORRECT_DXIL_METADATA);
+  IFTBOOL((pTupleMD->getNumOperands() & 0x1) == 0,
+          DXC_E_INCORRECT_DXIL_METADATA);
+
+  NodeRecordType Record = {};
+  for (unsigned iNode = 0; iNode < pTupleMD->getNumOperands(); iNode += 2) {
+    unsigned Tag = DxilMDHelper::ConstMDToUint32(pTupleMD->getOperand(iNode));
+    const MDOperand &MDO = pTupleMD->getOperand(iNode + 1);
+    IFTBOOL(MDO.get() != nullptr, DXC_E_INCORRECT_DXIL_METADATA);
+
+    switch (Tag) {
+    case DxilMDHelper::kDxilNodeRecordSizeTag: {
+      Record.size = ConstMDToUint32(MDO);
+    } break;
+    case DxilMDHelper::kDxilNodeSVDispatchGridTag: {
+      MDTuple *pSVDTupleMD = cast<MDTuple>(MDO.get());
+      // < 3 if fatal
+      IFTBOOL(pSVDTupleMD->getNumOperands() >= 3,
+              DXC_E_INCORRECT_DXIL_METADATA);
+      // > 3 is extra metadata, validator will fail.
+      if (pSVDTupleMD->getNumOperands() > 3)
+        m_bExtraMetadata = true;
+      Record.SV_DispatchGrid.ByteOffset =
+          ConstMDToUint32(pSVDTupleMD->getOperand(0));
+      Record.SV_DispatchGrid.ComponentType = static_cast<DXIL::ComponentType>(
+          ConstMDToUint32(pSVDTupleMD->getOperand(1)));
+      Record.SV_DispatchGrid.NumComponents =
+          ConstMDToUint32(pSVDTupleMD->getOperand(2));
+    } break;
+    default:
+      m_bExtraMetadata = true;
+      break;
+    }
+  }
+  return Record;
+}
+
 NodeIOProperties DxilMDHelper::LoadDxilNodeIOState(const llvm::MDOperand &MDO) {
   const MDTuple *pTupleMD = dyn_cast<MDTuple>(MDO.get());
   IFTBOOL(pTupleMD != nullptr, DXC_E_INCORRECT_DXIL_METADATA);
@@ -2841,20 +2881,7 @@ NodeIOProperties DxilMDHelper::LoadDxilNodeIOState(const llvm::MDOperand &MDO) {
       Node.Flags = NodeFlags(ConstMDToUint32(MDO));
     } break;
     case DxilMDHelper::kDxilNodeRecordTypeTag: {
-      MDTuple *pTupleMD = cast<MDTuple>(MDO.get());
-      Node.RecordType.size = ConstMDToUint32(pTupleMD->getOperand(1));
-      if (pTupleMD->getNumOperands() > 2) {
-        DXASSERT(pTupleMD->getNumOperands() == 4,
-                 "incorrect number of operands");
-        MDTuple *pSVDTupleMD = cast<MDTuple>(pTupleMD->getOperand(3));
-        Node.RecordType.SV_DispatchGrid.ByteOffset =
-            ConstMDToUint32(pSVDTupleMD->getOperand(0));
-        Node.RecordType.SV_DispatchGrid.ComponentType =
-            static_cast<DXIL::ComponentType>(
-                ConstMDToUint32(pSVDTupleMD->getOperand(1)));
-        Node.RecordType.SV_DispatchGrid.NumComponents =
-            ConstMDToUint32(pSVDTupleMD->getOperand(2));
-      }
+      Node.RecordType = LoadDxilNodeRecordType(MDO);
     } break;
     case DxilMDHelper::kDxilNodeOutputArraySizeTag: {
       Node.OutputArraySize = ConstMDToUint32(MDO);

tools/clang/test/HLSLFileCheck/hlsl/workgraph/record_type_metadata.ll

@@ -0,0 +1,46 @@
+; RUN: %dxilver 1.8 | %D3DReflect %s | FileCheck %s -check-prefixes=RDAT
+
+; Make sure NodeRecordType metdata loading is robust to tag,value list ordering.
+
+; RDAT: FunctionTable[{{.*}}] = {
+; RDAT-LABEL: UnmangledName: "node01"
+; RDAT: Inputs: <11:RecordArrayRef<IONode>[1]>  = {
+; RDAT: AttribKind: RecordSizeInBytes
+; RDAT-NEXT: RecordSizeInBytes: 12
+; RDAT: AttribKind: RecordDispatchGrid
+; RDAT-NEXT: RecordDispatchGrid: <RecordDispatchGrid>
+; RDAT-NEXT: ByteOffset: 0
+; RDAT-NEXT: ComponentNumAndType: 23
+; RDAT-LABEL: RecordTable
+
+target datalayout = "e-m:e-p:32:32-i1:32-i8:32-i16:32-i32:32-i64:64-f16:32-f32:32-f64:64-n8:16:32:64"
+target triple = "dxil-ms-dx"
+
+define void @node01() {
+  ret void
+}
+
+!llvm.ident = !{!0}
+!dx.version = !{!1}
+!dx.valver = !{!1}
+!dx.shaderModel = !{!2}
+!dx.typeAnnotations = !{!3}
+!dx.entryPoints = !{!7, !8}
+
+!0 = !{!"dxc(private) 1.8.0.4454 (rdat-dump-flags, c997ea026-dirty)"}
+!1 = !{i32 1, i32 8}
+!2 = !{!"lib", i32 6, i32 8}
+!3 = !{i32 1, void ()* @node01, !4}
+!4 = !{!5}
+!5 = !{i32 0, !6, !6}
+!6 = !{}
+!7 = !{null, !"", null, null, null}
+!8 = !{void ()* @node01, !"node01", null, null, !9}
+!9 = !{i32 8, i32 15, i32 13, i32 1, i32 15, !10, i32 16, i32 -1, i32 22, !11, i32 20, !12, i32 4, !11, i32 5, !16}
+!10 = !{!"node01", i32 0}
+!11 = !{i32 4, i32 4, i32 4}
+!12 = !{!13}
+!13 = !{i32 1, i32 101, i32 2, !14}
+!14 = !{i32 1, !15, i32 0, i32 12} ; reordered tag,value list entries for NodeRecordType
+!15 = !{i32 0, i32 5, i32 3}
+!16 = !{i32 0}