Add reraise_raw_backtrace primitive by bobot · Pull Request #378 · ocaml/ocaml

bobot · 2015-12-27T11:25:18Z

(splitted from #374)

The function Printexc.reraise_raw_backtrace is an extension of reraise where you specify the backtrace to start with. This is what was well-received from #293 for 4.03, however this PR arrived a lot later than requested.

This PR doesn't make the heuristic try ... with e -> ...; raise e use reraise_raw_backtrace because it requires to save the backtrace and it would be going a little to far for this kind of heuristic to create allocation point where there is none.

This PR also lists different cases as new tests where wrong backtraces are used. Some can be avoided using explicitly reraise_raw_backtrace. And one can be fixed automatically (but not done yet): when the computation of a lazy raise an exception we can store the backtrace and reraise it when the lazy is forced (cf. Camlinternallazy.force_lazy_block). But that increases the memory cost of a lazy that raises an exception, so is it worth it?

bobot · 2015-12-27T11:31:34Z

byterun/backtrace.c

@damiendoligez or @xavierleroy Could you check that my refinement of this comment is correct? If I'm wrong other part of this PR is wrong. Thank you.

Looks right.

xavierleroy · 2015-12-27T11:45:33Z

I'm not fully functional at the moment, so instead of reviewing code I'll discuss the color of the bike shed. Concerning the name of the primitive, why not raise_with_backtrace instead of reraise_raw_backtrace?

gasche · 2015-12-27T12:08:58Z

I reviewed the code and I think that it is good. The use that the primitive that is made in #374 is convincing (it brings a solution that is both definitive and non-invasive to a tricky problem), which makes me confident that the primitive is useful and that it would be interesting to have it for 4.03.

The previous implementation of reraise_raw_backtrace would only work if the backtrace buffer had already been initialized (by a previous stash call in the same runtime); the current one is more general, and supports the naming change that Xavier suggests: raise_with_backtrace sounds nicer indeed.

One small remark: raise_with_backtrace currently assumes that the raw backtrace is represented as an OCaml array, but @bobot convincingly argues that we should change that in the future (to make restoring the backtrace faster). @bobot, could you add an explicit comment on the primitive that people should not assume that arbitrary OCaml arrays can be passed, only values of the Printexc-private raw_backtrace type, as the representation may change in the future?

bobot · 2015-12-27T22:30:17Z

With the name raise_with_backtrace, the bike shed has indeed a pretty color. The rename is done. However I should remark that the color of the bike itself is not so well assorted, due to the multiple layer of paint: what is called backtrace in the runtime is called raw_backtrace in Printexc, what is called backtrace in Printexc is the string of the backtrace. This new name goes in the right direction.

@gasche I don't understand where you want me to add the comment. There is already one about the conversion of raw_backtrace to raw_backtrace_slot array:

external slot_array_of_raw_backtrace: raw_backtrace -> raw_backtrace_slot array
  = "%identity"
(** Currently raw_backtrace are arrays of raw_backtrace_slot, but it
    would be better that raw_backtrace match exactly the format of
    caml_backtrace_buffer in order to make {!get_raw_backtrace} to
    {!reraise_raw_backtrace} round-trip cost as small as possible. *)

I must temper the definitive qualification. The primitive raise_with_backtrace is a definitive answer for restoring the backtrace saved at get_raw_backtrace however the backtrace at get_raw_backtrace could already be the wrong one, for example if a when clause used backtrace. There is example of that in backtrace2.ml, case Error "f". The last part of #293 is still needed.

gasche · 2015-12-28T06:02:04Z

byterun/backtrace.c

@bobot : I was thinking of commenting here, to avoid people calling this function on arrays they have created themselves on the OCaml side.

bobot · 2015-12-28T22:03:14Z

The comment is added but I think the runtime should have a way to indicate that an exported symbol should not be used, or at least that we have no backward compatibility requirement for this symbol.

gasche · 2015-12-30T15:15:52Z

@xavierleroy , @damiendoligez, @alainfrisch or @def-lkb, any opinion on this change? I would like to merge it for 4.03, but I think it needs at least one extra pair of eyes.

mshinwell · 2015-12-30T16:25:50Z

I am going to be a spoilsport, as per #358, and suggest that this wait rather than being rushed in.

gasche · 2016-03-04T14:12:55Z

This issue is now tracked by @AltGr's independently-submitted mantis ticket MPR#7163. It's good to have a mantis ticket for it, and we should refer to it in Changes when applicable.

bobot · 2016-04-18T09:30:09Z

Could people have a new look at this PR for 4.04?

gasche · 2016-04-18T11:21:20Z

I would like indeed to merge this in trunk, but probably after the remaining release work is done.

gasche · 2016-05-30T19:57:05Z

I like this change and, now that #247 has been merged, I would like to move forward and eventually merge it -- along with its sister PR #374 -- in the trunk. @mshinwell (or anyone else), do you have objections?

bobot · 2016-07-08T12:18:55Z

@gasche, @damiendoligez It was accepted at the last developer meeting. I just rebased and moved the bootstrap to #374 (the developers asked to remove the bootstrap but in fact the bootstrap is needed since it is a compiler primitive and not a primitive of the runtime).

let-def · 2016-07-17T06:22:21Z

asmrun/backtrace_prim.c

  }
 }

