Skip to content

Conversation

@noxwell
Copy link
Contributor

@noxwell noxwell commented Dec 11, 2024

Update root file in DWARF file/line table as soon as we see the first "#line" directive.

This was moved from "enabledGenDwarfForAssembly", which is called right before we emit DWARF information. But if the file is empty or contains expressions that doesn't need DWARF, it is never called, leaving an original root file and not the file in the "#line" directive.

Add a test checking for this case.

This is reapply of #119229 with the following fix:

"MCContext::setMCLineTableRootFile" has the effect of adding ".debug_line" section to the output, even if DWARF generation is disabled. Add a check and a test for this case.

Fixes: #119020
Fixes: #119229

@llvmbot llvmbot added the llvm:mc Machine (object) code label Dec 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 11, 2024

@llvm/pr-subscribers-mc

Author: Aleksei Vetrov (noxwell)

Changes

Update root file in DWARF file/line table as soon as we see the first "#line" directive.

This was moved from "enabledGenDwarfForAssembly", which is called right before we emit DWARF information. But if the file is empty or contains expressions that doesn't need DWARF, it is never called, leaving an original root file and not the file in the "#line" directive.

Add a test checking for this case.

This is reapply of #119229 with the following fix:

"MCContext::setMCLineTableRootFile" has the effect of adding ".debug_line" section to the output, even if DWARF generation is disabled. Add a check and a test for this case.

Fixes: #119020
Fixes: #119229


Full diff: https://github.com/llvm/llvm-project/pull/119572.diff

3 Files Affected:

  • (modified) llvm/lib/MC/MCParser/AsmParser.cpp (+16-10)
  • (added) llvm/test/MC/ELF/debug-hash-file-disabled-dwarf.s (+12)
  • (added) llvm/test/MC/ELF/debug-hash-file-empty-dwarf.s (+26)
diff --git a/llvm/lib/MC/MCParser/AsmParser.cpp b/llvm/lib/MC/MCParser/AsmParser.cpp
index d15a9a8a36c5a3..153c1070a68c82 100644
--- a/llvm/lib/MC/MCParser/AsmParser.cpp
+++ b/llvm/lib/MC/MCParser/AsmParser.cpp
@@ -162,8 +162,8 @@ class AsmParser : public MCAsmParser {
   };
   CppHashInfoTy CppHashInfo;
 
-  /// The filename from the first cpp hash file line comment, if any.
-  StringRef FirstCppHashFilename;
+  /// Have we seen any file line comment.
+  bool HadCppHashFilename = false;
 
   /// List of forward directional labels for diagnosis at the end.
   SmallVector<std::tuple<SMLoc, CppHashInfoTy, MCSymbol *>, 4> DirLabels;
