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

Auxtools debugger #230

Merged
merged 31 commits into from
Nov 28, 2020
Merged

Auxtools debugger #230

merged 31 commits into from
Nov 28, 2020

Conversation

willox
Copy link
Contributor

@willox willox commented Nov 26, 2020

This adds support for a debug server written in auxtools (currently located at https://github.com/willox/auxtools.)

The dependency is similar to extools, where SpacemanDMM's auxtools_types.rs has to be up-to-date with the server_types.rs file located in whichever version of debug server is used. In the future this could change to be some shared dependency, or maybe the debug server could just be moved into SpacemanDMM.

There's a bunch of repeated code in mod.rs where there's match statements where one branch is for the extools client and one branch is for auxtools client. It's a bit iffy, but they all have minor differences and wouldn't be super easy to merge.

I accidentally ran a cargo fmt on the files I was working with at some point, so there's a few formatting changes about. I don't think it's too much to read over.

I haven't edited any documentation yet, so here's how it works:
In your DM project:

// Currently needed for auxtools' error reporting. TG code already has this defined.
/proc/stack_trace(msg)
	CRASH(msg)

/proc/enable_debugging(mode, port)
	CRASH("auxtools not loaded")

/world/New()
	var/debug_server = world.GetConfig("env", "AUXTOOLS_DEBUG_DLL")
	if (debug_server)
		call(debug_server, "auxtools_init")()
		enable_debugging()
	. = ..()

/world/Del()
	var/debug_server = world.GetConfig("env", "AUXTOOLS_DEBUG_DLL")
	if (debug_server)
		call(debug_server, "auxtools_shutdown")()
	. = ..()

In your project's SpacemanDMM.toml

[debugger]
engine = "auxtools"

The extension doesn't have a way to override the DLL being used (and I don't think it should), so if you're testing stuff I suggest you set the env vars to something like below and use the attach mode:

AUXTOOLS_DEBUG_DLL=path_to_your_build
AUXTOOLS_DEBUG_MODE=BLOCK

LMK what needs changing

@@ -5,14 +5,15 @@ authors = ["Tad Hardesty <tad@platymuus.com>"]
edition = "2018"

[[bin]]
name = "dm-langserver"
name = "dm_langserver"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was changed to avoid a rust bug where symbols are not generated (at least on Windows with MSVC) if a dash is in the name

Copy link
Owner

Choose a reason for hiding this comment

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

is there an upstream ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for this specific issue idk, it was a problem before but got 'fixed'.
rust-lang/cargo#8123
perhaps it was only fixed when there was no [[bin]].name override.

Copy link
Owner

Choose a reason for hiding this comment

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

I will probably revert this in the name of not having to modify my deploy scripts, but I suggest filing an up-to-date report upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine. It's more that I forgot to remove it first, but it was worth pointing out anyway.

@SpaceManiac
Copy link
Owner

The extension doesn't have a way to override the DLL being used (and I don't think it should)

The extension currently has a hidden setting (not exposed in the UI) to override the extools DLL for testing purposes, would the same work here?

@willox
Copy link
Contributor Author

willox commented Nov 26, 2020

Yeah, I haven't added the equivalent for auxtools because I don't see it actually helping me. There was maybe a reason for people to use it before when developing their own extensions onto extools but with auxtools we can have multiple separate libraries loaded at once.

We can add it, but changing that option just seems more complicated than setting the env-var.

Copy link
Owner

@SpaceManiac SpaceManiac left a comment

Choose a reason for hiding this comment

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

Is there any chance that the /world/Del hook can be stuffed into the .dll itself rather than seeping into DM? What about stack_trace?

we can have multiple separate libraries loaded at once.

Can you explain how this works? Would I be shipping a "just the debugger" DLL and require users to install the base separately, or would I be shipping a base+debugger DLL where the base might or might not be the one "in charge"? What are the failure modes?

I accidentally ran a cargo fmt on the files I was working with at some point, so there's a few formatting changes about.

I'll live, but please avoid this in the future. It does add noise to the diff.

src/langserver/debugger/mod.rs Outdated Show resolved Hide resolved
src/langserver/debugger/auxtools_types.rs Outdated Show resolved Hide resolved
src/langserver/debugger/auxtools.rs Outdated Show resolved Hide resolved
src/langserver/debugger/auxtools.rs Outdated Show resolved Hide resolved
src/langserver/debugger/auxtools.rs Show resolved Hide resolved
@SpaceManiac SpaceManiac self-assigned this Nov 26, 2020
@willox
Copy link
Contributor Author

willox commented Nov 26, 2020

Is there any chance that the /world/Del hook can be stuffed into the .dll itself rather than seeping into DM? What about stack_trace?

Doing the cleanup from a hooked proc is a little difficult because the code isn't really in a state where the hooks can be removed. Using the FFI interface was a simple way to get around that. It doesn't just disconnect the debug server, but also gets auxtools into a state where starting another DMB won't fuck up.

