Skip to content

Commit

Permalink
Reapply "[DebugInfo][RemoveDIs] Turn on non-instrinsic debug-info by …
Browse files Browse the repository at this point in the history 
…default"

This reapplies commit bdde5f9 by undoing the revert bc66e0c.

The previous reapplication 5c9f768 was reverted due to a crash
(reproducer in comments for 5c9f768) which was fixed in #81595.

As noted in the original commit, this commit may break downstream tests.
If this commit is breaking your downstream tests, please see comment 12 in
[0], which documents the kind of variation in tests we'd expect to see from
this change and what to do about it.

[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939
  • Loading branch information
@OCHyams
OCHyams committed Feb 13, 2024
1 parent 4612208 commit d759618
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion llvm/lib/IR/BasicBlock.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ cl::opt<bool>
UseNewDbgInfoFormat("experimental-debuginfo-iterators",
cl::desc("Enable communicating debuginfo positions "
"through iterators, eliminating intrinsics"),
cl::init(false));
cl::init(true));

DPMarker *BasicBlock::createMarker(Instruction *I) {
assert(IsNewDbgInfoFormat &&
Expand Down

8 comments on commit d759618

@aeubanks
Copy link
Contributor

Choose a reason for hiding this comment

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

another crash:

$ cat /tmp/a.ll

target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
target triple = "aarch64-unknown-fuchsia"

%struct.hb_glyph_info_t = type { i32, i32, i32, %union._hb_var_int_t, %union._hb_var_int_t }
%union._hb_var_int_t = type { i32 }

define void @_Z22_hb_ot_shape_normalizePK18hb_ot_shape_plan_tP11hb_buffer_tP9hb_font_t() {
entry:
  br label %while.body193

while.body193:                                    ; preds = %while.body193, %entry
  %starter.0337 = phi i32 [ %spec.select322, %while.body193 ], [ 0, %entry ]
  %idxprom207 = zext i32 %starter.0337 to i64
  %arrayidx208 = getelementptr %struct.hb_glyph_info_t, ptr null, i64 %idxprom207
  %0 = load i32, ptr %arrayidx208, align 4
  %call247.val = load i16, ptr null, align 4
  %cmp249327 = icmp ult i16 %call247.val, 0
  %cmp249 = select i1 false, i1 false, i1 %cmp249327
  %spec.select322 = select i1 %cmp249, i32 0, i32 %starter.0337
  tail call void @llvm.dbg.value(metadata i32 %spec.select322, metadata !13, metadata !DIExpression()), !dbg !20
  br label %while.body193
}

; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
declare void @llvm.dbg.value(metadata, metadata, metadata) #0

attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!12}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !2, globals: !3, imports: !2, splitDebugInlining: false, nameTableKind: GNU)
!1 = !DIFile(filename: "../../third_party/harfbuzz-ng/src/src/hb-ot-shape-normalize.cc", directory: ".")
!2 = !{}
!3 = !{!4, !9}
!4 = !DIGlobalVariableExpression(var: !5, expr: !DIExpression())
!5 = distinct !DIGlobalVariable(scope: null, file: !1, line: 383, type: !6, isLocal: true, isDefinition: true)
!6 = !DICompositeType(tag: DW_TAG_array_type, baseType: !7, size: 112, elements: !2)
!7 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !8)
!8 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_unsigned_char)
!9 = !DIGlobalVariableExpression(var: !10, expr: !DIExpression())
!10 = distinct !DIGlobalVariable(scope: null, file: !1, line: 410, type: !11, isLocal: true, isDefinition: true)
!11 = !DICompositeType(tag: DW_TAG_array_type, baseType: !7, size: 96, elements: !2)
!12 = !{i32 2, !"Debug Info Version", i32 3}
!13 = !DILocalVariable(name: "starter", scope: !14, file: !1, line: 441, type: !19)
!14 = distinct !DILexicalBlock(scope: !15, file: !1, line: 435, column: 3)
!15 = distinct !DILexicalBlock(scope: !16, file: !1, line: 431, column: 7)
!16 = distinct !DISubprogram(name: "_hb_ot_shape_normalize", linkageName: "_Z22_hb_ot_shape_normalizePK18hb_ot_shape_plan_tP11hb_buffer_tP9hb_font_t", scope: !1, file: !1, line: 291, type: !17, scopeLine: 294, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
!17 = distinct !DISubroutineType(types: !18)
!18 = !{null}
!19 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
!20 = !DILocation(line: 0, scope: !14)

$ llc -O3 -o /dev/null /tmp/a.ll

Trailing DPValues in block
Invalid configuration of new-debug-info data found
in function _Z22_hb_ot_shape_normalizePK18hb_ot_shape_plan_tP11hb_buffer_tP9hb_font_t
LLVM ERROR: Broken function found, compilation aborted!

@jmorse
Copy link
Member

@jmorse jmorse commented on d759618 Feb 13, 2024

Choose a reason for hiding this comment

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

Thanks for the catch; are the two crashes so far are coming from the same codebase, and if so is it opensource so that we can test over that before turning on again?

@slackito
Copy link
Collaborator

Choose a reason for hiding this comment

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

We've also noticed a test regression after this change: we have a very basic smoke test to ensure the compiler produces some debug info, and it consists of a test program like this:

int main(int argc, char **argv) { return 0; }

and some logic that runs the test program under gdb and tries to print main. When building with -O0 -flto=thin (weird combination of flags, it comes from generating combinations of a bunch of configs we need to test) it drops the arguments to main and we get something like this from the debugger:

{int (void)} 0x298950 <main()>

@dwblaikie helped me verify that this reproduces upstream:

clang++-tot -g test.cpp -flto=thin -O0 -fuse-ld=lld && llvm-dwarfdump-tot a.out
a.out:  file format elf64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000033, format = DWARF32, version = 0x0005, unit_type = DW_UT_compile, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000037)