+int caml_alloc_backtrace_buffer(){


Isn't int caml_alloc_backtrace_buffer(void){ still preferable (even in C-99)?

Yes indeed. It is fixed. I tried to add the warning that detect such errors bobot/ocaml@d1f37d7, but it will wait another PR (and when I have less open PR).

let-def · 2016-07-17T06:44:18Z

The approach is good and the code looks fine to me once the issues above are addressed.

The reference given are the current results not the ideal results. The tests show cases where the backtrace we want to use during reraise have been lost by intermediary raise.

It reraises an exception just after copying the given backtrace to the backtrace buffer. The C primitive `caml_restore_raw_backtrace` only does the copying part, the compiler adds the reraise.

xavierleroy · 2016-07-29T08:59:53Z

Call me grumpy, but I'm not completely happy with emitting code to set caml_backtrace_pos to 0 at every Lraise operation (of the non-reraise kind). @alainfrisch's original design, with the two runtime functions caml_raise_exn and caml_reraise_exn, is better for code compactness.

bobot · 2016-07-29T11:05:31Z

That would be indeed better to not increase code size. I would like to try to not reintroduce architecture specific code. I can add the generation at link time (at the same time and way (in cmmgen) than caml_curry* and caml_apply*) of a function that does this two instructions and that would be called for raise. Would that be better and possible?

bobot · 2016-07-29T11:10:19Z

That is not so simple because the pc and sp would be of this new function and not its caller.

gasche · 2016-07-29T13:47:57Z

I merged @bobot's last patch for i386nt, because I'd like to get the CI in a clean state. However, I won't have much time or connectivity in the following days, so I probably won't be able to follow the current discussion. I would be happy if someone else took care of merging or reverting changes as deemed necessary from now on. ( @alainfrisch maybe? )

…acktrace_primitive" This reverts commit 5adf895, reversing changes made to 38c3db4. The reason for the revert is a continuous integration failure on 32bit arm machines. I don't have the time (or capabilities) to investigate it right now, and we need functional continuous testing for other upcoming merges, so the safest choice is to revert -- and hopefully merge back after the issue is fixed.

bobot · 2016-12-05T12:25:13Z

Oh... @gasche I didn't understood that the changed was never merged back! I though you did it before merging my last patch. Or I'm missing something?

gasche · 2016-12-05T12:32:21Z

Woops, sorry. I must admit that I forgot about it, and I don't remember what I did or didn't do.

bobot · 2016-12-05T15:50:26Z

Funnily I don't understand what went wrong. You merged again my branch into trunk but only a part of the modification get back. In fact merging again bobot:feature/reraise_raw_backtrace_primitive changes nothing. I'm still looking at how to make the change come back. And for the futur what is the good way to revert a merge, and after the fix is known how to get back the changes (revert the revert? and then merging again?).

…se_raw_backtrace_primitive"" This reverts commit f1b63d4. Conflicts: Changes

…backtrace_primitive" This reverts commit f1b63d4. It is the second time ocaml#378 tried to be recovered (e6c779a). Conflicts: Changes

bobot · 2016-12-05T16:11:50Z

Reverting the revert seems to work bobot/ocaml@1248563 (branch revert-revert-reraise) , could you take it?

bobot · 2016-12-05T16:16:20Z

Travis failing, I will come back with a new GPR.

…backtrace_primitive" This reverts commit f1b63d4. It is the second time ocaml#378 tried to be recovered (e6c779a). Conflicts: Changes

bobot · 2016-12-05T16:55:57Z

It was just a change of line number in hashtbl module, it doesn't mandate the trouble of a new GPR. Here the successful travis job: https://travis-ci.org/bobot/ocaml/builds/181390977 .

…backtrace_primitive" This reverts commit f1b63d4. It is the second time ocaml#378 tried to be recovered (e6c779a).

…race_primitive" This reverts commit f1b63d4. It is the second time #378 tried to be recovered (e6c779a).

…_primitive Add reraise_raw_backtrace primitive

…acktrace_primitive" This reverts commit 5adf895, reversing changes made to 38c3db4. The reason for the revert is a continuous integration failure on 32bit arm machines. I don't have the time (or capabilities) to investigate it right now, and we need functional continuous testing for other upcoming merges, so the safest choice is to revert -- and hopefully merge back after the issue is fixed.

…ace_primitive' into trunk This restores the merge of ocaml#378 which had been previously reverted to fix reraise support in some architectures: ocaml#378 (comment)

…backtrace_primitive" This reverts commit f1b63d4. It is the second time ocaml#378 tried to be recovered (e6c779a).

hhugo · 2017-11-04T19:38:20Z

Just reported an issue with the reraise_raw_backtrace primitive
see https://caml.inria.fr/mantis/view.php?id=7666

nojb · 2017-11-14T17:42:13Z

bytecomp/translcore.ml

+                               e.exp_loc)),
+                 wrap0 (Lprim(Praise Raise_reraise,
+                              [event_after texn1 (Lvar vexn)],
+                              e.exp_loc))


