Do not optimize debug locations across section boundaries

[pcalixte]

Summary

Prevent optimization of DebugLoc across section boundaries, such optimization will yield incorrect source location if memory layout of sections does not strictly match the Asm file.

[Orlando]

This seems sensible and the test looks good. I've added other reviewers who might want to take a look.

Please could you add context to the patch (e.g. git diff -U9999)?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2049–2050

I think it'd be worth splitting the new condition out to name it - or possibly just adding a comment instead.

bool PrevInstInSameSection = PrevInstBB &&
      PrevInstBB->getSectionIDNum() == MI->getParent()->getSectionIDNum();

if (DL == PrevInstLoc && PrevInstInSameSection)  {

And I'm not sure if I'm reading it right but should the condition be (!PrevInstBB || PrevInstBB->getSectionIDNum() == MI->getParent()->getSectionIDNum()) instead to maintain "first instruction" behaviour when DL is an unspecified location (!DL)?

[pcalixte]

Fixing and naming new condition for clarity (according to review)

[pcalixte]

Thank you Orlando - you are correct - I have updated the review as you suggested

[pcalixte]

not change - regenerate diff based on main

[dblaikie]

Looks OK to me. Not sure if this'll end up having any overlap with D147506

[pcalixte]

same contents - removing extra blanks

[MaskRay]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

5

Nit: It's more common to omit the blank line.

llvm/test/DebugInfo/debuglineinfo-at-section-boundary.ll

1 ↗

(On Diff #518609)

Drop -filetype=asm. It's the default and assumed by most `llc invocations.

Drop -asm-verbose=0 as well. There is no difference in the output. The option isn't common to specify.

[MaskRay]

Prevent optimization of DebugLoc across section boundaries, such optimization will yield incorrect source location if memory layout of sections does not strictly match the Asm file.

Consider adding the .loc 0 4 1 information to the summary (commit message).

[pcalixte]

DwarfDebug.cpp:
Remove empty line as per review
clang-format

testcase:
remove -filetype=asm and -asm-verbose=0 as per review.
[you may have noticed that the test was failing on Windows/ -basic-block-sections not supported]
I have moved and renamed the testcase to be more in sync with existing testcases for basic-block-sections

[pcalixte]

I do not have commit access, can someone with the proper credentials commit the patch? [if the the latest version looks ok]

[probinson]

Sorry I haven't been keeping up, but I have a suggestion for the test.

llvm/test/DebugInfo/X86/basic-block-sections-debug-lineinfo-at-section-boundary.ll

17 ↗

(On Diff #520535)

This shows there are two .loc directives, but not that they are in different sections. I know I've seen redundant .loc directives and we even have a downstream patch to filter them out, so I think it would be good for this test to at least verify there's a .section directive between the two .loc directives.

[pcalixte]

revised in https://reviews.llvm.org/D150308

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

 //===- llvm/CodeGen/DwarfDebug.cpp - Dwarf Debug Framework ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains support for writing dwarf debug info into asm files.
 //
 //===----------------------------------------------------------------------===//
 
 #include "DwarfDebug.h"
if (MI->getFlag(MachineInstr::FrameDestroy) && DL) {
     }
   }
 
   // When we emit a line-0 record, we don't update PrevInstLoc; so look at
   // the last line number actually emitted, to see if it was line 0.
   unsigned LastAsmLine =
       Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
 
-  if (DL == PrevInstLoc) {
+  bool PrevInstInSameSection = (!PrevInstBB ||
+         PrevInstBB->getSectionIDNum() == MI->getParent()->getSectionIDNum());
+
+  if (DL == PrevInstLoc && PrevInstInSameSection) {
     // If we have an ongoing unspecified location, nothing to do here.
     if (!DL)
       return;
     // We have an explicit location, same as the previous location.
     // But we might be coming back to it after a line 0 record.
     if ((LastAsmLine == 0 && DL.getLine() != 0) || Flags) {
       // Reinstate the source location but not marked as a statement.
       const MDNode *Scope = DL.getScope();

llvm/test/DebugInfo/Generic/debuglineinfo-at-section-boundary.ll

+; RUN: llc -filetype=asm -asm-verbose=0 --basic-block-sections=all < %s | FileCheck %s
+;
+; Verify that AsmPrinter does not optimize debug line directives
+; across two distinct sections. Such optimization will lead
+; to incorrect debug line number if the memory layout of those
+; sections does not strictly mirror the order in the Asm file.
+;
+;    int G;
+;    void func(int x) {
+;      if (x) G=0;
+;    }
+;
+; Reduced from the above example, the IR has been modified so that
+; the store and return instructions have the same DebugLoc; in this
+; context, we should see 2 different directives for line #4
+;
+; CHECK-COUNT-2: .loc    0 4 1
+
+define void @func(i1 %0) !dbg !8 {
+  br i1 %0, label %3, label %2
+
+2:                                                ; preds = %1
+  store i32 0, ptr null, align 4, !dbg !12
+  br label %3
+
+3:                                                ; preds = %2, %1
+  ret void, !dbg !12
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!6, !7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 16.0.2", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/tmp", checksumkind: CSK_MD5, checksum: "fd2935e6157b382ad81da17a61f24bb1")
+!2 = !{!3}
+!3 = !DIGlobalVariableExpression(var: !4, expr: !DIExpression())
+!4 = distinct !DIGlobalVariable(name: "G", scope: !0, file: !1, line: 1, type: !5, isLocal: false, isDefinition: true)
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!6 = !{i32 7, !"Dwarf Version", i32 5}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = distinct !DISubprogram(name: "func", scope: !1, file: !1, line: 2, type: !9, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null}
+!11 = !{}
+!12 = !DILocation(line: 4, column: 1, scope: !8)