We should get @alainfrisch 's opinion, but I personally think that this behavior is nicer than the current one. (The code is also a bit simpler.)

It would be nice to have some test coverage for reraise in the testsuite, but I'm not sure how to write good tests for this behavior.

Isn't it still the case that the backtrace mechanism discards the current segment when the current exception is physically different from the previous one? It used to be like that, which breaked the "reraise after wrapping" scenario anyway.

The reason I restricted the reraise heuristics was to be closer to the existing behavior when I changed the representation of constant exception constructors (which has the effect of making all instances of e.g. "Exit" physically equal, and thus breaking the runtime decision based on physical equality of exceptions and common cases).

The risk here is to get completely bogus stack traces, not corresponding to any actual control flow.

Yes in its current state the backtrace mechanism still check for equality. Equality of exception values is too restrictive and not really meaningful (especially with parameter less exceptions).

My opinion would be to drop the mechanism and rely only on raise vs reraise (that is done in my branch storing multiple exception values, I should probably also have included this patch here :)).

The rewriting done ensures backtraces are correct (I have no formal proof, but it's turning a raise to a reraise only occurs inside the handler of a try, not going through closures to avoid 'capturing and reraising in a different context').

Ah, I had not realized that the backtrace would still be cut above the now-reraise point because of the runtime. The "nicer behavior" I had in mind would include the trace of both the raised and the re-raised exceptions in the backtrace.

This latest commit makes backtrace reset only happen when using raise primitive.

In bytecode, it is determined by reraise boolean flag.
In native. it is done in the assembly stub: https://github.com/def-lkb/ocaml/blob/static-reraise/asmrun/amd64.S#L542 .

Small brain dump -- sorry.

Here is one scenario where maybe the new approach is not as good as the previous one. Consider a backtracking search implemented using try .. with ...: try <first> with Failure -> <second> represents the asymmetric search alternative <first> + <second>. (I think this technique is used in Coq's proof search, for example). Is it the right decision to include the partial failure trace of <first> if <second> statically contains a raise?

I think both "yes" and "no" would be reasonable answers to this question. It is clear that it does not make much sense the failure trace of first before the failure trace of second, but having both traces in the whole backtrace can actually be useful (as an auditing user I may be surprised by the failure of <second>, but also be equally interested in the reason why <first> failed). In any case, it will rarely be the case that <second> statically contains a raise, it is more like to call some search function that may in turn fail.

Which comes to the question: I suppose that the re-raise decision is currently is made before inlining (so in particular raises that would be inlined inside a handler would not be turned into re-raises). Would there be any reason to change this?

How should we think of backtrace to evaluate whether a particular behavior is a feature or "completely bogus"? What does it mean for a trace to "correspond to any control flow" in the relaxed setting proposed by @def-lkb ? One proposal would be causality: it is good to have to raise traces S1 and S2 concatenated in the returned trace if the raise S2 was caused by the raise S1.

(This all means we should have tests in the testsuite, to at least specify some partial behavior.)

Is it the right decision to include the partial failure trace of if statically contains a raise?
In my opinion yes. Right now it will be printed as a "re-raise", which might be a bit misleading.
In the many exception branch, it's printed as "Raised Not_found at ... Re-raised as (Failure _) at ...".

Which comes to the question: I suppose that the re-raise decision is currently is made before inlining (so in particular raises that would be inlined inside a handler would not be turned into re-raises). Would there be any reason to change this?

It is done during translation, so before inlining yes. I like it this way, it is a simple syntactic criterion.

How should we think of backtrace to evaluate whether a particular behavior is a feature or "completely bogus"? What does it mean for a trace to "correspond to any control flow" in the relaxed setting proposed by @def-lkb ? One proposal would be causality: it is good to have to raise traces S1 and S2 concatenated in the returned trace if the raise S2 was caused by the raise S1.

I just realised that a function called inside the handler, raising and catching an exception (not letting it escape), and returning to the handler which finally reraise will produce a "bogus" backtrace:

try ...
with exn ->
  f (); (* f messes with backtrace buffer *)
  raise exn

This can already be done by using constant exceptions or capturing a closure that raises inside a static handler, not checking for equality just make it happen more often.
I don't know what can be done about that.

Well we could rewrite this handler in a more invasive way that just turning the raise into a reraise. For example, the beginning of the handler could retrieve a runtime value representing the "trace timestamp", incremented by caml_stash_backtrace, and then pass it as parameter to a "smart reraise" primitive called at raise point. I am not convinced this is a good idea.

Neither am I.

We could also keep a sequence of traces instead of just the last one, as long as the total memory usage does not grow above the BACKTRACE_BUFFER_SIZE limit. This would not solve the problem (although indexing in that sequence could serve as timestamp), but make those bogus backtraces much more useful.

Yes, the example 4 posts above this one (try...with exn -> f(); raise exn) is typical of the situations we don't handle well, neither with the current OCaml solution (the backtrace is too short, claiming that the exception is raised for the first time at the reraise point) nor with the proposed change (the backtrace claims that the exception was first raised inside f(), right?). I don't have anything better to suggest at this point.

@gasche Your idea of keeping at the start of the handler some timestamp(caml_backtrace_pos) seems very good. What are the problems you forsee? What I understand of your proposition:

• A supplementary datastructure caml_reraise (array of pair) keep a mapping between try_with_a_reraise (the stack pointer during the execution of the try_with) and its timestamp (value of caml_backtrace_pos at the start of the handler). Elements are added in order since it follows the stack pointer.
• A new global variable caml_backtrace_start keeps the current start of caml_backtrace_buffer (invariant all the timestamp of caml_reraise are smaller than caml_backtrace_start)
• Function like caml_get_exception_raw_backtrace doesn't start to read the backtrace at 0 but at caml_backtrace_start
• The function caml_stash_backtrace for a raise look in caml_reraise for the smallest try_with_a_reraise that is bigger than trapsp (smaller/bigger in the sense of level not stack address pointer comparison) and set caml_backtrace_start to its timestamp (doesn't change if there is none). Set caml_backtrace_pos to caml_backtrace_start. Remove the element from caml_reraise until the previously found smallest try_with_a_reraise.
  By doing so the memory used by the try_with_a_reraise that are jumped is reclaimed in caml_backtrace_buffer.
• At the start of a handler that contains reraise, the mapping is added between the current stack_pointer and caml_backtrace_pos
• During reraise, the function caml_stash_backtrace does the same thing than for raise except that the element in caml_backtrace_buffer between the timestamp for sp and the next timestamp in caml_reraise (or caml_backtrace_start if none) are copied at the new caml_backtrace_pos.

I can partially reply to my own question. If the invariant of the datastructure needs the participation of the raise or reraise, it will not work when a code compiled with -g is linked with a code not
compiled with -g since without -g raise is limited to:

movq    %r14, %rsp
popq    %r14
ret

It is also true for code compiled with -g that use raise_notrace.

PS: raise_notrace doesn't do what the name implies since the trace of the last raise is returned. There is no STORE_VAR32($0, caml_backtrace_pos) during raise_notrace.

external raise_notrace: exn -> 'a = "%raise_notrace"

let f () =
  try raise Not_found
  with _ -> raise_notrace Exit

Fatal error: exception Pervasives.Exit
Raised at file "test_raise_notrace.ml", line 4, characters 12-21

One remark on this:

@chambart pointed out that GPR#270, with flambda, may mean that we get worse backtraces (I think since arguments to [raise] will be more likely to be lifted out as constants and shared, which means such things will be physically equal to each other more often). It is suspected that the work of @def-lkb referred to above (removing the phys-equal check) would solve this problem.

The latest comment for this PR goes back to Nov 2015, so I feel safe in closing it. Reopen if anyone has new things to contribute.

Maybe we would need a specific label "Frozen Nice Stuff that Frédéric may have time to look at again someday."