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

emitcode: merge events after instructions reordering #1573

Merged
merged 1 commit into from Feb 19, 2018

Conversation

Projects
None yet
3 participants
@trefis
Copy link
Contributor

trefis commented Jan 17, 2018

Because of some "peephole optimizations" done in Emitcode we can sometimes end-up with two debug events at the same position.
Consider for example test.ml:

let run f =
  try f ()
  with exn ->
    match exn with
    | x -> (* disable reraise *)
      raise x

let () =
  run (fun () -> raise Exit)

If we look at the .cmo currently being generated, we get:

## start of ocaml dump of "test.cmo"
       0  BRANCH 13
File "test.ml", line 2, characters 2-90:
       2  PUSHTRAP 10
       4  CONST0
       5  PUSHACC5
       6  APPLY1
File "test.ml", line 2, characters 6-10:
       7  POPTRAP
       8  RETURN 1
      10  PUSHACC0
File "test.ml", line 4, characters 4-65:
      11  PUSHACC0
File "test.ml", line 6, characters 6-13:
File "test.ml", line 6, characters 12-13:
      12  RAISE
      13  CLOSURE 0, 2
      16  PUSH
File "_none_", line 1, characters -1--1:
      17  ACC0
      18  MAKEBLOCK1 0
      20  POP 1
      22  SETGLOBAL Test
## end of ocaml dump of "test.cmo"

When I run that bytecode executable on my linux machine, I get the following

$ ./a.out
Fatal error: exception Pervasives.Exit
Raised at file "test.ml", line 6, characters 6-13
Called from file "test.ml", line 9, characters 2-28

But when @dra27 runs it on his windows (mingw?) machine, he gets:

Fatal error: exception Pervasives.Exit
Raised at file "test.ml", line 6, characters 12-13
Called from file "test.ml", line 9, characters 2-28

Which is consistent with what we observe on #1567 (that is: the appveyor failure).

I'm guessing that's because of the call to qsort in process_debug_events in byterun/backtrace_prim.c, and that a stable sort would make this discrepancy disappear.

As I was not in the mood to write any C code, I instead chose to change Emitcode so it doesn't emit two events at the same position.
That is, after this PR test.cmo looks like this:

## start of ocaml dump of "test.cmo"
       0  BRANCH 13
File "test.ml", line 2, characters 2-90:
       2  PUSHTRAP 10
       4  CONST0
       5  PUSHACC5
       6  APPLY1
File "test.ml", line 2, characters 6-10:
       7  POPTRAP
       8  RETURN 1
      10  PUSHACC0
File "test.ml", line 4, characters 4-65:
      11  PUSHACC0
File "test.ml", line 6, characters 12-13:
      12  RAISE
      13  CLOSURE 0, 2
      16  PUSH
File "_none_", line 1, characters -1--1:
      17  ACC0
      18  MAKEBLOCK1 0
      20  POP 1
      22  SETGLOBAL Test
## end of ocaml dump of "test.cmo"

Which will result in consistent backtraces between linux and windows.


Questions / remarks:

  • I am not adding a new test in the testsuite because I'm hoping for #1567 to get merged, and it does contain a test for this.
  • why are the optimizations done in emitcode and not bytegen? Before calling tools/dumpobj I was actually looking at the output of -dinstr which happens before the optimizations, it made me sad.
  • in byterun/backtrace_prim.c there is a comment saying "ocamlc sometimes moves an event past a following PUSH instruction; ...". Could that be related to this? (or this to that)
@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Jan 17, 2018

It's ugly, but why not do both:

diff --git a/byterun/backtrace_prim.c b/byterun/backtrace_prim.c
index e69b2568e..5bf61cd16 100644
--- a/byterun/backtrace_prim.c
+++ b/byterun/backtrace_prim.c
@@ -100,12 +100,27 @@ static struct debug_info *find_debug_info(code_t pc)
   return NULL;
 }

-static int cmp_ev_info(const void *a, const void *b)
+static int cmp_ev_info(const struct ev_info *a, const struct ev_info *b)
 {
-  code_t pc_a = ((const struct ev_info*)a)->ev_pc;
-  code_t pc_b = ((const struct ev_info*)b)->ev_pc;
+  code_t pc_a = a->ev_pc;
+  code_t pc_b = b->ev_pc;
+  int result;
   if (pc_a > pc_b) return 1;
   if (pc_a < pc_b) return -1;
+  if ((result = strcmp(a->ev_filename, b->ev_filename))) {
+    return result;
+  } else {
+    int ofs_a = a->ev_lnum;
+    int ofs_b = b->ev_lnum;
+    if (ofs_a > ofs_b) return 1;
+    if (ofs_a < ofs_b) return -1;
+    ofs_a = a->ev_startchr;
+    ofs_b = b->ev_startchr;
+    if (ofs_a < ofs_b) return 1;
+    if (ofs_a > ofs_b) return -1;
+    ofs_a = a->ev_endchr;
+    ofs_b = b->ev_endchr;
+  }
   return 0;
 }

Note that the comparison of startchr is intentionally the other way around, as that makes Windows behave the same as Unix has hitherto 😛

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Jan 17, 2018

Oops - MSVC doesn't mind saving characters on casts, but GCC does. Correct version at dra27@77f02ac (feel free to cherry-pick)

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Jan 22, 2018

Oops - MSVC doesn't mind saving characters on casts, but GCC does.

And that confirms I made the right choice by not trying to write any C! :)

More seriously, I don't feel competent to judge your patch (could it have any impact on runtime performances?) so, as it is not necessary if my patch is there, I don't think I'll include it in this PR.
Perhaps you should submit it in a separate PR so we can discuss there whether to include it in addition to (or as a replacement of) mine?

@trefis trefis requested a review from damiendoligez Feb 13, 2018

@let-def
Copy link
Contributor

let-def left a comment

This change on its own looks good to me.

@let-def

This comment has been minimized.

Copy link
Contributor

let-def commented Feb 16, 2018

However I think @dra27 patch is worth merging (the performance impact should be negligible, unless you end up with a huge amount of events at the same pc !?).

@trefis trefis force-pushed the trefis:merge-events branch from 2244b0a to 8898c04 Feb 19, 2018

@trefis trefis merged commit 74c9dfa into ocaml:trunk Feb 19, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@trefis trefis deleted the trefis:merge-events branch Feb 19, 2018

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.