@@ -952,12 +952,6 @@ bool AsmParser::enabledGenDwarfForAssembly() {
   // the assembler source was produced with debug info already) then emit one
   // describing the assembler source file itself.
   if (getContext().getGenDwarfFileNumber() == 0) {
-    // Use the first #line directive for this, if any. It's preprocessed, so
-    // there is no checksum, and of course no source directive.
-    if (!FirstCppHashFilename.empty())
-      getContext().setMCLineTableRootFile(
-          /*CUID=*/0, getContext().getCompilationDir(), FirstCppHashFilename,
-          /*Cksum=*/std::nullopt, /*Source=*/std::nullopt);
     const MCDwarfFile &RootFile =
         getContext().getMCDwarfLineTable(/*CUID=*/0).getRootFile();
     getContext().setGenDwarfFileNumber(getStreamer().emitDwarfFileDirective(
@@ -2440,8 +2434,20 @@ bool AsmParser::parseCppHashLineFilenameComment(SMLoc L, bool SaveLocInfo) {
   CppHashInfo.Filename = Filename;
   CppHashInfo.LineNumber = LineNumber;
   CppHashInfo.Buf = CurBuffer;
-  if (FirstCppHashFilename.empty())
-    FirstCppHashFilename = Filename;
+  if (!HadCppHashFilename) {
+    HadCppHashFilename = true;
+    // If we haven't encountered any .file directives, then the first #line
+    // directive describes the "root" file and directory of the compilation
+    // unit.
+    if (getContext().getGenDwarfForAssembly() &&
+        getContext().getGenDwarfFileNumber() == 0) {
+      // It's preprocessed, so there is no checksum, and of course no source
+      // directive.
+      getContext().setMCLineTableRootFile(
+          /*CUID=*/0, getContext().getCompilationDir(), Filename,
+          /*Cksum=*/std::nullopt, /*Source=*/std::nullopt);
+    }
+  }
   return false;
 }
 
diff --git a/llvm/test/MC/ELF/debug-hash-file-disabled-dwarf.s b/llvm/test/MC/ELF/debug-hash-file-disabled-dwarf.s
new file mode 100644
index 00000000000000..3a28a5ea838144
--- /dev/null
+++ b/llvm/test/MC/ELF/debug-hash-file-disabled-dwarf.s
@@ -0,0 +1,12 @@
+// RUN: llvm-mc -triple x86_64-unknown-linux-gnu -filetype obj -o %t %s
+// RUN: llvm-readelf --sections %t | FileCheck %s
+
+// CHECK: Section Headers:
+// CHECK-NOT: .debug_
+
+# 1 "/MyTest/Inputs/other.S"
+
+foo:
+  nop
+  nop
+  nop
diff --git a/llvm/test/MC/ELF/debug-hash-file-empty-dwarf.s b/llvm/test/MC/ELF/debug-hash-file-empty-dwarf.s
new file mode 100644
index 00000000000000..cc1c3d1796b6e2
--- /dev/null
+++ b/llvm/test/MC/ELF/debug-hash-file-empty-dwarf.s
@@ -0,0 +1,26 @@
+// RUN: llvm-mc -triple x86_64-unknown-linux-gnu -filetype obj -g -dwarf-version 5 -o %t %s
+// RUN: llvm-dwarfdump -debug-info -debug-line %t | FileCheck %s
+
+// CHECK-NOT: DW_TAG_
+
+// CHECK:      include_directories[ 0] =
+// CHECK-NOT:  include_directories[ 1] =
+// CHECK:      file_names[ 0]:
+// CHECK-NEXT:           name: "/MyTest/Inputs/other.S"
+// CHECK-NEXT:      dir_index: 0
+// CHECK-NOT:  file_names[ 1]:
+
+// RUN: llvm-mc -triple=x86_64 -filetype=obj -g -dwarf-version=5 -fdebug-prefix-map=/MyTest=/src_root %s -o %t.5.o
+// RUN: llvm-dwarfdump -debug-info -debug-line %t.5.o | FileCheck %s --check-prefixes=MAP
+
+// MAP-NOT: DW_TAG_
+
+// MAP:      include_directories[  0] = "{{.*}}"
+// MAP-NEXT: file_names[  0]:
+// MAP-NEXT:            name: "/src_root/Inputs/other.S"
+// MAP-NEXT:       dir_index: 0
+
+# 1 "/MyTest/Inputs/other.S"
+
+.section .data
+.asciz "data"

@noxwell
Copy link
Contributor Author

noxwell commented Dec 11, 2024

@nickdesaulniers @dwblaikie Could you review the fixed version of #119229?
@chapuni Could you provide me with a test that was broken? I added a specific test for #line without -g flag and did check-all with -DLLVM_ENABLE_PROJECTS=clang. But it looks like you have a separate project that was broken by this change, and I would like to check that it will not be broken with this reapply.

@nickdesaulniers nickdesaulniers changed the title [MC] Fix DWARF file table for files with empty DWARF (#119020) [MC] Fix DWARF file table for files with empty DWARF Dec 11, 2024
Copy link
Contributor

@chapuni chapuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noxwell They were stage2 (and stage3 that was built on another instance) objects in Support/BLAKE3. I noticed they were different each other.

Update root file in DWARF file/line table as soon as we see the first
"#line" directive.

This was moved from "enabledGenDwarfForAssembly", which is called right
before we emit DWARF information. But if the file is empty or contains
expressions that doesn't need DWARF, it is never called, leaving an
original root file and not the file in the "#line" directive.

Add a test checking for this case.

This is reapply of llvm#119229 with the following fix:

"MCContext::setMCLineTableRootFile" has the effect of adding
".debug_line" section to the output, even if DWARF generation is
disabled. Add a check and a test for this case.

Fixes: llvm#119020
Fixes: llvm#119229
@nickdesaulniers nickdesaulniers merged commit 4a5f82b into llvm:main Dec 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:mc Machine (object) code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MC] Compiler produces incorrect DWARF file/line table on some assembly files

4 participants