Skip to content
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

Reenable callback on |break| instruction #216

Merged
merged 13 commits into from Oct 2, 2018

Conversation

peace-maker
Copy link
Member

@peace-maker peace-maker commented May 27, 2018

Reenable the IPluginContext::SetDebugBreak Add ISourcePawnEngine::SetDebugBreakHandler to allow others to listen for every break instruction in the code stream. Add functions to IPluginDebugInfo to allow finding the address of functions and lines in plugins for break points.

This doesn't expose any local nor global variable information yet, so debugging capabilities are pretty limited. You can print which file and line the plugin is about to execute currently.

This is partly extracted from #51.

Call the given callback on every `dbreak` instruction in the code stream - which is emitted for every line break in the code.
Don't pass the state in multiple variables, but use a struct to hold the VM state.
Expose the file names of the source files.
Keep it as simple as possible.
To be able to set breakpoints we need to be able to get the cip of the function or line we want to break on.
Lookup the function address from the rtti.methods table if available.
Copy link
Member

@dvander dvander left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small things.

SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_t* funcaddr)
{
uint32_t index = 0;
const char* tgtfile;
Copy link
Member

Choose a reason for hiding this comment

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

nit: move tgtfile down to where it's actually used. Same for index. (We're not using C :p)

vm/x86/jit_x86.cpp Show resolved Hide resolved
vm/x86/jit_x86.cpp Show resolved Hide resolved
The stack should be aligned to 16 bytes before a call.
Simplify the API to only allow one debug break handler for all plugins instead of maintaining different ones per runtime.
ISourcePawnEnvironment::EnableDebugBreak() has to be called before any plugins are loaded to have the call to the debug break handler compiled into the jitted code of the plugins.

This is disabled by default, so it has no impact on performance.
Copy link
Member

@dvander dvander left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few comments and nits.

*/
typedef struct sp_debug_break_info_s
{
uint16_t version; /**< Version of this struct */
Copy link
Member

Choose a reason for hiding this comment

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

How is this version field supposed to be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

To know whether new fields are available. When adding more info to the struct, like maybe a pointer to the smx in memory to parse the debug symbols manually, allow extensions know which fields they can use.

Probably could get rid of this seperate version field and just increase the whole vm api version if it's changed. Extensions just don't have access to SOURCEPAWN_API_VERSION.

vm/debugging.cpp Outdated
void InvokeDebugger(PluginContext* ctx, const IErrorReport* report)
{
// Ignore any calls if this isn't enabled.
if (!Environment::get()->IsDebugBreakEnabled())
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to get here if the debug break is not enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, through the interpreter.

// now find the first line in the function where we can "break" on
uint32_t index = 0;
for (; index < debug_info_->num_lines && debug_lines_[index].addr < *funcaddr; index++)
/* nothing */;
Copy link
Member

Choose a reason for hiding this comment

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

nit: prefer {} for empty statements, or continue

for (file = 0; file < debug_info_->num_files; file++) {
// find the (next) matching instance of the file
if (debug_files_[file].name >= debug_names_section_->size ||
strcmp(debug_names_ + debug_files_[file].name, filename) != 0)
Copy link
Member

Choose a reason for hiding this comment

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

nit: indent strcmp to line up with debug_files_

SmxV1Image::LookupLineAddress(const uint32_t line, const char* filename, uint32_t* addr)
{
/* Find a suitable "breakpoint address" close to the indicated line (and in
* the specified file). The address is moved up to the next "breakable" line
Copy link
Member

Choose a reason for hiding this comment

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

nit: Line up * or use C++-style comments


// browse until the line is found or until the top address is exceeded
while (index < debug_info_->num_lines &&
debug_lines_[index].line < line &&
Copy link
Member

Choose a reason for hiding this comment

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

nit: Line up debug_lines_ with index

No need to check it again when coming from the JIT path, because the debugger wouldn't be called at all if it wasn't enabled.
@dvander dvander merged commit 5511c10 into alliedmodders:master Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants