Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[Linux][GDB-JIT] Fix bugs in gdbjit that break lldb stepping #8637

Merged
merged 6 commits into from
Dec 19, 2016
Merged

[Linux][GDB-JIT] Fix bugs in gdbjit that break lldb stepping #8637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
679bbdc
Fix .text and .thunk symbols overlapping
ayuckhulk Nov 25, 2016
c6d8104
Fix __thunk symbols containing garbage
ayuckhulk Dec 13, 2016
0f11960
Do not create DWARF line table entries with the same address
ayuckhulk Nov 25, 2016
8dc2527
For each HiddenLine assign a zero line number in DWARF
ayuckhulk Dec 5, 2016
6dfdce6
Fix missing check for the end of list of compiled methods
ayuckhulk Dec 14, 2016
bde801d
Remove unnecessary check for zero prevLine in gdbjit
ayuckhulk Dec 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions src/vm/gdbjit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1432,13 +1432,11 @@ void NotifyGdb::MethodCompiled(MethodDesc* MethodDescPtr)
CodeHeader* pCH = (CodeHeader*)pCode - 1;
CalledMethod* pCalledMethods = reinterpret_cast<CalledMethod*>(pCH->GetCalledMethods());
/* Collect addresses of thunks called by method */
if (!CollectCalledMethods(pCalledMethods))
if (!CollectCalledMethods(pCalledMethods, (TADDR)MethodDescPtr->GetNativeCode()))
{
return;
}
pCH->SetCalledMethods(NULL);
if (!codeAddrs.Contains((TADDR)pCode))
codeAddrs.Add((TADDR)pCode);

MetaSig sig(MethodDescPtr);
int nArgsCount = sig.NumFixedArgs();
Expand Down Expand Up @@ -1886,18 +1884,17 @@ static void fixLineMapping(SymbolsInfo* lines, unsigned nlines)
int prevLine = 0;
for (int i = 0; i < nlines; ++i)
{
if (lines[i].lineNumber == HiddenLine)
continue;
if (lines[i].ilOffset == ICorDebugInfo::PROLOG) // will be fixed in next step
{
prevLine = 0;
}
else
{
if (lines[i].lineNumber == 0 || lines[i].lineNumber == HiddenLine)
if (lines[i].lineNumber == 0)
{
if (prevLine != 0)
{
lines[i].lineNumber = prevLine;
}
lines[i].lineNumber = prevLine;
}
else
{
Expand All @@ -1909,11 +1906,23 @@ static void fixLineMapping(SymbolsInfo* lines, unsigned nlines)
prevLine = lines[nlines - 1].lineNumber;
for (int i = nlines - 1; i >= 0; --i)
{
if (lines[i].lineNumber == 0 || lines[i].lineNumber == HiddenLine)
if (lines[i].lineNumber == HiddenLine)
continue;
if (lines[i].lineNumber == 0)
lines[i].lineNumber = prevLine;
else
prevLine = lines[i].lineNumber;
}
// Skip HiddenLines
for (int i = 0; i < nlines; ++i)
{
if (lines[i].lineNumber == HiddenLine)
{
lines[i].lineNumber = 0;
if (i + 1 < nlines && lines[i + 1].ilOffset == ICorDebugInfo::NO_MAPPING)
Copy link
Member

Choose a reason for hiding this comment

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

In which case is a line with ilOffset == ICorDebugInfo::NO_MAPPING right after a hidden line?

Copy link
Author

Choose a reason for hiding this comment

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

I saw this in generated code for try {...}, catch {...} finally {...}.

Copy link
Member

Choose a reason for hiding this comment

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

What would happen from the debugging experience point of view if we did not have this if here?

Copy link
Author

Choose a reason for hiding this comment

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

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 {...}.

lines[i + 1].lineNumber = 0;
}
}
}

/* Build program for DWARF source line section */
Expand Down Expand Up @@ -1950,6 +1959,13 @@ bool NotifyGdb::BuildLineProg(MemBuf& buf, PCODE startAddr, TADDR codeSize, Symb
prevFile = lines[i].fileIndex;
}

// GCC don't use the is_prologue_end flag to mark the first instruction after the prologue.
Copy link
Member

Choose a reason for hiding this comment

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

A nit - we don't build with GCC, we use Clang

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I've misread the comment a bit.

// 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.
// We do not want to confuse the debugger so we have to avoid adding a line in such case.
if (i > 0 && lines[i - 1].nativeOffset == lines[i].nativeOffset)
continue;

IssueSetAddress(ptr, startAddr + lines[i].nativeOffset);

if (lines[i].lineNumber != prevLine) {
Expand Down Expand Up @@ -2139,10 +2155,13 @@ bool NotifyGdb::BuildDebugPub(MemBuf& buf, const char* name, uint32_t size, uint
}

/* Store addresses and names of the called methods into symbol table */
bool NotifyGdb::CollectCalledMethods(CalledMethod* pCalledMethods)
bool NotifyGdb::CollectCalledMethods(CalledMethod* pCalledMethods, TADDR nativeCode)
{
AddrSet tmpCodeAddrs;

if (!codeAddrs.Contains(nativeCode))
codeAddrs.Add(nativeCode);

CalledMethod* pList = pCalledMethods;

/* count called methods */
Expand All @@ -2159,7 +2178,8 @@ bool NotifyGdb::CollectCalledMethods(CalledMethod* pCalledMethods)
SymbolNames = new (nothrow) Elf_Symbol[SymbolCount];

pList = pCalledMethods;
for (int i = 1 + method.GetCount(); i < SymbolCount;)
int i = 1 + method.GetCount();
while (i < SymbolCount && pList != NULL)
{
TADDR callAddr = (TADDR)pList->GetCallAddr();
if (!codeAddrs.Contains(callAddr))
Expand All @@ -2178,7 +2198,7 @@ bool NotifyGdb::CollectCalledMethods(CalledMethod* pCalledMethods)
pList = pList->GetNext();
delete ptr;
}

SymbolCount = i;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! It seems that I forgot to add check for pList != NULL.

return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/vm/gdbjit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ class NotifyGdb
static void IssueSimpleCommand(char*& ptr, uint8_t command);
static void IssueParamCommand(char*& ptr, uint8_t command, char* param, int param_len);
static void SplitPathname(const char* path, const char*& pathName, const char*& fileName);
static bool CollectCalledMethods(CalledMethod* pCM);
static bool CollectCalledMethods(CalledMethod* pCM, TADDR nativeCode);
#ifdef _DEBUG
static void DumpElf(const char* methodName, const MemBuf& buf);
#endif
Expand Down