Fixes and improvements for debugging output #3469

ggiraldez · 2016-10-24T17:17:08Z

Output declarations of both parameters and local variables when emitting function debugging information.
Add functions for forward declaration of types in LLVM >= 3.8 (wrap llvm::DIBuilder.createReplaceableCompositeType and llvm::DIBuilder.replaceTemporary
Push LLVM version conditionals from debug.cr into di_builder.cr
Add spacing and comments in preprocessor directives in llvm_ext.cc to improve readability

Eg. when compiling with debug info the following code

def my_add(x, y)
  z = x + y
  z
end

my_add(10, 20)

you can now display the values of the parameters x and y, like so:

(gdb) break my_add
Breakpoint 1 at 0x1000041ca: file /Users/ggiraldez/ScratchPad/crystal-debug/foo.cr, line 1.
(gdb) run
...
Breakpoint 1, my_add (x=10, y=20) at /Users/ggiraldez/ScratchPad/crystal-debug/foo.cr:1
1   def my_add(x, y)
(gdb) info locals
z = 0
(gdb) info arg
x = 10
y = 20
(gdb)

Regarding the new functions to handle forward declarations, using LLVMTemporaryMDNode and LLVMMetadataReplaceAllUsesWith no longer works in LLVM 3.8. I run into this issue simply by compiling with LLVM 3.8.1 from Homebrew, then trying to compile this program with debug info:

class Foo
  @parent = uninitialized Foo
end

def test
  f = Foo.new
end

test

This crashed the compiler with a failed assertion:

Using compiled compiler at .build/crystal
Assertion failed: ((!MD || isa<MDString>(MD) || isa<T>(MD)) && "Expected valid ref"), function TypedDINodeRef, file /private/tmp/llvm38-20161006-73093-mrdx69/llvm-3.8.1.src/include/llvm/IR/DebugInfoMetadata.h, line 60.
Abort trap: 6

Found the solution here: https://groups.google.com/d/msg/llvm-dev/9C3yuck4JZY/yDxxP9ypEwAJ

- Declare parameters as well as local variables when emitting function debugging information - Add functions for forward declaration of types in LLVM >= 3.8 - Push LLVM version conditionals from debug.cr into di_builder.cr - Add spacing and comments in preprocessor directives in llvm_ext.cc to improve readability

ysbaddaden

Thanks! Improvements to debug are always welcome. Could you refer to the LLVM documentation or expose a little bit the benefits. I don't know much of LLVM, and the documentation is sometimes a little abrupt to search.

ysbaddaden · 2016-10-24T19:06:59Z

src/llvm/lib_llvm_ext.cr

@@ -86,8 +94,17 @@ lib LibLLVMExt
                                                                      align_in_bits : UInt64,
                                                                      name : LibC::Char*) : Metadata

+{% if LibLLVM::IS_38 %}


This should be the other way around: if IS_35 or IS_36, because we also support 3.9 now, and I expect the functions to be available in 4.x too.

You are absolutely right. It should be fixed in 0ca0868.

ysbaddaden · 2016-10-24T19:08:23Z

src/llvm/di_builder.cr

@@ -31,27 +31,37 @@ struct LLVM::DIBuilder

  def create_function(scope, name, linkage_name, file, line, composite_type, is_local_to_unit, is_definition,
                      scope_line, flags, is_optimized, func)
-    LibLLVMExt.di_builder_create_function(self, scope, name, linkage_name, file, line, composite_type, is_local_to_unit, is_definition,
-      scope_line, flags, is_optimized, func)
+    {% if LibLLVM::IS_36 || LibLLVM::IS_35 %}


Thanks for hidding discrepancies into the LLVM module!

ysbaddaden · 2016-10-24T19:10:00Z

src/compiler/crystal/codegen/debug.cr

-      {% end %}
-      builder.set_metadata(declare, @dbg_kind, builder.current_debug_location)
-      {% end %}
+      # TODO: This is redundant for LLVM >= 3.8, but currently we don't have access to builder from DIBuilder


Is it avoidable? Or could the method be a noop on 3.8+?

Yep! Moved the logic to set the debug location to the C++ extension for LLVM <= 3.6 as well.

I also refactored these methods to avoid code repetition.

ysbaddaden · 2016-10-24T19:11:30Z

src/compiler/crystal/codegen/debug.cr

