Add renaming of PDB files to avoid blocking them

[DmitriySalnikov]

Fixes #82536
Supersedes godotengine/godot-cpp#1331

In this implementation, I patch the copied ~DLL by replacing the PDB path in it with a new non-blocked path. Actually, all that is left of the PDB path is the file name, so that the new name almost never goes beyond the existing CODEVIEW entry.

Strangely, while I was testing this implementation the debugger did not block more than one PDB file, whereas in python/scons implementation PDBs were blocked until the debugger was restarted. If PDBs will be blocked until the debugger is restarted, 1000 copies of the PDB can be created.
I also noticed that vscode and vs load debug symbols with no problem, but vs doesn't update the breakpoints state and they have to be manually recreated.

If the DLL is renamed, then the PDB too, and previously created copies of it will not be deleted automatically.
Also, if a version without debug symbols is built, past previous will not be deleted.

Tested only with godot-cpp and msvc.

[DmitriySalnikov]

After a strange bug with breakpoints in Visual Studio, I decided to try other debuggers:

• remedybg 3.9.6+ loads the PDB and updates breakpoints out of the box. That said, it doesn't even lock the PDB and this patch makes no sense for it 😐.
• x64dbg also loads updated PDBs without problems and does not block them.
• raddbg updates breakpoints without problems and does not lock the PDB (and creates its own locked files). But at the moment has problems with relative paths.

Visual Studio bug

In my testing only Visual Studio has the breakpoints bug. Of course, there are other IDEs and debuggers, but I have not tested them.
Example of a bug:

vs.mp4

[dsnopek]

Thanks!

Overall, I think pursuing the approach of copying the PDB file is the right way to go. However, I'm not sure about a couple of things:

• I don't know that it makes sense for something under core/ to directly include something from platform/. In other cases where we do this, it's much more generic, and it's done with special headers like platform_thread.h, platform_gl.h, and platform_config.h. I'll defer to the production team (cc @akien-mga @YuriSizov) on this, but it may be better to keep the Windows code for this in core/extension/
• With the DLL copies, we're only making one copy and we clean it up at shutdown. This PR appears to allow making up to 1000 copies of the PDB and it appears to clean them up only when running the editor again, but not at editor shutdown. I'd prefer we handle the PDB in the same way we handle the DLL, where we just make one copy with a predictable name, and clean it up afterwards - unless there is a compelling technical reason not to do it that way

[DmitriySalnikov]

• I don't know that it makes sense for something under core/ to directly include something from platform/

Yeah I knew it would probably lead to a discussion, but I didn't want to embed 200 lines of Windows-specific code either.

• With the DLL copies, we're only making one copy and we clean it up at shutdown.

There's a problem with that.
I'll try to add some code that tries to clean the folder before closing engine, but it probably won't be possible to delete all PDBs anyway.
During testing, I set a breakpoint at the very end of main.cpp, but the PDB remained locked.

This won't be a problem for some debuggers, but Visual Studio will block the removal of the PDB (at least one).

[YuriSizov]

• I don't know that it makes sense for something under core/ to directly include something from platform/. In other cases where we do this, it's much more generic, and it's done with special headers like platform_thread.h, platform_gl.h, and platform_config.h. I'll defer to the production team (cc @akien-mga @YuriSizov) on this, but it may be better to keep the Windows code for this in core/extension/

As a general rule platform code should not be bundled into anything outside of the each platform's respective folder. This allows for platforms to remain modular and for new platforms to be able to use all of the existing framework without patching the engine.

I can see in this case that there is already a bunch of platform-specific code in the modified file. So it's tempting to say that this is okay as is. However, your example of platform_thread.h and friends looks more appealing to me. It offers a hook for platforms to integrate into the process without an explicit dependency. That would be the way to do it, IMO. Possibly to move all platform-dependent hacks out of this core file this way.

[dsnopek]

This won't be a problem for some debuggers, but Visual Studio will block the removal of the PDB (at least one).

I think one debugger (even a popular one) having issues with cleaning up the PDB when the editor shuts down is probably acceptable. We'll always need code to clean-up or overwrite a left over PDB when the editor starts up again, because the editor can always crash before it's clean up can run. But I think attempting to clean up (and only making one copy in the first place) is important to try to do.

[DmitriySalnikov]

Added HashMap for storing temporary PDB files.
Added remove_temp_temp_pdbs to try to delete temporary PDB files when closing the extension.
Fixed renaming of PDB files when running a project.