Auxtools modules do not unload/reload when restarting the world.

Can you explain how this works? Would I be shipping a "just the debugger" DLL and require users to install the base separately, or would I be shipping a base+debugger DLL where the base might or might not be the one "in charge"? What are the failure modes?

The base is statically linked in. You can just ship the dll and everything will work as long as the SpacemanDMM.toml and DM have the necessary stuff. There's a small performance loss on proc call hooking for each library loaded, but except for when using the debug server I don't expect any code-bases to load more than one at a time.

I didn't mention above, but the call(debug_server, "auxtools_init")() will gracefully return "FAILED (<message here>)" if it fails to load and "SUCCESS" otherwise.

@willox
Copy link
Contributor Author

willox commented Nov 26, 2020

Fixed up the indentation you mentioned, there could easily be more.

@Cyberboss
Copy link
Contributor

Based PR

@ZeWaka
Copy link
Contributor

ZeWaka commented Nov 27, 2020

Based

@willox
Copy link
Contributor Author

willox commented Nov 27, 2020

Is there any chance that the /world/Del hook can be stuffed into the .dll itself rather than seeping into DM? What about stack_trace?

I put a little thought into stack_trace, but we don't really have a way to trigger a runtime in BYOND without calling some sort of proc as auxtools has no way to manually create a new stack frame. If we just cause a runtime to happen at the end of our proc hook, it'll be treated as the runtime occurring in the caller and will cease execution of the proc that is calling us.

@SpaceManiac
Copy link
Owner

Still working through my review of auxtools itself.

We can add [a DLL setting], but changing that option just seems more complicated than setting the env-var.

I'll take care of it myself if I find myself needing it.

except for when using the debug server I don't expect any code-bases to load more than one at a time.

Well, I do have to worry about that situation. Is there a risk of problems if whichever of auxtools-tg.dll or debug_server.dll loads first has an older auxtools bundled? Maybe you can point me to the implementation in auxtools.

debug server written in auxtools (currently located at https://github.com/willox/auxtools.)

Do you intend to maintain it going forward, or would it make sense to move it in-tree with SpacemanDMM eventually?

@willox
Copy link
Contributor Author

willox commented Nov 27, 2020

Well, I do have to worry about that situation. Is there a risk of problems if whichever of auxtools-tg.dll or debug_server.dll loads first has an older auxtools bundled? Maybe you can point me to the implementation in auxtools.

There's no part of auxtools where two modules communicate with each other. We've just been writing it in a way where they won't interfere with each other to the point where it breaks. When a BYOND C-function is being hooked, we make sure to scan for a reference to it instead of for the function itself. That's basically it.

The side-effects should be pretty simple because of that. Stuff like two modules hooking a single proc will result in only the last module to load ever seeing calls to that proc.

I've enforced a pretty simple rule in auxtools where we are only hooking BYOND functions that we absolutely have to. It's one of the older pieces of code, but if you check out hooks.rs's init function you'll see we only hook Runtime and CallProcById.

The biggest risk of backwards incompatibility is if we have a signature in an old version of auxtools that is of a function's contents and a new version of auxtools hooks that function. It's something I intend to just avoid, though. Less hooks = good.

Do you intend to maintain it going forward, or would it make sense to move it in-tree with SpacemanDMM eventually?

I'd expect to keep it working regardless of where it lives. It'd be easier for it to be in this repository, but at this point I'm more interested in getting auxtools on some servers than spending time moving the debug server.

Edit: There was one 'incompatibility' that hasn't been sorted. There's a #[runtime_handler] proc macro that'll trigger on runtimes that are meant to be caught by auxtools modules loaded before yours. Everything works out atm if the debug server loads first.

@SpaceManiac
Copy link
Owner

When a BYOND C-function is being hooked, we make sure to scan for a reference to it instead of for the function itself.

Thanks for the explanation. Sounds like it will keep problems to a minimum and now I can at least start to investigate if something does go wrong.

at this point I'm more interested in getting auxtools on some servers than spending time moving the debug server.

Sure, I was meaning more like after things have stabilized enough that /tg/station has migrated.

Everything works out atm if the debug server loads first.

Will we be okay if the debug server loads second, but whatever loads first doesn't have a #[runtime_handler] fn?

}

DebugClient::Auxtools(_) => {}
Copy link
Owner

Choose a reason for hiding this comment

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

Just a TODO or is there something making this difficult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's just no commands to add yet. You could add a diassemble command fairly easily, but I have only designed our disassembler to produce the correct length for each opcode/operands. The output is mostly correct, but that was a secondary objective.