I think having both wrap and wrap0 here is a mistake, since any extra arguments in args' will be evaluated twice. Am I missing something?

Related to #1465.

In fact this causes a segmentation fault, see https://caml.inria.fr/mantis/view.php?id=7674

Hold onto empty pools if swept while allocating

…#378)

* title case and grammar/syntax in packages.eml * fixing two more title case in packages.eml * changed to Learn More Signed-off-by: Christine Rose <[email protected]>

bobot mentioned this pull request Dec 27, 2015

Use reraise_raw_backtrace primitive in the compiler #374

Merged

bobot reviewed Dec 27, 2015
View reviewed changes

bobot force-pushed the feature/reraise_raw_backtrace_primitive branch 2 times, most recently from e61dabd to cfdda8e Compare December 27, 2015 18:40

gasche reviewed Dec 28, 2015
View reviewed changes

bobot force-pushed the feature/reraise_raw_backtrace_primitive branch from cfdda8e to ab8fc27 Compare December 28, 2015 21:56

gasche mentioned this pull request Dec 31, 2015

Proper backtraces upon compiler failure #358

Closed

gasche added the caml-weekly-news label Jan 12, 2016

damiendoligez added this to the 4.04-or-later milestone Jan 22, 2016

bobot force-pushed the feature/reraise_raw_backtrace_primitive branch 2 times, most recently from 76aa01e to d1c766f Compare July 7, 2016 19:41

bobot force-pushed the feature/reraise_raw_backtrace_primitive branch from d1c766f to 4aafc11 Compare July 8, 2016 14:24

bobot mentioned this pull request Jul 8, 2016

More precise and efficient handling of backtraces #293

Closed

let-def reviewed Jul 17, 2016
View reviewed changes

bobot force-pushed the feature/reraise_raw_backtrace_primitive branch from e991e31 to dd827d8 Compare July 17, 2016 20:27

bobot added 2 commits July 17, 2016 22:37

Add tests for wrong backtrace with reraise

e24af07