[DmitriySalnikov]

Do I need to make any other edits?

[DmitriySalnikov]

Done.

[dsnopek]

Thanks! This is looking really great to me :-)

I did some testing (although, I'm not the best person to test Windows and MSVC stuff). When building my extension with gcc (which doesn't use a PDB file), it correctly did nothing with no complaints. :-) Then with MSVC, it seems to be working correctly: the PDB file got renamed, and it still stopped on my breakpoints. I can confirm the behavior where the PDB file doesn't get cleaned up at exit when debugging with MSVC, but I don't think there's any more we can do about that.

The only remaining open question is if it's OK that core/extension/gdextension.cpp is directly including platform/windows/editor_windows_utils.h? Usually, we don't allow stuff in core/ to directly include stuff in platform/.

We have a couple of workarounds to this where all platforms have a specific header, like platform_config.h and platform_gl.h and we include that, and the current platform's version gets pulled in. We could add a platform_extension.h or something like that? Although, I don't know that we'll ever have other platforms that need special code aside from Windows.

@akien-mga What do you think? Is the direct core/ -> platform/ include OK in this case? Or, should we try to come up with some way to make it indirect? Or, move the Windows util code into core?

[akien-mga]

@akien-mga What do you think? Is the direct core/ -> platform/ include OK in this case? Or, should we try to come up with some way to make it indirect? Or, move the Windows util code into core?

Yeah this needs to be changed, core shouldn't include anything from outside core.

[dsnopek]

I haven't thought about this too deeply, but I wonder if we could put the PDB copying logic (and the existing DLL copying logic) into OS_Windows::open_dynamic_library() and OS_Windows::close_dynamic_library()?

Android has a bunch of custom copying logic in its open_dynamic_library() and this wouldn't be too dissimilar.

[DmitriySalnikov]

So you want me to move all #ifdef WINDOWS_ENABLED ... #endif to os_windows.cpp? I think it looks doable, but somehow it needs to be specified in open_dynamic_library that temporary files need to be created, any ideas?

Leave the editor_windows_utils.cpp or also merge with os_windows.cpp ?

[dsnopek]

So you want me to move all #ifdef WINDOWS_ENABLED ... #endif to os_windows.cpp?

Well, so far it's just an idea - I haven't thought through it far enough to know if it's a good idea or not. :-) If you take a look at it and it doesn't seem like there'd be any problems, then give it a try.

I think it looks doable, but somehow it needs to be specified in open_dynamic_library that temporary files need to be created, any ideas?

Right now, we make the temporary copies when Engine::get_singleton()->is_editor_hint() is true. The Engine singleton is part of core, so I think it should be fine to keep using that?

Leave the editor_windows_utils.cpp or also merge with os_windows.cpp ?

I think either way would fine. Within platform/windows/ it can have any many helper classes as makes sense.

[DmitriySalnikov]

Right now, we make the temporary copies when Engine::get_singleton()->is_editor_hint() is true.

I found the use of open_dynamic_library in the mono module. I don't think is_editor_hint is enough.

I'm thinking of adding another argument to open_dynamic_library. I will have to update all OS implementations, but then it will be possible to activate the generation of temporary files, for example for FFI (#85741).

[DmitriySalnikov]

• Moved Windows code from gdextension.cpp to os_windows.cpp .
• Added the p_generate_temp_files argument to open_dynamic_library.
• The maximum number of prints of Failed to remove temp PDB is limited.

[dsnopek]

Discussed at the GDExtension meeting, and we think moving the Windows-specific code into OS_Windows makes sense! Someone will still need to review the code and re-test, but at a high-level this seems like the right direction.

[dsnopek]

Sorry for taking so long to come back to this yet again!

However, I reviewed the code in-depth and it looks great to me. I also re-did the same Windows testing I did earlier, and everything still seems to be working fine. :-)

This needs to be re-based (there are conflicts now), but after that, I think this is ready to go!

[DmitriySalnikov]

In #89677, the paths in ERR_PRINT were fixed, but in my PR, these prints were moved to os_windows.cpp and I fixed the paths there a while ago.
There was also a conflict in SCsub, where I just added my cpp file.

[DmitriySalnikov]

Is there a chance of merging before 4.3 is released?

[dsnopek]

I'd personally really love to see this merged for Godot 4.3 - this fixes a very common complaint from folks using godot-cpp with MSVC. And, yes, at this point, there still is a chance :-)

[akien-mga]

Thanks!