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

Add option to not auto-update debug start settings when selected assembly changes #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mitchcapper
Copy link

right now when the assembly selected changes the debug start settings change as well. This isn't a huge issue if you have only a assemblies open, but if you end up with dozens upon dozens it becomes a bit painful. This option re-uses the last debug settings by default no matter what assembly you are on.

Naming / description could probably use some work.

@ElektroKill ElektroKill added this to the vNext milestone Jul 19, 2022
@mitchcapper mitchcapper force-pushed the feature_no_auto_update_debug_settings_pr branch from 2501c71 to a25c5b7 Compare July 19, 2022 19:39
@ElektroKill
Copy link
Member

Hi, is there really a need for this change? When a user debugs a exe, stops, and clicks the start debugging button again while having a DLL selected it will default to using the previous debugger start options. This however does not happen when an exe is selected which I assume is the goal of your changes. There is however a flaw in the implementation here, if the user debugs a exe for example A.exe, and then tries to debug B.exe while having the new option enabled it will use the debugger start options used for A.exe and will then cache them as the options for B.exe. This can lead to confusion for the user. Disabling the new option does not reset the cache and dnSpy will keep using A.exe start options for B.exe until it is restarted. This needs to be resolved in some way before I can merge this.

Also,a re you sure implementing this behavior is required for .exe files? In most cases, the debugged program comes with only one exe and a few DLLs as dependencies.

@@ -43,10 +43,12 @@ sealed class StartDebuggingOptionsProvider {
readonly Lazy<StartDebuggingOptionsPageProvider>[] startDebuggingOptionsPageProviders;
readonly Lazy<DbgProcessStarterService> dbgProcessStarterService;
readonly Lazy<GenericDebugEngineGuidProvider, IGenericDebugEngineGuidProviderMetadata>[] genericDebugEngineGuidProviders;
readonly Lazy<Settings.DebuggerSettingsImpl> debuggerSettingsImpl;
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure we don't actually need the DebuggerSettingsImpl and we could just DI the base DebuggerSettings class instead.

Comment on lines +50 to +51
StartDebuggingOptionsProvider(IAppWindow appWindow, IDocumentTabService documentTabService, Lazy<DbgProcessStarterService> dbgProcessStarterService, [ImportMany] IEnumerable<Lazy<StartDebuggingOptionsPageProvider>> startDebuggingOptionsPageProviders, [ImportMany] IEnumerable<Lazy<GenericDebugEngineGuidProvider, IGenericDebugEngineGuidProviderMetadata>> genericDebugEngineGuidProviders, Lazy<Settings.DebuggerSettingsImpl> debuggerSettingsImpl) {
this.debuggerSettingsImpl = debuggerSettingsImpl;
Copy link
Member

Choose a reason for hiding this comment

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

Same as before, I am pretty sure we don't actually need the DebuggerSettingsImpl and we could just DI the base DebuggerSettings class instead.

@@ -87,6 +89,9 @@ sealed class StartDebuggingOptionsProvider {

var oldOptions = mru.TryGetOptions(filename);
var lastOptions = mru.TryGetLastOptions();
if (oldOptions == null && lastOptions.HasValue && lastOptions!.Value.options != null && debuggerSettingsImpl.Value.DontAutoUpdateDebugLaunchSettings)
Copy link
Member

Choose a reason for hiding this comment

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

lastOptions!.Value.options != null is redundant here as options element in the tuple is not marked as nullable and thus should never be null.

@mitchcapper
Copy link
Author

Let me put together a repro case, I can say that even debugging one exe I can get this often (debug conditions lost). Maybe it happens in those modules are dynamic and not file based. Will find an easy test and can figure out if it is just a flaw in the current detection or if a new option/feature is needed.

@mitchcapper
Copy link
Author

Ok two conditions it seems to 'fail':

  1. have a .net core application, open the DLL, start debugging, uncheck use host executable and select the .exe launcher instead.
    - If you are on the dll assembly when you go to relaunch it fails as:
    dnSpy.Debugger.Debugger.DbgUI.StartDebuggingOptionsMru::Find Trying to find old options list item: "c:\path\to\excepthrow.exe" compared to now: "c:"\path\to\excepthrow.dll" this is a bigger problem if the launcher is not the default .net launcher but does some other setup before loading the dll, then you must use it but the debug settings will never stick.

  2. if the dll has an entrypoint. I am guessing the check is not if it is .exe but rather if it has an entrypoint (thus supporting .net core dll auto-launches). Only problem is if an application uses the runtime dll from another application as a library file any time you go to start debugging from one of those dlls it will have the wrong launch settings.

@mitchcapper
Copy link
Author

#1 is tricky to fix too, while you could store debug settings based on the original module selected (rather than the resulting start executable) this could break if you store those settings for a non-entrypoint dll and later change the debug settings. IE:
Load a .net core dll mydll.dll, select custom launcher, RU caches the debug startup options based on the key mydll.dll that was selected at launch. Start debugging from another class library dll myclasses.dll. Launch settings are not found so it defaults to the last used mydll.dll settings. Once launched if this adds to the RU cache based on myclasses.dll for the key if you now go back to mydll.dll and change the launch options (say command line args) next time you start from code within myclasses.dll you will use the old launch args rather than the ones you just updated.

You could ignore storing any RU cache for non entrypoint dlls but that wouldn't fix a launcher that loads a library and manually calls a function to start the process rather than using a standardized entrypoint (as those debug settings would never stick).

Aside from this solution of always re-using the last launch settings, you could try to track how an assembly was loaded in. if opened from file open then when starting debug reset debug settings (if that specific assembly is not in the RU). This would need to be tracked cross dnspy opens though which would be an annoying additional piece of information. It also wouldn't solve if you opened it from an attached program by loading the module from memory. You could end up as well where you open a resource on an executable in assembly explorer and it looks just like if you opened it from file->open but would behave differently.

While I would assume 98% of the time when people open dnSpy they are debugging the same thing for the entire session there could be cases where you have say 5 executables you are jumping between and want the launch settings preserved between them.

Still I think there could be value in this sort of PR option. As it is just an option the user could easily toggle it on and off depending on the desired scenario for those 2% times.

Right now the best solution I have found is to have 'tabs' that have assemblies I want to launch from open, then after launching I switch to a new tab so that tab does not go off of the good assembly for launching. Inelegant to say the least.

@ElektroKill ElektroKill removed this from the vNext milestone Dec 24, 2022
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.

2 participants