Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attach debug locations to generated internal LLVM functions #10934

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Jul 13, 2021

Given the following:

# test.cr

FOO = begin
  x = 1
end

FOO

This PR changes the generated LLVM IR from:

define internal void @"~FOO:init"() {
alloca:
  %x = alloca i32, align 4
  br label %entry

entry:                                            ; preds = %alloca
  store i32 1, i32* %x, align 4
  ret void
}

to:

define internal void @"~FOO:init"() !dbg !15 {
alloca:
  %x = alloca i32, align 4, !dbg !16
  br label %entry

entry:                                            ; preds = %alloca
  store i32 1, i32* %x, align 4, !dbg !17
  ret void, !dbg !17
}

!11 = !DIFile(filename: "test.cr", directory: "...")
!15 = distinct !DISubprogram(name: "~FOO:init", linkageName: "~FOO:init", scope: !11, file: !11, line: 3, type: !6, scopeLine: 3, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)
!16 = !DILocation(line: 3, column: 1, scope: !15)
!17 = !DILocation(line: 4, column: 3, scope: !15)

This means theoretically it would be possible to debug through those complex constant initializers. The same goes for class variables.

Originally this PR covered only constant and class variables, but now it should include every generated internal LLVM function, e.g. ~metaclass and ~match. Ones that have no real location are attached to ??:1:1.

Another use case is that Compiler Explorer relies on .loc directives of the generated assembly files to properly filter "library code". Applying this PR reduces the number of ASM lines of an empty Crystal source file from around 13.3k to 4.1k, mainly because the huge initializer for String::CHAR_TO_DIGIT is now gone, i.e. regarded as library code originating from src/string.cr. (Yes, support for Crystal in the Compiler Explorer went live.)

@HertzDevil HertzDevil changed the title Attach debug location to constant and class variable initializers Attach debug locations to constant and class variable initializers Jul 13, 2021
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jul 13, 2021

I have no idea why Windows CI breaks without printing anything. It seems to be undefined behaviour on LLVM's side if a debug location isn't attached before the call to alloca_vars (or generally the first instruction in any such newly created LLVM function).

@HertzDevil HertzDevil marked this pull request as ready for review July 14, 2021 01:40
@HertzDevil HertzDevil changed the title Attach debug locations to constant and class variable initializers Attach debug locations to constant and class variable initializers + getters Jul 14, 2021
@HertzDevil HertzDevil changed the title Attach debug locations to constant and class variable initializers + getters Attach debug locations to generated internal LLVM functions Jul 17, 2021
@beta-ziliani
Copy link
Member

@HertzDevil this looks good, yet I have the following questions if you can spare some time to clear them:

  1. I can't find the connection between this PR and the shrinking in generated code. How does it affect code generation, besides the debugging information?

  2. I agree with @Sija that location 0, 0 is a better marker for an unknown position. What's your take on the matter?

Thanks!

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Aug 3, 2021

  1. It should not affect code generation besides the extra debug information. It does not make the LLVM IR or disassembly smaller; the full disassembly of an empty Crystal file still has around 190k lines, it is only Compiler Explorer that leverages the debug information to filter lines if appropriate options are enabled there.

  2. I have no strong opinion over this. However if the fictitious location turns out to have a "real" filename then that could affect source colorization on Compiler Explorer.

@beta-ziliani
Copy link
Member

I see, thanks! This is ready then (we're seeing some fluke in CI, but that's unrelated).

@beta-ziliani beta-ziliani added this to the 1.2.0 milestone Aug 3, 2021
@straight-shoota straight-shoota merged commit 606ea3b into crystal-lang:master Aug 4, 2021
@HertzDevil HertzDevil deleted the feature/const-init-debug-loc branch August 4, 2021 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants