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

Location.mknoloc confuses ocamldebug #7939

Open
vicuna opened this Issue Mar 9, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@vicuna
Copy link
Collaborator

vicuna commented Mar 9, 2019

Original bug ID: 7939
Reporter: db
Status: feedback (set by @xavierleroy on 2019-03-09T15:15:21Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.07.1
Category: lexing and parsing
Monitored by: @nojb @diml

Bug description

If Location.mknoloc is used in a ppx extension, AST gets a node with "dummy" begin and end locations:

let dummy_pos = {
  pos_fname = "";
  pos_lnum = 0;
  pos_bol = 0;
  pos_cnum = -1;
}

Unfortunately this breaks ocamldebug that expects locations been monotonically increasing and uses binary search to set a breakpoint (debugger/symbols.ml):

(* Binary search of event at or just after char *)
let find_event ev char =
  let rec bsearch lo hi =
    if lo >= hi then begin
      if (Events.get_pos ev.(hi)).Lexing.pos_cnum < char
      then raise Not_found
      else hi
    end else begin
      let pivot = (lo + hi) / 2 in
      let e = ev.(pivot) in
      if char <= (Events.get_pos e).Lexing.pos_cnum
      then bsearch lo pivot
      else bsearch (pivot + 1) hi
    end
  in
  if Array.length ev = 0 then
    raise Not_found
  else
    bsearch 0 (Array.length ev - 1)

How to fix that? Completely avoid using Location.mknoloc (but many existing ppx extensions AFAIK use it) or do linear search in ocamldebug instead of binary one?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 9, 2019

Comment author: @gasche

Could we keep doing the bisection, but skip noloc-produced locations?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 9, 2019

Comment author: @xavierleroy

I don't think the binary search is the issue, because events are sorted by increasing pos_cnum when the debugger reads them off executable files (see function read_symbols in debugger/symbols.ml).

Could you give more details on the problem you observe? Like a repro case, for example?

@db4

This comment has been minimized.

Copy link

db4 commented Mar 16, 2019

@xavierleroy you are right and my analysis was incorrect (the issue title should be changed but I don't have rights to do so) Indeed, event lists are sorted so ghost events is not an issue but the current binary search implementation is. In the example below

let () =
  print_string "1";
  print_string "2";
  print_string "3";
  print_string "4";
  print_string "5"

if you try to set a breakpoint on line 5 it's actually set on line 4:

(ocd) break @ Test_debug 5 
Breakpoint 2 at 14588: file test_debug.ml, line 4, characters 3-19

We should find an event greater or equal to the current position, not the lesser one.

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.