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

[dap] support non-verified breakpoints #1374

Merged
merged 2 commits into from
Sep 15, 2022

Conversation

TheGeorge
Copy link
Contributor

Description

support non-verified breakpoints in case the module isn't available in the debugged node

Before this would crash, now create a non-verified breakpoint, with some description

Fixes #1373

Copy link
Member

@robertoaloi robertoaloi left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits.

@@ -1084,3 +1091,25 @@ distribution_error(Error) ->
io_lib:format("Could not start Erlang distribution. ~p", [Error])
)
).

-spec maybe_interprete_and_clear_module(node(), module()) -> {boolean(), binary()}.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: interprete -> interpret

true ->
{module, Module} = els_dap_rpc:i(ProjectNode, Module),

%% purge all breakpoints from the module
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to purge them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just easier to code this way. The DAP protocol gives the following information on changes to breakpoints:

  • complete set of line breakpoints per module
  • complete set of function breakpoints in total

So we delete all breakpoints and set them again.

We should probably calculate a delta and then use delete_break and del_break_in. This PR doesn't change the general way we set breakpoints, just makes sure we don't crash on invalid ones.

LineBreakPoints
),
do_line_breakpoints(Node, Module, LineBreakPoints, Breaks, Set) ->
case Set of
Copy link
Member

Choose a reason for hiding this comment

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

In which case do we get Set=false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the module does not exist, e.g. the debugged node cannot load the module.

do_function_breaks(Node, Module, FBreaks, Breaks) ->
[els_dap_rpc:break_in(Node, Module, Func, Arity) || {Func, Arity} <- FBreaks],
do_function_breaks(Node, Module, FBreaks, Breaks, Set) ->
[[els_dap_rpc:break_in(Node, Module, Func, Arity) || {Func, Arity} <- FBreaks] || Set],
Copy link
Member

Choose a reason for hiding this comment

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

Can we please use a regular statement (or a function) instead of the double list comprehension? :)

BreakpointsRsps = [
#{<<"verified">> => true, <<"line">> => Line}
|| {{_, Line}, _} <- els_dap_rpc:all_breaks(ProjectNode, Module)
#{<<"verified">> => IsModuleAvailable, <<"line">> => Line, message => Message}
Copy link
Member

Choose a reason for hiding this comment

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

Should message be a binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, message should be a binary.

@TheGeorge TheGeorge merged commit f1525fc into erlang-ls:main Sep 15, 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.

DAP crashes when a breakpoint is set on an unavailable module
2 participants