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 configuration to run debugger to application's entry point. #306

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

brownts
Copy link
Collaborator

@brownts brownts commented Nov 29, 2021

This adds a new stopAtEntry configuration option which can be used to
command the extension to run the application up to its entry point.
This can be used for both launch and attach configurations. For
launch configurations, the debugger's "start" command will be used if
available, otherwise a temporary breakpoint will be set at the entry
point and execution will be commanded (to allow running to the
breakpoint). When no entry point is provided for an attach
configuration the extension will default the entry point to "main" as
a reasonable default. When no entry point is provided for a launch
configuration and the debugger does not support the "start" command,
the extension will also default the entry point to "main" as well.
The user can explicitly specify the entry point, in which case that
will be used to set a temporary breakpoint.

This adds a new stopAtEntry configuration option which can be used to
command the extension to run the application up to its entry point.
This can be used for both launch and attach configurations.  For
launch configurations, the debuggers "start" command will be used if
available, otherwise a temporary breakpoint will be set at the entry
point and execution will be commanded (to allow running to the
breakpoint).  When no entry point is provided for an attach
configuration the extension will default the entry point to "main" as
a reasonable default.  When no entry point is provided for a launch
configuration and the debugger does not support the "start" command,
the extension will also default the entry point to "main" as well.
The user can explicitly specify the entry point, in which case that
will be used to set a temporary breakpoint.
@GitMensch
Copy link
Collaborator

Can you please elaborate what the point about that is and what issue this solves?

A working stopAtStart or similar would only use "start" when possible and in the other cases connect to the application and interrupt then, no?
And if you want to stop at a specific point you can set breakpoints the normal way already - or are those currently not handled correctly during startup?

src/mibase.ts Outdated Show resolved Hide resolved
@brownts
Copy link
Collaborator Author

brownts commented Nov 29, 2021

Can you please elaborate what the point about that is and what issue this solves?

A working stopAtStart or similar would only use "start" when possible and in the other cases connect to the application and interrupt then, no? And if you want to stop at a specific point you can set breakpoints the normal way already - or are those currently not handled correctly during startup?

Sure, I will attempt to expand on this.

This capability provides a feature similar to cpptools "stopAtEntry" and Eclipse/CDT "stop on startup at" with respect to bringing the debugged application to a recognizable place (i.e., the entry point of the application). Sometimes we don't want to run immediately and thus bringing the application to this point allows the developer to set other breakpoints prior to continuing execution. The developer could have set the other breakpoints prior to running the debugger, but that is not always the preferred workflow.

One of the problems with the cpptools version of this capability is that it is currently hard-coded to use "main" as the entry point. This means it doesn't work for applications which use a non-main entry point (e.g., Ada). Additionally, I wanted this to attempt to auto-detect the entry point if possible (thus the reason for use the "--start" option to "exec-run" if supported by the debugger for a "launch" configuration) as that is what that capability is for. The following snippet for a "launch" configuration will use "exec-run --start" if the debugger supports it, otherwise, it falls back to a sensible default of setting a temporary breakpoint at "main". For an "attach" configuration (since there is no "--start" option for that), this would immediately default to setting a temporary breakpoint at "main".

"stopAtEntry": true

The following snippet shows explicitly specifying the entry point in the case that either the debugger does not support the "--start" option ("launch" configuration only), or that the entry point is not "main" ("launch" or "attach" configuration). For the "attach" configuration, this is not generally applicable to the "attach to PID" scenario as likely the application will have already started running past the entry point. In the "attach to PID" scenario, this provides nothing useful. However, for the "attach to gdbserver" scenario, it works similarly to the launch configuration. If we run gdbserver and provide the application to it, gdbserver will not start running the application immediately. This means, we can attach to gdbserver and run the application to the entry point, similarly to how we would for the "launch" configuration.

"stopAtEntry": "hello_world"

In theory you could use the "autorun" configuration to set a breakpoint, but there can be race conditions with configuring breakpoints too early. Any breakpoints set prior to the "setBreakPointsRequest" arriving from the client will be cleared out (see MI2DebugSession::setBreakpointsRequest), thus the "autorun" configuration is not an ideal location to automatically configure additional breakpoints.

You could manually set a breakpoint at the entry point prior to executing, but this requires manual intervention. If this debug configuration is being shared with a team, it requires everyone on the team to perform this manual setting, if this is the desired startup behavior.

Hopefully this provides the additional details you were looking for.

@WebFreak001
Copy link
Owner

I'm a fan of this change.

I wonder does having this setting also add a checkbox in the breakpoints view for stop at entry or is that something you need to do separately?

grafik

@GitMensch
Copy link
Collaborator

GitMensch commented Nov 29, 2021

Currently it needs to be done additionally - but I really think we should implement what Termdebug has (now): it checks the MI output:

      elseif msg =~ '^\^done,bkpt=' || msg =~ '^=breakpoint-created,'
        call s:HandleNewBreakpoint(msg, 0)
      elseif msg =~ '^=breakpoint-modified,'
        call s:HandleNewBreakpoint(msg, 1)
      elseif msg =~ '^=breakpoint-deleted,
	call s:HandleBreakpointDelete(msg)      

