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

C#: Fix loading plugin to support debug symbols #87595

Closed

Conversation

granitrocky
Copy link

@granitrocky granitrocky commented Jan 25, 2024

Fixes godotengine/godot-csharp-vscode#43

This change replaces LoadFromStream with a direct LoadFromAssemblyPath to allow the C# module to interface with netcoredbg

This solution comes from this issue:
Samsung/netcoredbg#91 (comment)

This change replaces `LoadFromStream` with a direct
`LoadFromAssemblyPath` to allow the C# module to interface with
`netcoredbg`

This solution comes from this issue:
Samsung/netcoredbg#91 (comment)
@granitrocky granitrocky requested a review from a team as a code owner January 25, 2024 20:44
@granitrocky granitrocky changed the title Replace LoadFromStream with LoadFromAssemblyPath Fix for godotengine/godot-csharp-vscode#43 Jan 25, 2024
@raulsntos
Copy link
Member

This sounds like a bug on netcoredbg. AssemblyLoadContext.LoadFromStream should work as it does in Microsoft's debugger.

As I understand it from reading the issue you linked, netcoredbg is unable to find the PDB file when using the AssemblyLoadContext.LoadFromStream overload that only takes the assembly stream (the DLL). In this case, they try to find the PDB file name from the DLL and if it doesn't contain it then they can't find it.

I haven't checked but I'd imagine the DLL built from Godot contains the PDB file name, so they should be able to find it. But even if it didn't contain it, we're not using that overload. We're using the overload that takes the assembly stream AND the assembly symbols stream (the PDB), so we're already providing the PDB. This case (our case) should work, and if it doesn't then it's a bug on netcoredbg.

There's also a linked PR (Samsung/netcoredbg#108) that seems like it fixes the netcoredbg bug, so I suggest waiting for that instead. You could try that branch and see if it fixes the issue for you.


As for why we are using AssemblyLoadContext.LoadFromStream, if you read the comment: we're loading in memory to prevent locking the file.

// Load in memory to prevent locking the file

@granitrocky
Copy link
Author

I agree that the netcoredbg branch is probably the better option if it gets resolved, but reading through it, I do see that it's pretty stale.

Are we sure that LoadFromAssemblyPath locks the file? I couldn't find anything that says it does. Even took a quick glance through some of the code in the dotnet repo and nothing jumped out at me.

@raulsntos
Copy link
Member

raulsntos commented Jan 26, 2024

AFAIK it's a Windows thing not specific to LoadFromAssemblyPath that happens whenever you open a file handle (which naturally this method does). But I don't use Windows so I can't check. It seems GDExtensions have that issue right now:

@thanikTheDev
Copy link

I just created a new PR(Samsung/netcoredbg#164) which should fix this issue. I do believe the netcoredbg route because it was unable to read in-memory module symbols. msdbg does not.

@AThousandShips AThousandShips changed the title Fix for godotengine/godot-csharp-vscode#43 C#: Fix loading plugin to support debug symbols Feb 17, 2024
@thanikTheDev
Copy link

@granitrocky The changes have been implemented so you can try the latest version of netcoredbg.

@granitrocky
Copy link
Author

@thanikTheDev Thanks! I'll try it out and if it fixes the issue I can close this PR.

@raulsntos
Copy link
Member

@granitrocky Can you confirm that this was fixed upstream?

@granitrocky
Copy link
Author

@raulsntos I can confirm that the latest netcoredbg works. I will close this issue.

@AThousandShips AThousandShips removed this from the 4.x milestone Apr 29, 2024
@jolexxa
Copy link
Contributor

jolexxa commented May 26, 2024

@granitrocky Would you clarify how you were able to get netcoredbg working, which IDE you used, and whether or not it works in VSCodium? Per our conversation in Chickensoft, it sounds like it does not work from VSCodium, and I am wondering if there is a relevant bug on the VSCodium side of things (I was unable to find one). I tried using the latest VSCodium with the C# extension that uses netcoredbg, and it simply quit without any errors in any of the logs.

@granitrocky
Copy link
Author

granitrocky commented May 26, 2024

I used the netcore emacs package along with dap-mode. I did have to manually change the command line that was invoked to include a path to godot on the netcoredbg invocation. I imagine there's a better way to do that, though.

This does seem to be a VSCodium related problem and not netcoredbg or Godot

VSCodium/vscodium#82

@Novack
Copy link

Novack commented May 26, 2024

I used the netcore emacs package along with dap-mode. I did have to manually change the command line that was invoked to include a path to godot on the netcoredbg invocation. I imagine there's a better way to do that, though.

This does seem to be a VSCodium related problem and not netcoredbg or Godot

VSCodium/vscodium#82

I think either you or me, are misreading something :)

Apart from the "easy way" not working anymore, that post you linked seems to indicate precisely how to make it work. In fact is the current recommended approach by vscodium team. Its only in godot that we are having issues.

Edit: my bad, apologies. It seems I was focusing in that issue OP, but missing crucial parts of the conversation going on there.

@granitrocky granitrocky deleted the LoadAssemblyForDebugger branch May 27, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Godot 4 + mono
6 participants