0x0000000c: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 19.0.0git (git@github.com:llvm/llvm-project.git 89c1bf1230e011f2f0e43554c278205fa1819de5)")
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_name        ("test.cpp")
              DW_AT_str_offsets_base    (0x00000008)
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_low_pc      (0x00000000000016e0)
              DW_AT_high_pc     (0x00000000000016ef)
              DW_AT_addr_base   (0x00000008)

0x00000023:   DW_TAG_subprogram
                DW_AT_low_pc    (0x00000000000016e0)
                DW_AT_high_pc   (0x00000000000016ef)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("main")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/test.cpp")
                DW_AT_decl_line (1)
                DW_AT_type      (0x00000032 "int")
                DW_AT_external  (true)

0x00000032:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000036:   NULL

@aeubanks
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the catch; are the two crashes so far are coming from the same codebase, and if so is it opensource so that we can test over that before turning on again?

yeah they're both from Chromium. the problem is that there are so many configurations (opt level, arch, OS, debug level) that it'd be hard to test them all manually. personally I'm fine with continually relanding and reverting, I don't think there are too many issues left given that I only saw this issue when monitoring the bots today, but the bots can take a while to cycle and I may have missed others.

@jmorse
Copy link
Member

@jmorse jmorse commented on d759618 Feb 14, 2024

Choose a reason for hiding this comment

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

@slackito wrote:

We've also noticed a test regression after this change: we have a very basic smoke test to ensure the compiler produces some debug info, and it consists of a test program like this:

Thanks for the report; I think this was likely fixed with fa77e1f a couple of hours ago -- we'd completely missed a hole where the ThinLTO writer wasn't seeing the right flavour of debug-info (facepalms all around). You should be able to confirm it doesn't reproduce with the tip of master by adding -mllvm --experimental-debuginfo-iterators=true to the clang command line.

yeah they're both from Chromium. the problem is that there are so many configurations (opt level, arch, OS, debug level) that it'd be hard to test them all manually. personally I'm fine with continually relanding and reverting

Cool, if it's not majorly interfering with peoples workflows then that's great, obviously we don't want to (un-necessarily) fuzz other peoples CIs.

@OCHyams
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another crash:
<...>

Trailing DPValues in block
Invalid configuration of new-debug-info data found
in function _Z22_hb_ot_shape_normalizePK18hb_ot_shape_plan_tP11hb_buffer_tP9hb_font_t
LLVM ERROR: Broken function found, compilation aborted!

Thanks for the reduced reproducer! That's fxed in #81737.

@OCHyams
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-landed this in a93a4ec

@dwblaikie
Copy link
Collaborator

Choose a reason for hiding this comment

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

@slackito wrote:

We've also noticed a test regression after this change: we have a very basic smoke test to ensure the compiler produces some debug info, and it consists of a test program like this:

Thanks for the report; I think this was likely fixed with fa77e1f a couple of hours ago -- we'd completely missed a hole where the ThinLTO writer wasn't seeing the right flavour of debug-info (facepalms all around). You should be able to confirm it doesn't reproduce with the tip of master by adding -mllvm --experimental-debuginfo-iterators=true to the clang command line.

Yep, can confirm that seems to address the ThinLTO issue. Thanks!

Please sign in to comment.