The disassembler produces more correct output than extools, but it's not formatted. There's still some issues with GetVar (I've only noticed them in (init) methods so far.)

Example output would be:

Dism for Proc(/world/proc/enable_debugger)
	0-1: DbgFile(spaceman_dmm.dm)
	2-3: DbgLine(32)
	4-6: PushVal("env")
	7-9: PushVal("EXTOOLS_DLL")
	10-15: Call(RuntimeProcField(World, [], GetConfig), 2)
	16-18: SetVar(Local(0))
	19-20: DbgLine(33)
	21-23: GetVar(Local(0))
	24-24: Test
	25-26: Jz(38)
	27-28: DbgLine(34)
	29-31: GetVar(Local(0))
	32-34: PushVal("debug_initialize")
	35-36: CallName(ParamCount(0))
	37-37: Pop
	38-38: End

Copy link
Owner

Choose a reason for hiding this comment

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

The formatting doesn't really matter, but the #disassemble command helped me a lot when working on extools.

It's not a priority for this PR, but I wanted to be sure there wasn't anything fundamental blocking it.

The DAP Disassemble request doesn't matter because VSC doesn't implement it.

Copy link
Contributor Author

@willox willox Nov 28, 2020

Choose a reason for hiding this comment

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

I'll add it in when I get around to figuring out what's wrong with (init) method SetVar instructions (it wasn't GetVar, but they have similar operands so the bug probably exists in both.)

For reference, I'm seeing default values of var/list/x = list() being decoded to

NewList(0)
SetVar(Field(Src, [shotgun dart, x]))

I haven't looked into it at all.

@willox
Copy link
Contributor Author

willox commented Nov 28, 2020

Will we be okay if the debug server loads second, but whatever loads first doesn't have a #[runtime_handler] fn?

Nope. It won't crash or anything, but you will see extra runtimes coming from hooked proc calls when there's a runtime caught by our try/catch system.

The reasoning is that the runtime hook is defined as

extern "C" void runtime_hook(char* pError) {
	const char* pErrorCorrected = (pError != nullptr) ? pError : "<null>";
	if (runtime_contexts.top()) {
#ifdef USE_SJLJ
		longjmp(*current_jmp, 1);
#else
		throw AuxtoolsException(pErrorCorrected);
#endif
		return;
	}

	on_runtime(pErrorCorrected);
	return runtime_original(pError);
}

on_runtime is the part that calls into all the Rust runtime handlers. If your library is the second one to load, runtime_original will actually be pointing to the first library's hook. This means that your library will have called on_runtime before giving the first library a chance to catch and re-throw the runtime to itself.

Copy link
Owner

@SpaceManiac SpaceManiac left a comment

Choose a reason for hiding this comment

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

Still some distance from production-ready but this PR is itself mergeable

@SpaceManiac SpaceManiac merged commit 2737e5d into SpaceManiac:master Nov 28, 2020
@SpaceManiac SpaceManiac added this to the suite v1.7 milestone Dec 9, 2020
j-awn pushed a commit to j-awn/SpacemanDMM that referenced this pull request Apr 1, 2021
This adds support for a debug server written in auxtools (currently 
located at https://github.com/willox/auxtools).

The dependency is similar to extools, where SpacemanDMM's 
`auxtools_types.rs` has to be up-to-date with the `server_types.rs` 
file located in whichever version of debug server is used. In the 
future this could change to be some shared dependency, or maybe the 
debug server could just be moved into SpacemanDMM.

There's a bunch of repeated code in `mod.rs` where there's match 
statements where one branch is for the extools client and one branch is 
for auxtools client. It's a bit iffy, but they all have minor 
differences and wouldn't be super easy to merge.

I accidentally ran a `cargo fmt` on the files I was working with at 
some point, so there's a few formatting changes about. I don't think 
it's too much to read over.

I haven't edited any documentation yet, so here's how it works:
In your DM project:
```dm
// Currently needed for auxtools' error reporting. TG code already has this defined.
/proc/stack_trace(msg)
	CRASH(msg)

/proc/enable_debugging(mode, port)
	CRASH("auxtools not loaded")

/world/New()
	var/debug_server = world.GetConfig("env", "AUXTOOLS_DEBUG_DLL")
	if (debug_server)
		call(debug_server, "auxtools_init")()
		enable_debugging()
	. = ..()

/world/Del()
	var/debug_server = world.GetConfig("env", "AUXTOOLS_DEBUG_DLL")
	if (debug_server)
		call(debug_server, "auxtools_shutdown")()
	. = ..()
```

In your project's SpacemanDMM.toml
```toml
[debugger]
engine = "auxtools"
```

The extension doesn't have a way to override the DLL being used (and I 
don't think it should), so if you're testing stuff I suggest you set 
the env vars to something like below and use the attach mode:
```
AUXTOOLS_DEBUG_DLL=path_to_your_build
AUXTOOLS_DEBUG_MODE=BLOCK
```
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.

4 participants