Handle unknown lines in DebugInfo (#4252)

Mainly, changes the default from -1 to 0 in DiagnosticLoc, still trying to keep reusing that. Nothing except for the lowered output is affected, so I think this is fine. Also, have lowering consistently call GetDiagnosticLoc. Pulls in one of the CHECKs suggested from #4251 Co-authored-by: David Blaikie <[email protected]>
carbon-language · Sep 4, 2024 · 1412ecd · 1412ecd
1 parent 97e98bc
commit 1412ecd
Show file tree

Hide file tree

Showing 9 changed files with 35 additions and 20 deletions.
diff --git a/toolchain/diagnostics/diagnostic.cpp b/toolchain/diagnostics/diagnostic.cpp
@@ -21,7 +21,7 @@ auto DiagnosticLoc::FormatLocation(llvm::raw_ostream& out) const -> void {
 
 auto DiagnosticLoc::FormatSnippet(llvm::raw_ostream& out, int indent) const
     -> void {
-  if (column_number <= 0) {
+  if (column_number == -1) {
     return;
   }
 

diff --git a/toolchain/diagnostics/diagnostic.h b/toolchain/diagnostics/diagnostic.h
@@ -58,11 +58,12 @@ struct DiagnosticLoc {
   llvm::StringRef filename;
   // A reference to the line of the error.
   llvm::StringRef line;
-  // 1-based line number.
+  // 1-based line number. -1 indicates unknown; other values are unused.
   int32_t line_number = -1;
-  // 1-based column number.
+  // 1-based column number. -1 indicates unknown; other values are unused.
   int32_t column_number = -1;
-  // A location can represent a range of text if set to >1 value.
+  // The number of characters corresponding to the location in the line,
+  // starting at column_number. Should always be at least 1.
   int32_t length = 1;
 };
 

diff --git a/toolchain/lower/file_context.cpp b/toolchain/lower/file_context.cpp
@@ -369,12 +369,10 @@ auto FileContext::BuildDISubprogram(const SemIR::Function& function,
   if (!di_compile_unit_) {
     return nullptr;
   }
-  auto loc = converter_.ConvertLoc(
-      function.definition_id,
-      [](DiagnosticLoc, const Internal::DiagnosticBase<>&) {});
   auto name = sem_ir().names().GetAsStringIfIdentifier(function.name_id);
   CARBON_CHECK(name) << "Unexpected special name for function: "
                      << function.name_id;
+  auto loc = GetLocForDI(function.definition_id);
   // FIXME: Add more details here, including real subroutine type (once type
   // information is built), etc.
   return di_builder_.createFunction(
@@ -542,11 +540,15 @@ auto FileContext::BuildGlobalVariableDecl(SemIR::VarStorage var_storage)
                                   /*Initializer=*/nullptr, mangled_name);
 }
 
-auto FileContext::GetDiagnosticLoc(SemIR::InstId inst_id) -> DiagnosticLoc {
-  return converter_.ConvertLoc(
+auto FileContext::GetLocForDI(SemIR::InstId inst_id) -> LocForDI {
+  auto diag_loc = converter_.ConvertLoc(
       inst_id,
       [&](DiagnosticLoc /*context_loc*/,
           const Internal::DiagnosticBase<>& /*context_diagnostic_base*/) {});
+  return {.filename = diag_loc.filename,
+          .line_number = diag_loc.line_number == -1 ? 0 : diag_loc.line_number,
+          .column_number =
+              diag_loc.column_number == -1 ? 0 : diag_loc.column_number};
 }
 
 }  // namespace Carbon::Lower
diff --git a/toolchain/lower/file_context.h b/toolchain/lower/file_context.h
@@ -18,6 +18,15 @@ namespace Carbon::Lower {
 // Context and shared functionality for lowering handlers.
 class FileContext {
  public:
+  // Location information for use with DebugInfo. The line_number and
+  // column_number are >= 0, with 0 as unknown, so that they can be passed
+  // directly to DebugInfo.
+  struct LocForDI {
+    llvm::StringRef filename;
+    int32_t line_number;
+    int32_t column_number;
+  };
+
   explicit FileContext(llvm::LLVMContext& llvm_context, bool include_debug_info,
                        const Check::SemIRDiagnosticConverter& converter,
                        llvm::StringRef module_name, const SemIR::File& sem_ir,
@@ -46,8 +55,8 @@ class FileContext {
     return types_[type_id.index];
   }
 
-  // Returns the DiagnosticLoc associated with the specified inst_id.
-  auto GetDiagnosticLoc(SemIR::InstId inst_id) -> DiagnosticLoc;
+  // Returns location information for use with DebugInfo.
+  auto GetLocForDI(SemIR::InstId inst_id) -> LocForDI;
 
   // Returns a lowered value to use for a value of type `type`.
   auto GetTypeAsValue() -> llvm::Constant* {

diff --git a/toolchain/lower/function_context.cpp b/toolchain/lower/function_context.cpp
@@ -98,7 +98,10 @@ auto FunctionContext::LowerInst(SemIR::InstId inst_id) -> void {
   CARBON_VLOG() << "Lowering " << inst_id << ": " << inst << "\n";
   builder_.getInserter().SetCurrentInstId(inst_id);
   if (di_subprogram_) {
-    auto loc = file_context_->GetDiagnosticLoc(inst_id);
+    auto loc = file_context_->GetLocForDI(inst_id);
+    CARBON_CHECK(loc.filename == di_subprogram_->getFile()->getFilename())
+        << "Instructions located in a different file from their enclosing "
+           "function aren't handled yet";
     builder_.SetCurrentDebugLocation(
         llvm::DILocation::get(builder_.getContext(), loc.line_number,
                               loc.column_number, di_subprogram_));

diff --git a/toolchain/lower/testdata/global/class_obj.carbon b/toolchain/lower/testdata/global/class_obj.carbon
@@ -36,8 +36,8 @@ var a: A = {};
 // CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3}
 // CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
 // CHECK:STDOUT: !3 = !DIFile(filename: "class_obj.carbon", directory: "")
-// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, line: 4294967295, type: !5, spFlags: DISPFlagDefinition, unit: !2)
+// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, type: !5, spFlags: DISPFlagDefinition, unit: !2)
 // CHECK:STDOUT: !5 = !DISubroutineType(types: !6)
 // CHECK:STDOUT: !6 = !{}
 // CHECK:STDOUT: !7 = !DILocation(line: 12, column: 1, scope: !4)
-// CHECK:STDOUT: !8 = !DILocation(line: 4294967295, scope: !4)
+// CHECK:STDOUT: !8 = !DILocation(line: 0, scope: !4)
diff --git a/toolchain/lower/testdata/global/class_with_fun.carbon b/toolchain/lower/testdata/global/class_with_fun.carbon
@@ -54,6 +54,6 @@ var a: A = {};
 // CHECK:STDOUT: !5 = !DISubroutineType(types: !6)
 // CHECK:STDOUT: !6 = !{}
 // CHECK:STDOUT: !7 = !DILocation(line: 13, column: 3, scope: !4)
-// CHECK:STDOUT: !8 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, line: 4294967295, type: !5, spFlags: DISPFlagDefinition, unit: !2)
+// CHECK:STDOUT: !8 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, type: !5, spFlags: DISPFlagDefinition, unit: !2)
 // CHECK:STDOUT: !9 = !DILocation(line: 16, column: 1, scope: !8)
-// CHECK:STDOUT: !10 = !DILocation(line: 4294967295, scope: !8)
+// CHECK:STDOUT: !10 = !DILocation(line: 0, scope: !8)
diff --git a/toolchain/lower/testdata/global/simple_init.carbon b/toolchain/lower/testdata/global/simple_init.carbon
@@ -31,8 +31,8 @@ var a: i32 = 0;
 // CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3}
 // CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
 // CHECK:STDOUT: !3 = !DIFile(filename: "simple_init.carbon", directory: "")
-// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, line: 4294967295, type: !5, spFlags: DISPFlagDefinition, unit: !2)
+// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, type: !5, spFlags: DISPFlagDefinition, unit: !2)
 // CHECK:STDOUT: !5 = !DISubroutineType(types: !6)
 // CHECK:STDOUT: !6 = !{}
 // CHECK:STDOUT: !7 = !DILocation(line: 10, column: 1, scope: !4)
-// CHECK:STDOUT: !8 = !DILocation(line: 4294967295, scope: !4)
+// CHECK:STDOUT: !8 = !DILocation(line: 0, scope: !4)
diff --git a/toolchain/lower/testdata/global/simple_with_fun.carbon b/toolchain/lower/testdata/global/simple_with_fun.carbon
@@ -46,7 +46,7 @@ var a: i32 = test_a();
 // CHECK:STDOUT: !5 = !DISubroutineType(types: !6)
 // CHECK:STDOUT: !6 = !{}
 // CHECK:STDOUT: !7 = !DILocation(line: 12, column: 3, scope: !4)
-// CHECK:STDOUT: !8 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, line: 4294967295, type: !5, spFlags: DISPFlagDefinition, unit: !2)
+// CHECK:STDOUT: !8 = distinct !DISubprogram(name: "__global_init", linkageName: "__global_init", scope: null, file: !3, type: !5, spFlags: DISPFlagDefinition, unit: !2)
 // CHECK:STDOUT: !9 = !DILocation(line: 15, column: 14, scope: !8)
 // CHECK:STDOUT: !10 = !DILocation(line: 15, column: 1, scope: !8)
-// CHECK:STDOUT: !11 = !DILocation(line: 4294967295, scope: !8)
+// CHECK:STDOUT: !11 = !DILocation(line: 0, scope: !8)