@@ -3,6 +3,8 @@ require "./codegen"
 module Crystal
  class CodeGenVisitor
    CRYSTAL_LANG_DEBUG_IDENTIFIER = 0x8002_u32
+    # TODO: for correctness, eventually this should come from the current LLVM context
+    DBG_KIND = LibLLVM.get_md_kind_id("dbg", 3)


What about making it a method, so it would come from the current context?

No longer relevant in 0ca0868, as I removed it altogether

- Refactor code to emit the `llvm.dbg.declare` instrinsic and reuse for both local variables and method arguments - Move debug location setting for debug declare instrinsic into C++ extension for LLVM <= 3.6 (functionality is built-in for LLVM >= 3.8) - Reformat `llvm_ext.cc` to conform to Crystal formatter

ggiraldez · 2016-10-24T22:08:43Z

Agreed, the LLVM documentation is a bit hard to browse through. I'm a LLVM noob myself. Mostly, I've been referring to the source code directly, but the debugging chapter from the Kaleidoscope tutorial provided a nice introduction.

Anyway, I updated the opening comment explaining some of the changes.

ysbaddaden · 2016-10-25T09:26:44Z

Thank you! This looks all good to me, Travis is happy, and it should still compile against LLVM 3.6. @asterite what do you think?

asterite · 2016-10-25T10:49:02Z

Looks really good to me! Merge? :-) (or rebase?)

ysbaddaden · 2016-10-25T11:45:12Z

@ggiraldez we can't compile llvm_ext.cc with LLVM 3.5 anymore. If it fixable?

It happened in 0ca0868

ggiraldez · 2016-10-25T13:09:44Z

Heh, I just opened #3472 but I see you already fixed it in #3471 :) I think I like your PR better, feel free to discard mine.

ggiraldez added 2 commits October 24, 2016 13:46

Compile C/C++ extensions with debug information

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

3e8bfd1

ysbaddaden approved these changes Oct 24, 2016

View reviewed changes

ggiraldez force-pushed the feature/improve-debugging branch from 218f974 to 0ca0868 Compare October 24, 2016 21:48

ysbaddaden merged commit ac92a36 into crystal-lang:master Oct 25, 2016

ysbaddaden added this to the 0.20.0 milestone Oct 25, 2016

ggiraldez deleted the feature/improve-debugging branch October 27, 2016 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and improvements for debugging output #3469

Fixes and improvements for debugging output #3469

ggiraldez commented Oct 24, 2016 •

edited

Loading

ysbaddaden left a comment

ysbaddaden Oct 24, 2016

ggiraldez Oct 24, 2016

ysbaddaden Oct 24, 2016

ysbaddaden Oct 24, 2016

ggiraldez Oct 24, 2016

ysbaddaden Oct 24, 2016

ggiraldez Oct 24, 2016 •

edited

Loading

ggiraldez commented Oct 24, 2016

ysbaddaden commented Oct 25, 2016 •

edited

Loading

asterite commented Oct 25, 2016

ysbaddaden commented Oct 25, 2016 •

edited

Loading

ggiraldez commented Oct 25, 2016

Fixes and improvements for debugging output #3469

Fixes and improvements for debugging output #3469

Conversation

ggiraldez commented Oct 24, 2016 • edited Loading

ysbaddaden left a comment

Choose a reason for hiding this comment

ysbaddaden Oct 24, 2016

Choose a reason for hiding this comment

ggiraldez Oct 24, 2016

Choose a reason for hiding this comment

ysbaddaden Oct 24, 2016

Choose a reason for hiding this comment

ysbaddaden Oct 24, 2016

Choose a reason for hiding this comment

ggiraldez Oct 24, 2016

Choose a reason for hiding this comment

ysbaddaden Oct 24, 2016

Choose a reason for hiding this comment

ggiraldez Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

ggiraldez commented Oct 24, 2016

ysbaddaden commented Oct 25, 2016 • edited Loading

asterite commented Oct 25, 2016

ysbaddaden commented Oct 25, 2016 • edited Loading

ggiraldez commented Oct 25, 2016

ggiraldez commented Oct 24, 2016 •

edited

Loading

ggiraldez Oct 24, 2016 •

edited

Loading

ysbaddaden commented Oct 25, 2016 •

edited

Loading

ysbaddaden commented Oct 25, 2016 •

edited

Loading