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

debug: don't set end_location at function exit #4972

Merged
merged 1 commit into from Sep 15, 2017
Merged

debug: don't set end_location at function exit #4972

merged 1 commit into from Sep 15, 2017

Conversation

asterite
Copy link
Member

@asterite asterite commented Sep 14, 2017

The codegen was setting the end location of a method for the last return LLVM instruction.

So for example for this code:

def foo
  bar
end

def bar
  a = [1]
  a[2]
end

foo

we would get:

Index out of bounds (IndexError)
0x10d77753e: at at /usr/local/Cellar/crystal-lang/0.23.1/src/indexable.cr 0:17
0x10d7774d9: [] at /usr/local/Cellar/crystal-lang/0.23.1/src/indexable.cr 74:5
0x10d75da51: bar at /Users/asterite/Projects/crystal/foo.cr 8:3
0x10d75d9d9: foo at /Users/asterite/Projects/crystal/foo.cr 3:3
0x10d74d807: __crystal_main at /Users/asterite/Projects/crystal/foo.cr 10:1
0x10d75d8b8: main at /usr/local/Cellar/crystal-lang/0.23.1/src/main.cr 12:15

foo.cr at line 3 is the end, but it should be foo.cr at line 2, bar.

The same goes for indexable.cr at line 74, it points to the end where it should point to the last expression (one line above it).

The solution is to leave the debug info for the last return LLVM instruction as the location of the previously executed expression.

The indexable.cr line 0 is still a mistery, but it must be an unrelated issue.


# The first line is the old (incorrect) behaviour,
# the second line is the new (correct) behaviour.
# TODO: keep only the second one after Crystal 0.23.1
Copy link
Contributor

@RX14 RX14 Sep 14, 2017

Choose a reason for hiding this comment

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

Perhaps we could use the crystal compiler programatically to ensure we don't have to handle multiple compiler versions, just master.

Copy link
Member Author

@asterite asterite Sep 14, 2017

Choose a reason for hiding this comment

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

It's a good idea. I'll change it later.

Copy link
Member Author

@asterite asterite Sep 14, 2017

Choose a reason for hiding this comment

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

Actually, I'd prefer now to do that just now. That means the spec will depend on the compiler, and that will slow down std_spec a lot.

@RX14
Copy link
Contributor

@RX14 RX14 commented Sep 14, 2017

This looks like a great fix, but regarding specs I think we should do something like #4718 sooner or later. I think sticking all debuginfo specs inside "callstack" is a bad idea.

@asterite
Copy link
Member Author

@asterite asterite commented Sep 14, 2017

@RX14 I agree. I just fixed the spec because it was failing. It's just coincidence that there's a spec for this (I'm sure it's testing the callstack, not the debug info location)

@ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Sep 15, 2017

Yes, the spec is testing that the DWARF information (function name, file:line:colum) is correctly parsed and printed, so we don't have regressions, and we get a failure when porting to an unsupported system (can't parse executable, failed to find/parse DWARF).

RX14
RX14 approved these changes Sep 15, 2017
@RX14 RX14 added this to the Next milestone Sep 15, 2017
@RX14 RX14 merged commit c0cac8d into crystal-lang:master Sep 15, 2017
2 checks passed
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.

None yet

3 participants