This allows for:

  • a breakpoint being created or deleted from the command line and still be recognized
  • a breakpoint that was created by some debugger script be recognized
  • a breakpoint that was previously pending (because it was set before attaching/running) is reached and then modified (because now the debugger has symbols for it) be recognized
  • breakpoint changes (commonly enabled/disabled, but also "commands executed when reached" or "conditions") being recoggnized by the UI

I suggest to use the same approach in this extension and this will also automatically bring in the support for showing the entry breakpoint (if it is used and necessary) be added in the breakpoint list - automatically as soon as it is reached.

Any volunteers?

Note: I'd also call the auto-removing of breakpoints a mis-feature, so I strongly suggest to remove that

this.miDebugger.clearBreakPoints().then(() => {
together with implementing the auto-recognition and addition to the BP list.

Co-authored-by: Jan Jurzitza <gh@webfreak.org>
@GitMensch
Copy link
Collaborator

@brownts Doesn't that conflict and/or t least overlap with stopAtConnect? Once this is clear and solved (may need README.md and/or package..json adjustments) I'm fine with merging that PR.

@brownts
Copy link
Collaborator Author

brownts commented Nov 30, 2021

I wonder does having this setting also add a checkbox in the breakpoints view for stop at entry or is that something you need to do separately?

This is set as a temporary breakpoint (either explicitly with the breakpoint command or implicitly via the "exec-run --start" command), so will be deleted from the breakpoint list as soon as it is hit. I'm not sure it makes sense for it to be shown in the UI as it would just be inserted and then almost immediately removed. It might be more of a distraction than a benefit. From previous experience with Eclipse/CDT, I've never seen this temporary breakpoint shown in the UI's breakpoint list.

Note: I'd also call the auto-removing of breakpoints a mis-feature, so I strongly suggest to remove that

This behavior (of clearing breakpoints) seems to be consistent with the specificiation. The only difference is that the current implementation clears all breakpoints where the specification indicates that only breakpoints in the same source file should be cleared. It looks like there is a PR for this (i.e., #259), but not sure what happened with it.

Doesn't that conflict and/or t least overlap with stopAtConnect? Once this is clear and solved (may need README.md and/or package..json adjustments) I'm fine with merging that PR.

I would not expect much overlap between "stopAtConnect" and "stopAtEntry". They are expected to be used in different situations. For example, "stopAtConnect" might be used to debug early startup code prior to the application entry point. Another example might be to stop a running application to debug it...maybe it's stuck in a loop and you want to see what it's doing. Thus continuing execution after connect is not desirable. Yet another example might be that the application being debugged is stopped at a breakpoint or exception and you want to attach to it and debug it...again, you wouldn't want to continue execution in this scenario. These are few examples where "stopAtConnect" would be desirable.

The "stopAtEntry" would be used during application startup and is primarily used to just quickly setup the debugger at the application entry point. This way you can set additional breakpoints here, start stepping through your application, or continue execution. This often makes more sense than either stopping at connect (in the early startup code) or continuing execution, for a nominal debug scenario. Also, "stopAtConnect" can only be associated with "attaching", but "stopAtEntry" can be associated with both "launching" and "attaching" (primarily in combination with gdbserver).

I would not expect that it's useful that both "stopAtConnect" and "stopAtEntry" both be enabled at the same time (and this would only be possible with an "attach" configuration as the "launch" configuration does not allow configuration for "stopAtConnect"). However, in the case that they are, the debugger will stop on connection, however it will also set a temporary breakpoint for either "main" or the supplied entry point breakpoint, as specified in the "stopAtEntry" configuration. Thus, when continuing from the point where it was initially stopped on connect, it should run and hit the temporary entry point breakpoint. So these can both be enabled at the same time, and should work as expected, but I wouldn't expect that this configuration would be useful for most scenarios.

@brownts
Copy link
Collaborator Author

brownts commented Jan 4, 2022

Is there any further discussion to be had on this or can it be pulled in?

@brownts
Copy link
Collaborator Author

brownts commented Feb 21, 2022

ping @GitMensch, @WebFreak001

@WebFreak001
Copy link
Owner

LGTM, but didn't have any objections before, would like to see what @GitMensch thinks

@GitMensch GitMensch merged commit c46ebc7 into WebFreak001:master Feb 25, 2022
@GitMensch
Copy link
Collaborator

Thanks for your additional contribution, for the explanation (after re-reading README I've seen no conflict any more) and for your patience (I was away for two weeks).

I still think that it would be good to implement a breakpoint recognition as outlined above, but this is a general issue not only related to this issue (I'd argue for "useful to be shown" when connecting / reaching the debugger's application entry point takes a while).
If someone (@brownts ?) could have a look into this it would be marvelous, but as it is a different issue - merged this.

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.

3 participants