[Linux][GDB-JIT] Fix bugs in gdbjit that break lldb stepping#8637
[Linux][GDB-JIT] Fix bugs in gdbjit that break lldb stepping#8637janvorli merged 6 commits intodotnet:masterfrom
Conversation
When current method calls itself, a __thunk* symbol might be generated with the same address as the method symbol in .text section. Avoid generating such __thunk* symbol.
Fix a bug when __thunk* symbols of previously compiled methods cause generation of __thunk* symbols for currently compiled method without filling symbol info.
Allow LLDB to to skip HiddenLines when stepping.
| delete ptr; | ||
| } | ||
|
|
||
| SymbolCount = i; |
There was a problem hiding this comment.
I don't understand the reason for this change. The loop will always exit when i is equal to the SymbolCount, so this assignment doesn't make sense. It seems the previous version of the code would have the same effect.
There was a problem hiding this comment.
Thanks! It seems that I forgot to add check for pList != NULL.
| prevFile = lines[i].fileIndex; | ||
| } | ||
|
|
||
| // GCC don't use the is_prologue_end flag to mark the first instruction after the prologue. |
There was a problem hiding this comment.
A nit - we don't build with GCC, we use Clang
There was a problem hiding this comment.
If LLDB sees two lines with the same native address, it thinks that the code was compiled by GCC and sets is_prologue_end flag. We want to avoid such random prologue end in the middle of a function.
There was a problem hiding this comment.
Ok, I've misread the comment a bit.
src/vm/gdbjit.cpp
Outdated
| if (lines[i].lineNumber == 0 || lines[i].lineNumber == HiddenLine) | ||
| if (lines[i].lineNumber == 0) | ||
| { | ||
| if (prevLine != 0) |
There was a problem hiding this comment.
This if is not necessary. If you leave it out, if the prevLine was 0, it would assign zero to the lines[i].lineNumber and since it was already zero, it would not change.
| if (lines[i].lineNumber == HiddenLine) | ||
| { | ||
| lines[i].lineNumber = 0; | ||
| if (i + 1 < nlines && lines[i + 1].ilOffset == ICorDebugInfo::NO_MAPPING) |
There was a problem hiding this comment.
In which case is a line with ilOffset == ICorDebugInfo::NO_MAPPING right after a hidden line?
There was a problem hiding this comment.
I saw this in generated code for try {...}, catch {...} finally {...}.
There was a problem hiding this comment.
What would happen from the debugging experience point of view if we did not have this if here?
There was a problem hiding this comment.
When stepping past catch{...} block LLDB will stop at the end of try{...} and and only than go to the start of finally{...}. Bhis change allows lldb to skip that unnecessary stop at the end of try {...}.
|
@janvorli Thank you for the review! |
|
@dotnet-bot test Linux ARM Emulator Cross Debug Build |
|
@dotnet-bot test Windows_NT x86 Checked Build and Test |
|
@janvorli Is this PR ready to be merged? |
|
Thank you! |
This PR fixes a number of bugs in GDBJIT that sometimes break LLDB stepping:
When current method calls itself, a __thunk* symbol might be generated with the same address as the method symbol in .text section. Avoid generating such __thunk* symbol.
When __thunk* symbols of previously compiled methods cause generation of __thunk* symbols for currently compiled method without filling symbol info.
GCC don't use the is_prologue_end flag to mark the first instruction after the prologue. Instead of it it is issueing a line table entry for the first instruction of the prologue and one for the first instruction after the prologue.
@mikem8361, @janvorli PTAL
cc @Dmitri-Botcharnikov @chunseoklee @seanshpark @lucenticus