dsymutil support for DW_OP_convert

[aprantl]

Summary

So far I only got this testcase and it fails...

I'll update this as soon as I'm done implementing the missing pieces in dsymutil. We'll have to scan each DW_AT_location for DW_OP_convert DIE references and mark them as kept and then during cloning ensure to update the relative CU DIE offset if necessary.

[aprantl]

Here's the the first part of the patch: support for inline locations. Support for location lists is still coming.

[aprantl]

Here's a first complete implementation.

In this form a ReleaseAsserts dsymutil run on a ReleaseAsserts+modules clang is about 5% slower:

# trunk
$ time dsymutil clang   # best out of 3 runs

real	0m46.861s
user	0m53.870s
sys	0m4.019s

# with this patch

real	0m48.995s
user	0m55.888s
sys	0m4.174s

[markus]

llvm/tools/dsymutil/DwarfLinker.cpp

723 ↗

(On Diff #188250)

Check if DWARFExpression::Operation::Encoding is BaseTypeRef instead.

1151 ↗

(On Diff #188250)

Instead of checking for DW_OP_convert explicitly we could check the DWARFExpression::Operation::Encoding to see if it is BaseTypeRef.

[aprantl]

Thanks for pointing out that DW_OP_convert isn't the only operation that uses a CU-relative type DIE offset!
I went through the DWARF 5 spec once more and it looks like all the operations that require a type parameter also require that that type parameter is a basic type. Since basic types are tiny, we can just keep them all unconditionally and skip the entire scanExpressionDIERefs part. With this change dsymutil gets more than 2 seconds faster on clang, basically almost the original performance.

[aprantl]

Faster version without the first scan expression pass.

Removing the WIP tag; this is now ready for a thorough review!

[friss]

llvm/tools/dsymutil/DwarfLinker.cpp

1100 ↗

(On Diff #188451)

if you hit the error, then it looks like you're only going to emit NOPs instead of the ULEB. This is going to be badly misinterpreted by a consumer. You really want to NOP-out the whole convert expression.

[markus]

llvm/tools/dsymutil/DwarfLinker.cpp

1100 ↗

(On Diff #188451)

If the BaseTypeRef ULEB does not fit then I believe that you want to replace it (the BaseTypeRef ULEB) with zeros as that will be interpreted as a reference to the generic type according to the spec. I guess that replacing the entire operation and its operands with several DW_OP_nop would also be an option. Doing the former seems easier though and would arguably be more accurate in my opinion.

[aprantl]

Address review feedback.

[friss]

LGTM on the surface. I wouldn't mind someone else (@JDevlieghere ?) taking a look.

[JDevlieghere]

Few nits in addition to the stuff we discussed offline.

llvm/include/llvm/DebugInfo/DWARF/DWARFAttribute.h

47 ↗

(On Diff #188451)

nit/subjective: I prefer mayHaveLocationDescription

llvm/tools/dsymutil/DwarfLinker.cpp

1066 ↗

(On Diff #188565)

Let's do this at the beginning of the functions.

1071 ↗

(On Diff #188565)

Can we invert this and do a continue?

llvm/tools/dsymutil/DwarfLinker.h

203 ↗

(On Diff #188451)

Can we use the endianness enum from Support/Endian.h?

368 ↗

(On Diff #188451)

Can you include why these two are different in the doxygen comment?

[probinson]

Drive-by nit.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp

713 ↗

(On Diff #188565)

DWARF v5? As this will be yet another place to update in future revisions.

[aprantl]

Got rid of most of the callbacks and per-byte copies.

[aprantl]

llvm/tools/dsymutil/DwarfLinker.cpp

1071 ↗

(On Diff #188565)

No, there is an else path, too.

[aprantl]

Got rid of the second cloneExpression function.

[aprantl]

Add a comment.

[JDevlieghere]

LGTM

llvm/test/tools/dsymutil/Inputs/op-convert.ll

+; ModuleID = 'dbg.ll'
+source_filename = "dbg.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx"
+
+; Function Attrs: noinline nounwind uwtable
+define signext i8 @foo(i8 signext %x) #0 !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i8 42, metadata !17, metadata !DIExpression(DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)), !dbg !12
+  call void @llvm.dbg.value(metadata i8 %x, metadata !11, metadata !DIExpression()), !dbg !12
+  call void @llvm.dbg.value(metadata i8 %x, metadata !13, metadata !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)), !dbg !15
+  ret i8 %x, !dbg !16
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline nounwind uwtable }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 9.0.0 (trunk 353791) (llvm/trunk 353801)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "dbg.c", directory: "/tmp", checksumkind: CSK_MD5, checksum: "2a034da6937f5b9cf6dd2d89127f57fd")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 9.0.0 (trunk 353791) (llvm/trunk 353801)"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !8, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10}
+!10 = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+!11 = !DILocalVariable(name: "x", arg: 1, scope: !7, file: !1, line: 1, type: !10)
+!12 = !DILocation(line: 1, column: 29, scope: !7)
+!13 = !DILocalVariable(name: "y", scope: !7, file: !1, line: 3, type: !14)
+!14 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = !DILocation(line: 3, column: 14, scope: !7)
+!16 = !DILocation(line: 4, column: 3, scope: !7)
+!17 = !DILocalVariable(name: "c", scope: !7, file: !1, line: 3, type: !14)

llvm/test/tools/dsymutil/X86/op-convert.test

+# REQUIRES: object-emission
+# RUN: dsymutil -f -o %t --verify -oso-prepend-path=%p/../Inputs -y %s
+# RUN: llvm-dwarfdump %t | FileCheck %s
+
+---
+triple:          'x86_64-apple-darwin'
+objects:
+  - filename: op-convert.macho.x86_64
+    symbols:
+      - { sym: _foo, objAddr: 0x0, binAddr: 0x1000, size: 0x4 }
+...
+
+
+CHECK:      DW_TAG_base_type
+CHECK-NEXT:     DW_AT_name  ("DW_ATE_signed_8")
+CHECK-NEXT:     DW_AT_encoding  (DW_ATE_signed)
+CHECK-NEXT:     DW_AT_byte_size (0x01)
+
+CHECK:   DW_TAG_base_type
+CHECK-NEXT:     DW_AT_name  ("DW_ATE_signed_32")
+CHECK-NEXT:     DW_AT_encoding  (DW_ATE_signed)
+CHECK-NEXT:     DW_AT_byte_size (0x04)
+
+CHECK:     DW_TAG_variable
+
+CHECK:     DW_TAG_variable
+CHECK-NEXT:     DW_AT_location  (0x00000036
+CHECK-NEXT:     [0x0000000000000000,  0x0000000000000002): DW_OP_breg5 RDI+0, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_convert (0x0000002a) "DW_ATE_signed_8", DW_OP_convert (0x00000031) "DW_ATE_signed_32", DW_OP_stack_value
+CHECK-NEXT:     [0x0000000000000002,  0x0000000000000003): DW_OP_breg0 RAX+0, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_convert (0x0000002a) "DW_ATE_signed_8", DW_OP_convert (0x00000031) "DW_ATE_signed_32", DW_OP_stack_value)
+CHECK-NEXT:     DW_AT_name  ("y")
+
+CHECK:     DW_TAG_variable
+CHECK-NEXT:     DW_AT_location  (DW_OP_constu 0x2a, DW_OP_convert (0x00000031) "DW_ATE_signed_32", DW_OP_stack_value)
+CHECK-NEXT:     DW_AT_name  ("c")
+