-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
JIT: IP mapping for method doesn't contain IL offset corresponding to the one of the sequence points #9064
Comments
@dotnet/jit-contrib |
The last sequence point corresponds to return statement that was deleted during returns merging:
Block 22:
Maybe we should to disable returns merging for debug mode. What is the proper way to fix it? |
@dotnet/jit-contrib @JosephTremoulet |
We sometimes must merge returns for correctness, and have been doing so since before dotnet/coreclr#13792. Evidently, we were doing it in a way that preserved the sequence points before, and that's what dotnet/coreclr#13792 broke. In particular, dotnet/coreclr#13792 added a code path that separates out returns of constants, and that new path must be dropping the sequence point. Note that we have an open question/issue about what's the best behavior for this return merging/separation in minopts and debug (#13915). I'm not sure exactly what IR constraints "preserve the sequence points" translates to, presumably someone on @dotnet/jit-contrib does. So possibly one option is to enforce those constraints on the separate-constant-return path. And I believe the logic for creating non-constant merged returns was refactored but largely unchanged, so another option may be bypassing the constant return check in debug, so that debug always does the temp-introducing return merging that has been preserving sequence points correctly. |
@JosephTremoulet thank you for response! I think it's better to figure out how JIT guarantees sequence point preserving (especially for return blocks) and apply this approach in the separate-constant-return path. And if it's impossible we can use second option and bypass the constant return check in debug. |
Could anyone explain how JIT guarantees sequence point preserving (especially for return blocks)? Thanks! |
Generally speaking, when generating debuggable code, the jit will not do any optimizations or flow transformations. So the sequence point information that is captured initially when the jit IR is created is preserved in the IR throughout compilation. However return merging is a special case as the jit must also honor the constraints imposed by the runtime. So for this case some flow transformation may be required even when generating debuggable code. Most returns logically split into two parts: assign the return value, then execute the epilog. The runtime constraint is on the number of epilogs. So the general idea is that the return sequence point associates with the return value assignment, which remain distinct, one per return point. Then the number of epilogs can be altered to suit the constraints of the runtime. Likely the recent change to merge returns of constants effectively broke this one-to-one mapping and so some return sequence points can get lost. The fix is likely to disable the same return value merging for debuggable codegen. |
@AndyAyersMS Thank you for explanation! |
If we merge constant returns into a common point we may lose track of sequence points. So inhibit this when we are generating debuggable code. Fixes #14339.
If we merge constant returns into a common point we may lose track of sequence points. So inhibit this when we are generating debuggable code. Fixes #14339.
For following example:
pdb contains following sequence points for
MoveNext
method:But IP mapping for compiled
MoveNext
method doesn't contain IL offset for the last sequence point (at the line 25):IL code of the
MoveNext
method is following:During compilation statement corresponding last sequence point was removed:
I've fugured out that this issue was introduced by dotnet/coreclr#13792
Before this changes IP mapping contains all sequence points:
The text was updated successfully, but these errors were encountered: