Missing !dbg locations in generated LLVM IR #3904
Conversation
|
I can also build/run the compiler and std specs with those fixes. Hopefully, there aren't that much more cases...? |
| @@ -638,7 +638,7 @@ module Crystal | |||
| end | |||
| end | |||
|
|
|||
| Call.new(cond, "===", right_side).at(obj) | |||
| Call.new(cond, "===", right_side) | |||
There was a problem hiding this comment.
Quick question: why is this no longer needed? (asking just out of curiosity, as I'm not particularly familiar with this part of the compiler)
There was a problem hiding this comment.
Other paths don't set it, and we always override it in expand.
|
@ysbaddaden the build seems to be failing. I think that after that's fixed we are good to merge this PR. |
ysbaddaden
force-pushed
the
fix-missing-dbg-locations
branch
from
3cadfb5 to
35788a8
Compare
|
I restarted the build. I was unable to repro the failures even with jhass docker image used in ci. Note: jhass docker images use LLVM 3.8 |
| @@ -103,7 +103,7 @@ module Crystal | |||
| break if end_token? | |||
| end | |||
|
|
|||
| Expressions.from(exps) | |||
| Expressions.from(exps).at(exps.first) | |||
There was a problem hiding this comment.
CI fails because of this patch.
The root problem is that the following:
def foo
rescue # or ensure
endIs being expanded to:
def foo
begin # <= missing location
rescue
end
endbut the generated begin block is missing a location, then LLVM 3.9 module validation fails because some calls to fun like __crystal_get_exception or __crystal_raise are missing a !dbg location. The above fixes the bug, by making sure the body has a position (set to be the one of its first expression).
- Maybe that should be done at some other place? Like at the place where the
beginblock is injected. The problem is, I have no idea where :'( - Maybe we should fix the failing cases?
I don't know what to do, here.
There was a problem hiding this comment.
Note that I reproduce the CI failure locally (Ubuntu 14.04, LLVM 3.9 apt repository).
There was a problem hiding this comment.
The specs seem to get fixed with this patch:
diff --git a/src/compiler/crystal/syntax/ast.cr b/src/compiler/crystal/syntax/ast.cr
index 687d6df..1cd9538 100644
--- a/src/compiler/crystal/syntax/ast.cr
+++ b/src/compiler/crystal/syntax/ast.cr
@@ -138,6 +138,10 @@ module Crystal
@expressions.last
end
+ def location
+ @location || @expressions.first?.try &.location
+ end
+
def end_location
@end_location || @expressions.last?.try &.end_location
end
diff --git a/src/compiler/crystal/syntax/parser.cr b/src/compiler/crystal/syntax/parser.cr
index 127ab72..1d3883e 100644
--- a/src/compiler/crystal/syntax/parser.cr
+++ b/src/compiler/crystal/syntax/parser.cr
@@ -103,7 +103,7 @@ module Crystal
break if end_token?
end
- Expressions.from(exps).at(exps.first)
+ Expressions.from(exps)
end
def parse_multi_assignExpressions already had an overload for end_location that returned that of the last expression, but we were missing one for location as well.
I didn't check if it still fixed the missing !dbg issue, but I guess it does.
C struct setters automatically convert the assigned value, but failed to provide a location for the conversion (the setters are automatically generated).
ysbaddaden
force-pushed
the
fix-missing-dbg-locations
branch
from
35788a8 to
4a71729
Compare
|
@asterite you nailed it! Now I can build Crystal in release mode with llvm 3.9 and specs are passing. |
|
Awesome! This deserves a release ;-) |
This allows to compile the Crystal compiler itself using Crystal 0.20.4 with the default debug configuration (always generate line numbers) against LLVM 3.9, which requires a
!dbglocation to be defined for every inlinable calls (among other cases).I separated each commit, so each correction can be reviewed more easily.
fixes #3890