This is an archive of the discontinued LLVM Phabricator instance.

Adjust some id bit shifts to fit inside 32 bit integers
ClosedPublic

Authored by lanza on Oct 23 2018, 11:58 AM.

Details

Summary

The DAP on vscode uses a JavaScript number for identifiers while the
Visual Studio version uses a C# Int for identifiers. lldb-vscode is
bit shifting identifiers 32 bits to form a unique ID. Change this to
a 16 bit shift to allow the data to fit into 32 bits.

Event Timeline

lanza created this revision.Oct 23 2018, 11:58 AM

There should be quite a few more things switched to 32bit values to account for this. I've added that to my tasks. Putting this up just because this breaks functionality on Visual Studio.

clayborg requested changes to this revision.Oct 23 2018, 3:39 PM

This won't work when we have many frames. If you get a stack trace that blows the stack, you can easily have 300K stack frames. Thread index ID can get quite high too. There are also no lifetimes on the frame IDs we use, so we never get told to free a frame ID when the IDE is done with one. So we either have to switch to using a map that can map between frame ID and lldb::SBFrame or let the map just grow and grow, or something else. This isn't sufficient.

This revision now requires changes to proceed.Oct 23 2018, 3:39 PM
lanza added a comment.Nov 5 2018, 6:44 PM

@clayborg We can either store a dap-frame-to-lldb-frame map as well as a lldb-frame-to-dap-frame map or we can truncate the thread count down to a much smaller count of bits. I think the former is an ugly idea to just let a pair of gigantic maps grow endlessly over the lifetime of a program. E.G. for debugging some game that could be running for a long duration. For the latter, I haven't seen a thousand thread program before and if you're using a thousand threads you probably have much bigger problems than your GUI debugger. How about a split with 10 or so bits for the thread ID and the remaining 22 bits for the frame ID?

As per the breakpoint situation, I think we could do something similar with the breakpoint ID taking 10 bits and the BreakpointLocation ID taking 22 bits since a user likely isn't going to provide 1000 chosen breakpoints anyways.

@clayborg We can either store a dap-frame-to-lldb-frame map as well as a lldb-frame-to-dap-frame map or we can truncate the thread count down to a much smaller count of bits. I think the former is an ugly idea to just let a pair of gigantic maps grow endlessly over the lifetime of a program. E.G. for debugging some game that could be running for a long duration. For the latter, I haven't seen a thousand thread program before and if you're using a thousand threads you probably have much bigger problems than your GUI debugger. How about a split with 10 or so bits for the thread ID and the remaining 22 bits for the frame ID?

2^19 = 524288
2^13 = 8192

Not many OSs can get a half million stack frames without running out of stack. So how about 19 bits for frame ID and 13 for the thread ID?

As per the breakpoint situation, I think we could do something similar with the breakpoint ID taking 10 bits and the BreakpointLocation ID taking 22 bits since a user likely isn't going to provide 1000 chosen breakpoints anyways.

Sounds good

lanza updated this revision to Diff 172841.Nov 6 2018, 1:33 PM

adjust the shifts

lanza updated this revision to Diff 172843.Nov 6 2018, 1:39 PM

formating

clayborg added inline comments.Nov 6 2018, 2:17 PM
tools/lldb-vscode/LLDBUtils.cpp
68–70

It would be nice to make all encoding and decoding into functions here. I would suggest also making:

#define THREAD_INDEX_SHIFT 19
int64_t MakeVSCodeFrameID(lldb::SBFrame &frame) {
  return (int64_t)(frame.GetThread().GetIndexID() << THREAD_INDEX_SHIFT | frame.GetFrameID());
}
uint32_t GetLLDBThreadIndexID(int64_t frame_id) {
  return frame_id >> THREAD_INDEX_SHIFT;
}
uint32_t GetLLDBFrameIndex(int64_t frame_id) {
  return frame_id & ((1u << THREAD_INDEX_SHIFT) - 1);
}
tools/lldb-vscode/VSCode.cpp
311
lldb::SBThread thread = process.GetThreadByIndexID(GetLLDBThreadIndexID(frame_id));
313
return thread.GetFrameAtIndex(GetLLDBFrameIndex(frame_id));
lanza added a comment.Nov 6 2018, 5:55 PM

Am I missing some line of code or does lldb-vscode not ever map from the DAP breakpoint ID back to the LLDB BreakpointLocation and Breakpoint IDs? I wrote the equivalent functions but there doesn't seem to be anywhere that maps in the inverse direction.

lanza updated this revision to Diff 172885.Nov 6 2018, 6:00 PM

add functions that handle the bit shifting

lanza updated this revision to Diff 172887.Nov 6 2018, 6:04 PM

clang-format

Am I missing some line of code or does lldb-vscode not ever map from the DAP breakpoint ID back to the LLDB BreakpointLocation and Breakpoint IDs? I wrote the equivalent functions but there doesn't seem to be anywhere that maps in the inverse direction.

Yeah, it is not ever mapped back. I believe the ID is sent back later, but it isn't used right now. When you have breakpoints set in a source file by file and line, they send one breakpoint request with multiple lines, and when a breakpoint is removed, they just send another packet with all source file and line breakpoints and you are expected to just "figure it out". Kind of lame, but it is what we have.

tools/lldb-vscode/LLDBUtils.cpp
68

"static constexpr" if we aren't going to use a #define statement

68–69

We should rename this to MakeDAPFrameID to match other functions.

clayborg requested changes to this revision.Nov 7 2018, 7:24 AM

Just rename MakeVSCodeFrameID to MakeDAPFrameID, or rename all MakeDAPXXX functions you added to be MakeVSCodeXXX.

This revision now requires changes to proceed.Nov 7 2018, 7:24 AM
clayborg accepted this revision.Nov 7 2018, 10:57 AM
This revision is now accepted and ready to land.Nov 7 2018, 10:57 AM
This revision was automatically updated to reflect the committed changes.