Merge pull request #213 from frasercrmck/spirv-ll-fix-more-debug-crashes

[spirv-ll] Fix a couple more crashes in debug info translation

modules/compiler/spirv-ll/include/spirv-ll/builder.h

@@ -330,10 +330,10 @@ class Builder {
 
   /// @brief Return a `DIType` object that represents the given type
   ///
-  /// @param type `Type` to get `DIType` from
+  /// @param tyID spv::Id `Type` to get `DIType` from
   ///
   /// @return pointer to `DIType` derived from the given `Type`
-  llvm::DIType *getDIType(llvm::Type *type);
+  llvm::DIType *getDIType(spv::Id tyID);
 
   /// @brief Gets (or creates) a DIFile for the given OpLine.
   llvm::DIFile *getOrCreateDIFile(const OpLine *op_line);

modules/compiler/spirv-ll/source/builder.cpp

@@ -15,6 +15,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 #include <compiler/utils/target_extension_types.h>
+#include <llvm/BinaryFormat/Dwarf.h>
 #include <llvm/IR/Attributes.h>
 #include <llvm/Support/Debug.h>
 #include <llvm/Support/type_traits.h>
@@ -61,78 +62,69 @@ llvm::Value *spirv_ll::Builder::popFunctionArg() {
   return arg;
 }
 
-llvm::DIType *spirv_ll::Builder::getDIType(llvm::Type *type) {
-  const llvm::DataLayout &datalayout =
+llvm::DIType *spirv_ll::Builder::getDIType(spv::Id tyID) {
+  const llvm::DataLayout &DL =
       IRBuilder.GetInsertBlock()->getModule()->getDataLayout();
 
-  uint32_t align = datalayout.getABITypeAlign(type).value();
+  auto *const llvmTy = module.getLLVMType(tyID);
+  SPIRV_LL_ASSERT_PTR(llvmTy);
+  auto *const opTy = module.get<OpType>(tyID);
+  SPIRV_LL_ASSERT_PTR(opTy);
 
-  uint64_t size = datalayout.getTypeAllocSizeInBits(type);
+  uint32_t align = DL.getABITypeAlign(llvmTy).value();
 
-  std::string name;
-
-  if (type->isAggregateType()) {
-    switch (type->getTypeID()) {
-      case llvm::Type::ArrayTyID: {
-        llvm::DIType *elem_type = getDIType(type->getArrayElementType());
+  uint64_t size = DL.getTypeAllocSizeInBits(llvmTy);
 
-        return DIBuilder.createArrayType(type->getArrayNumElements(), align,
-                                         elem_type, llvm::DINodeArray());
-      }
-      case llvm::Type::StructTyID: {
-        llvm::StructType *struct_type = llvm::cast<llvm::StructType>(type);
+  if (opTy->isArrayType()) {
+    llvm::DIType *elem_type = getDIType(opTy->getTypeArray()->ElementType());
+    return DIBuilder.createArrayType(llvmTy->getArrayNumElements(), align,
+                                     elem_type, llvm::DINodeArray());
+  }
 
-        llvm::SmallVector<llvm::Metadata *, 4> element_types;
+  if (opTy->isVectorType()) {
+    llvm::DIType *elem_type = getDIType(opTy->getTypeVector()->ComponentType());
+    return DIBuilder.createVectorType(opTy->getTypeVector()->ComponentCount(),
+                                      align, elem_type, llvm::DINodeArray());
+  }
 
-        for (uint32_t elem_index = 0;
-             elem_index < struct_type->getNumElements(); elem_index++) {
-          element_types.push_back(
-              getDIType(struct_type->getElementType(elem_index)));
-        }
+  if (opTy->isStructType()) {
+    auto *const opStructTy = opTy->getTypeStruct();
+    llvm::StructType *struct_type = llvm::cast<llvm::StructType>(llvmTy);
 
-        // TODO: track line info for struct definitions, will require further
-        // interface changes so for now just use 0
-        return DIBuilder.createStructType(
-            module.getCompileUnit(), struct_type->getName(), module.getDIFile(),
-            0, size, align, llvm::DINode::FlagZero, nullptr,
-            DIBuilder.getOrCreateArray(element_types));
-      }
-      case llvm::Type::FixedVectorTyID: {
-        llvm::DIType *elem_type =
-            getDIType(multi_llvm::getVectorElementType(type));
+    llvm::SmallVector<llvm::Metadata *, 4> member_types;
 
-        return DIBuilder.createVectorType(
-            multi_llvm::getVectorNumElements(type), align, elem_type,
-            llvm::DINodeArray());
-      }
-      default:
-        llvm_unreachable("unsupported debug type");
-    }
-  } else {
-    switch (type->getTypeID()) {
-      case llvm::Type::IntegerTyID:
-        if (llvm::cast<llvm::IntegerType>(type)->getSignBit()) {
-          name = "dbg_int_ty";
-        } else {
-          name = "dbg_uint_ty";
-        }
-        break;
-      case llvm::Type::FloatTyID:
-        name = "dbg_float_ty";
-        break;
-      case llvm::Type::PointerTyID: {
-        auto *opTy = module.getFromLLVMTy<OpType>(type);
-        SPIRV_LL_ASSERT(opTy && opTy->isPointerType(), "Type is not a pointer");
-        llvm::DIType *elem_type =
-            getDIType(module.getLLVMType(opTy->getTypePointer()->Type()));
-        return DIBuilder.createPointerType(elem_type, size, align);
-      }
-      default:
-        llvm_unreachable("unsupported debug type");
+    for (auto memberTyID : opStructTy->MemberTypes()) {
+      member_types.push_back(getDIType(memberTyID));
     }
+
+    // TODO: track line info for struct definitions, will require further
+    // interface changes so for now just use 0
+    return DIBuilder.createStructType(
+        module.getCompileUnit(), struct_type->getName(), module.getDIFile(),
+        /*LineNumber*/ 0, size, align, llvm::DINode::FlagZero, nullptr,
+        DIBuilder.getOrCreateArray(member_types));
+  }
+
+  if (opTy->isIntType()) {
+    return opTy->getTypeInt()->Signedness()
+               ? DIBuilder.createBasicType("dbg_int_ty", size,
+                                           llvm::dwarf::DW_ATE_signed)
+
+               : DIBuilder.createBasicType("dbg_uint_ty", size,
+                                           llvm::dwarf::DW_ATE_unsigned);
+  }
+
+  if (opTy->isFloatType()) {
+    return DIBuilder.createBasicType("dbg_float_ty", size,
+                                     llvm::dwarf::DW_ATE_float);
+  }
+
+  if (opTy->isPointerType()) {
+    llvm::DIType *elem_type = getDIType(opTy->getTypePointer()->Type());
+    return DIBuilder.createPointerType(elem_type, size, align);
   }
 
-  return DIBuilder.createBasicType(name, size, align);
+  llvm_unreachable("unsupported debug type");
 }
 
 void spirv_ll::Builder::addDebugInfoToModule() {

modules/compiler/spirv-ll/source/builder_core.cpp

@@ -167,14 +167,18 @@ llvm::DISubprogram *Builder::getOrCreateDebugFunctionScope(
     return function_scope;
   }
 
-  llvm::SmallVector<llvm::Metadata *, 4> dbg_function_types;
+  llvm::SmallVector<llvm::Metadata *, 4> dbg_function_param_types;
 
-  for (auto &arg : function->args()) {
-    dbg_function_types.push_back(getDIType(arg.getType()));
+  const OpFunction *opFunction = module.get<OpFunction>(function);
+  const OpTypeFunction *opTypeFunction =
+      module.get<OpTypeFunction>(opFunction->FunctionType());
+
+  for (auto spv_ty_id : opTypeFunction->ParameterTypes()) {
+    dbg_function_param_types.push_back(getDIType(spv_ty_id));
   }
 
   llvm::DISubroutineType *dbg_function_type = DIBuilder.createSubroutineType(
-      DIBuilder.getOrCreateTypeArray(dbg_function_types));
+      DIBuilder.getOrCreateTypeArray(dbg_function_param_types));
 
   auto *di_file = getOrCreateDIFile(op_line);
   auto *di_compile_unit = getOrCreateDICompileUnit(op_line);
@@ -2085,14 +2089,17 @@ cargo::optional<Error> Builder::create<OpFunction>(const OpFunction *op) {
   function->setCallingConv(CC);
   setCurrentFunction(function);
 
+  // Add the ID before calling getOrCreateDebugFunctionScope below, so we can
+  // easily retrieve the OpFunction directly from the function.
+  module.addID(op->IdResult(), op, function);
+
   // If there's a line range currently open at this point, create and attach
   // the DISubprogram for this function. If there isn't, we'll generate one on
   // the fly when we hit an OpLine.
   if (auto current_range = module.getCurrentOpLineRange()) {
     getOrCreateDebugFunctionScope(function, current_range->op_line);
   }
 
-  module.addID(op->IdResult(), op, function);
   return cargo::nullopt;
 }
 

modules/compiler/spirv-ll/source/context.cpp

@@ -1117,7 +1117,12 @@ cargo::expected<spirv_ll::Module, spirv_ll::Error> spirv_ll::Context::translate(
     }
   }
 
-  // replace all global builtin vars with function local versions
+  // Add debug info, before we start replacing global builtin vars; the
+  // instruction ranges we've recorded are on the current state of the basic
+  // blocks. Replacing the global builtins will invalidate the iterators.
+  builder.addDebugInfoToModule();
+
+  // Replace all global builtin vars with function local versions
   builder.replaceBuiltinGlobals();
 
   // Check non-entry-point functions with empty names and re-set the name if it
@@ -1136,9 +1141,8 @@ cargo::expected<spirv_ll::Module, spirv_ll::Error> spirv_ll::Context::translate(
     }
   }
 
-  // add any remaining metadata to llvm module
+  // Add any remaining metadata to llvm module
   builder.finalizeMetadata();
-  builder.addDebugInfoToModule();
 
 #if defined(__GNUC__) && !defined(__clang__) && __GNUC__ < 9
   // GCC <9 requires this redundant move, this branch of the #if can be

modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt

@@ -22,6 +22,8 @@ endif()
 
 set(SPVASM_FILES
   debug_info.spvasm
+  debug_info_params.spvasm
+  debug_info_builtins.spvasm
   debug_info_funcs_and_blocks.spvasm
   ext_variable_pointers_op_variable.spvasm
   ext_variable_pointers_function_call.spvasm

modules/compiler/spirv-ll/test/spvasm/debug_info_builtins.spvasm

@@ -0,0 +1,66 @@
+; Copyright (C) Codeplay Software Limited
+;
+; Licensed under the Apache License, Version 2.0 (the "License") with LLVM
+; Exceptions; you may not use this file except in compliance with the License.
+; You may obtain a copy of the License at
+;
+;     https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt
+;
+; Unless required by applicable law or agreed to in writing, software
+; distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+; WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+; License for the specific language governing permissions and limitations
+; under the License.
+;
+; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+; RUN: %if online-spirv-as %{ spirv-as --target-env %spv_tgt_env -o %spv_file_s %s %}
+; RUN: %if online-spirv-as %{ spirv-val %spv_file_s %}
+; RUN: spirv-ll-tool -a OpenCL -b 64 -c Int64 %spv_file_s | FileCheck %s
+                 OpCapability Addresses
+                 OpCapability Linkage
+                 OpCapability Kernel
+                 OpCapability Int64
+                 OpMemoryModel Physical64 OpenCL
+
+         %file = OpString "spvasm/debug_info_builtins.comp"
+
+                 OpDecorate %giid BuiltIn GlobalInvocationId
+                 OpDecorate %giid LinkageAttributes "giid" Import
+
+         %void = OpTypeVoid
+        %ulong = OpTypeInt 64 0
+      %v3ulong = OpTypeVector %ulong 3
+ %_ptr_v3ulong = OpTypePointer Input %v3ulong
+    %ptr_ulong = OpTypePointer CrossWorkgroup %ulong
+         %fnty = OpTypeFunction %void %ptr_ulong
+
+                 OpLine %file 4 0
+         %giid = OpVariable %_ptr_v3ulong Input
+                 OpNoLine
+
+           %fn = OpFunction %void None %fnty
+    %arg_inout = OpFunctionParameter %ptr_ulong
+        %entry = OpLabel
+
+; Check that we correctly get the line for the builtin access (line 5)
+; CHECK: = call spir_func i64 @_Z13get_global_idj(i32 {{%.*}}), !dbg [[gidLoc:![0-9]+]]
+                 OpLine %file 5 0
+           %26 = OpLoad %v3ulong %giid
+           %24 = OpCompositeExtract %ulong %26 0
+                 OpNoLine
+
+; CHECK: = getelementptr inbounds i64, ptr addrspace(1) {{%.*}}, !dbg [[nextLoc:![0-9]+]]
+; CHECK: = load i64, ptr addrspace(1) {{%.*}}, align 8, !dbg [[nextLoc]]
+; CHECK: store i64 {{%.*}}, ptr addrspace(1) {{%.*}}, align 8, !dbg [[nextLoc]]
+                 OpLine %file 6 0
+  %inout_ptr_i = OpInBoundsPtrAccessChain %ptr_ulong %arg_inout %24
+           %38 = OpLoad %ulong %inout_ptr_i Aligned 8
+                 OpStore %arg_inout %38 Aligned 8
+
+                 OpReturn
+                 OpFunctionEnd
+
+; CHECK: [[gidLoc]] = !DILocation(line: 5, scope: [[mainLexicalBlock:![0-9]+]])
+; CHECK: [[mainLexicalBlock]] = distinct !DILexicalBlock(scope: {{![0-9]+}}, file: {{![0-9]+}}, line: 5)
+; CHECK: [[nextLoc]] = !DILocation(line: 6, scope: [[mainLexicalBlock]])

modules/compiler/spirv-ll/test/spvasm/debug_info_params.spvasm

@@ -0,0 +1,76 @@
+; Copyright (C) Codeplay Software Limited
+;
+; Licensed under the Apache License, Version 2.0 (the "License") with LLVM
+; Exceptions; you may not use this file except in compliance with the License.
+; You may obtain a copy of the License at
+;
+;     https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt
+;
+; Unless required by applicable law or agreed to in writing, software
+; distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+; WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+; License for the specific language governing permissions and limitations
+; under the License.
+;
+; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+; RUN: %if online-spirv-as %{ spirv-as --target-env %spv_tgt_env -o %spv_file_s %s %}
+; RUN: spirv-ll-tool -a Vulkan -b 64 -c Linkage %spv_file_s | FileCheck %s
+
+; CHECK: ; ModuleID = '{{.*}}'
+               OpCapability Shader
+               OpCapability Linkage
+          %1 = OpExtInstImport "OpenCL.std"
+               OpMemoryModel Logical GLSL450
+               OpSource OpenCL_C 102000
+
+       %file = OpString "test/spvasm/debug_info_params.comp"
+
+               OpName %my_kernel "my_kernel"
+               OpName %a "a"
+               OpName %b "b"
+               OpName %c "c"
+               OpName %d "d"
+               OpName %file "file"
+       %void = OpTypeVoid
+     %int_ty = OpTypeInt 32 1
+    %uint_ty = OpTypeInt 32 0
+   %float_ty = OpTypeFloat 32
+  %float_pty = OpTypePointer CrossWorkgroup %float_ty
+       %bool = OpTypeBool
+          %3 = OpTypeFunction %void %int_ty %uint_ty %float_ty %float_pty
+
+; CHECK: define private spir_func void @my_kernel(
+; CHECK-SAME:     i32 %a, i32 %b, float %c, ptr addrspace(1) %d)
+; CHECK-SAME: !dbg [[MainSubprogram:![0-9]+]]
+       %my_kernel = OpFunction %void None %3
+          %a = OpFunctionParameter %int_ty
+          %b = OpFunctionParameter %uint_ty
+          %c = OpFunctionParameter %float_ty
+          %d = OpFunctionParameter %float_pty
+          %5 = OpLabel
+               OpLine %file 6 3
+               OpNoLine
+               OpReturn
+               OpFunctionEnd
+
+; CHECK: !llvm.dbg.cu = !{[[CompileUnit:![0-9]+]]}
+
+; CHECK: [[CompileUnit]] = distinct !DICompileUnit(language: DW_LANG_OpenCL, file: [[File:![0-9]+]], isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+; CHECK: [[File]] = !DIFile(filename: "debug_info_params.comp", directory: "test/spvasm")
+
+; CHECK: [[MainSubprogram]] = distinct !DISubprogram(scope: null, file: [[File]],
+; CHECK-SAME: line: 6, type: [[MainSubroutineType:![0-9]+]]
+; CHECK-SAME: {{(, isLocal: true, isDefinition: true)?}}, scopeLine: 1
+; CHECK-SAME: {{(, isOptimized: false)?}}
+; CHECK-SAME: {{(, spFlags: DISPFlagDefinition)?}}
+; CHECK-SAME: , unit: [[CompileUnit]]
+; CHECK-SAME: {{(, (variables|retainedNodes): ![0-9]+)?}}
+; CHECK-SAME: )
+
+; CHECK: [[MainSubroutineType]] = !DISubroutineType(types: [[MainSubroutineParamTypes:![0-9]+]])
+; CHECK: [[MainSubroutineParamTypes]] = !{[[A:![0-9]+]], [[B:![0-9]+]], [[C:![0-9]+]], [[D:![0-9]+]]}
+; CHECK: [[A]] = !DIBasicType(name: "dbg_int_ty", size: 32, encoding: DW_ATE_signed)
+; CHECK: [[B]] = !DIBasicType(name: "dbg_uint_ty", size: 32, encoding: DW_ATE_unsigned)
+; CHECK: [[C]] = !DIBasicType(name: "dbg_float_ty", size: 32, encoding: DW_ATE_float)
+; CHECK: [[D]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: [[C]], size: 64, align: 8)