The reference given are the current results not the ideal results. The tests show cases where the backtrace we want to use during reraise have been lost by intermediary raise.

Add %reraise_raw_backtrace primitive

10f5095

It reraises an exception just after copying the given backtrace to the backtrace buffer. The C primitive `caml_restore_raw_backtrace` only does the copying part, the compiler adds the reraise.

bobot added a commit to bobot/ocaml that referenced this pull request Dec 5, 2016

Revert "Revert "Merge pull request ocaml#378 from bobot/feature/rerai…

903f151

…se_raw_backtrace_primitive"" This reverts commit f1b63d4. Conflicts: Changes

bobot mentioned this pull request Dec 6, 2016

Fix the partial merge of #378 #951

Closed

gasche pushed a commit to gasche/ocaml that referenced this pull request Feb 13, 2017

Recover "Merge pull request ocaml#378 from bobot/feature/reraise_raw_…

42b4e0a

…backtrace_primitive" This reverts commit f1b63d4. It is the second time ocaml#378 tried to be recovered (e6c779a).

gasche pushed a commit that referenced this pull request Feb 14, 2017

Recover "Merge pull request #378 from bobot/feature/reraise_raw_backt…

50e9b96

…race_primitive" This reverts commit f1b63d4. It is the second time #378 tried to be recovered (e6c779a).

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request ocaml#378 from bobot/feature/reraise_raw_backtrace…

40ad95a

…_primitive Add reraise_raw_backtrace primitive

nojb mentioned this pull request Nov 5, 2017

Fix handling of Printexc.raise_with_backtrace under partial application #1465

Closed

nojb reviewed Nov 14, 2017

View reviewed changes

This was referenced Mar 14, 2019

Backtrace-friendly finalizers #7163

Closed

miscompilation of the "%raise_with_backtrace" primitive. #7666

Closed

anmolsahoo25 pushed a commit to anmolsahoo25/ocaml that referenced this pull request Aug 25, 2020

Merge pull request ocaml#378 from ctk21/dont_return_swept_if_allocating

c42ed52

Hold onto empty pools if swept while allocating

mshinwell added a commit to mshinwell/ocaml that referenced this pull request Mar 29, 2021

Remove remaining uses of Expr.free_names (except in test code) (ocaml…

22b0962

…#378)

chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Jan 4, 2022

Inline generative effects in cmm (ocaml#378)

5c45346

Conversation

bobot commented Dec 27, 2015

Uh oh!

bobot Dec 27, 2015

Choose a reason for hiding this comment

Uh oh!

damiendoligez Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

xavierleroy commented Dec 27, 2015

Uh oh!

gasche commented Dec 27, 2015

Uh oh!

bobot commented Dec 27, 2015

Uh oh!

gasche Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

bobot commented Dec 28, 2015

Uh oh!

gasche commented Dec 30, 2015

Uh oh!

mshinwell commented Dec 30, 2015

Uh oh!

gasche commented Mar 4, 2016

Uh oh!

bobot commented Apr 18, 2016

Uh oh!

gasche commented Apr 18, 2016

Uh oh!

gasche commented May 30, 2016

Uh oh!

bobot commented Jul 8, 2016

Uh oh!

let-def Jul 17, 2016

Choose a reason for hiding this comment

Uh oh!

bobot Jul 17, 2016

Choose a reason for hiding this comment

Uh oh!

let-def commented Jul 17, 2016

Uh oh!

xavierleroy commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobot commented Jul 29, 2016

Uh oh!

bobot commented Jul 29, 2016

Uh oh!

gasche commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobot commented Dec 5, 2016

Uh oh!

gasche commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobot commented Dec 5, 2016

Uh oh!

bobot commented Dec 5, 2016

Uh oh!

bobot commented Dec 5, 2016

Uh oh!

bobot commented Dec 5, 2016

Uh oh!

hhugo commented Nov 4, 2017

Uh oh!

nojb Nov 14, 2017

Choose a reason for hiding this comment

Uh oh!

nojb Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

xavierleroy commented Jul 29, 2016 •

edited

Loading

gasche commented Jul 29, 2016 •

edited

Loading

gasche commented Dec 5, 2016 •

edited

Loading