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

Add reraise_raw_backtrace primitive #378

Merged
merged 6 commits into from Jul 27, 2016

Conversation

Projects
None yet
9 participants
@bobot
Contributor

bobot commented Dec 27, 2015

(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?

needed in order to simplify the interface with the thread
library (thread creation doesn't need to allocate
caml_backtrace_buffer). So we don't have to allocate it here.
*/

This comment has been minimized.

@bobot

bobot Dec 27, 2015

Contributor

@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.

@bobot

bobot Dec 27, 2015

Contributor

@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.

This comment has been minimized.

@damiendoligez

damiendoligez Jan 22, 2016

Member

Looks right.

@damiendoligez

damiendoligez Jan 22, 2016

Member

Looks right.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 27, 2015

Contributor

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?

Contributor

xavierleroy commented Dec 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 27, 2015

Member

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?

Member

gasche commented Dec 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Dec 27, 2015

Contributor

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.

Contributor

bobot commented Dec 27, 2015

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.

Show outdated Hide outdated byterun/backtrace.c
@@ -152,6 +154,35 @@ CAMLprim value caml_get_exception_raw_backtrace(value unit)
CAMLreturn(res);
}
/* Copy back a backtrace and exception to the global state */
/* noalloc (caml value): so no CAMLparam* CAMLreturn* */

This comment has been minimized.

@gasche

gasche Dec 28, 2015

Member

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

@gasche

gasche Dec 28, 2015

Member

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

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Dec 28, 2015

Contributor

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.

Contributor

bobot commented Dec 28, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 30, 2015

Member

@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.

Member

gasche commented Dec 30, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 30, 2015

Contributor

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

Contributor

mshinwell commented Dec 30, 2015

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

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

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 4, 2016

Member

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.

Member

gasche commented Mar 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Apr 18, 2016

Contributor

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

Contributor

bobot commented Apr 18, 2016

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

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Apr 18, 2016

Member

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

Member

gasche commented Apr 18, 2016

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

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche May 30, 2016

Member

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?

Member

gasche commented May 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 8, 2016

Contributor

@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).

Contributor

bobot commented Jul 8, 2016

@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).

Show outdated Hide outdated asmrun/backtrace_prim.c
@@ -67,6 +67,14 @@ frame_descr * caml_next_frame_descriptor(uintnat * pc, char ** sp)
}
}
int caml_alloc_backtrace_buffer(){

This comment has been minimized.

@let-def

let-def Jul 17, 2016

Contributor

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

@let-def

let-def Jul 17, 2016

Contributor

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

This comment has been minimized.

@bobot

bobot Jul 17, 2016

Contributor

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).

@bobot

bobot Jul 17, 2016

Contributor

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).

Show outdated Hide outdated asmrun/backtrace_prim.c
@@ -67,6 +67,14 @@ frame_descr * caml_next_frame_descriptor(uintnat * pc, char ** sp)
}
}
int caml_alloc_backtrace_buffer(){
Assert(caml_backtrace_pos == 0);

This comment has been minimized.

@let-def

let-def Jul 17, 2016

Contributor

This seems wrong. When restoring a backtrace, you set caml_backtrace_pos first then allocate a buffer.
This assertion will fail when using reraise_raw_backtrace in debug mode!

@let-def

let-def Jul 17, 2016

Contributor

This seems wrong. When restoring a backtrace, you set caml_backtrace_pos first then allocate a buffer.
This assertion will fail when using reraise_raw_backtrace in debug mode!

This comment has been minimized.

@let-def

let-def Jul 17, 2016

Contributor

... And no buffer had been allocated before. So you probably need two steps to make the assertion fail:

  1. Capture a first backtrace.
  2. Create a new thread, restore the backtrace in this new context.
@let-def

let-def Jul 17, 2016

Contributor

... And no buffer had been allocated before. So you probably need two steps to make the assertion fail:

  1. Capture a first backtrace.
  2. Create a new thread, restore the backtrace in this new context.

This comment has been minimized.

@bobot

bobot Jul 17, 2016

Contributor

It was indeed wrong, it is fixed and I simplified the logic. Your test case has been added to the test suite.

@bobot

bobot Jul 17, 2016

Contributor

It was indeed wrong, it is fixed and I simplified the logic. Your test case has been added to the test suite.

@let-def

This comment has been minimized.

Show comment
Hide comment
@let-def

let-def Jul 17, 2016

Contributor

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

Contributor

let-def commented Jul 17, 2016

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

bobot added some commits Oct 13, 2015

Add tests for wrong backtrace with reraise
    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
  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.
Precise where reliable backtrace can't be obtained
        In fact currently even if you use `print_backtrace`,
        `get_backtrace` or `get_raw_backtrace` just at the start of an
        exception handler the backtrace can be wrong if an exception
        have been raised and catched in a `when` clause.
Fix caml_backtrace_pos invariant in caml_restore_raw_backtrace
    And simplify this function.

  Thx @let-def (Frédéric Bour)
@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 17, 2016

Contributor

Rebased and the issues should be fixed.

Contributor

bobot commented Jul 17, 2016

Rebased and the issues should be fixed.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 27, 2016

Contributor

@mshinwell Could you merge it? @let-def seems happy.

Contributor

bobot commented Jul 27, 2016

@mshinwell Could you merge it? @let-def seems happy.

- GPR#378: Add [Printexc.raise_with_backtrace] raise an exception using
an explicit backtrace
(François Bobot, review by Gabriel Scherer, Xavier Leroy, Damien Doligez,
Frédéric Bour)

This comment has been minimized.

@gasche

gasche Jul 27, 2016

Member

is there an encoding issue here?

@gasche

gasche Jul 27, 2016

Member

is there an encoding issue here?

This comment has been minimized.

@bobot

bobot Jul 27, 2016

Contributor

Strangely only in the full change window. emacs tells that it is usual UTF8, and file too UTF-8 Unicode text

@bobot

bobot Jul 27, 2016

Contributor

Strangely only in the full change window. emacs tells that it is usual UTF8, and file too UTF-8 Unicode text

@gasche gasche merged commit 5adf895 into ocaml:trunk Jul 27, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gasche added a commit that referenced this pull request Jul 27, 2016

Revert "Merge pull request #378 from bobot/feature/reraise_raw_backtr…
…ace_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.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jul 27, 2016

Member

I just reverted the merge (I didn't know github had a "revert" option...) because of a continuous integration failure on ARM 32bits:

List of failed tests:
    tests/backtrace/'backtrace2.ml' with ocamlopt

I don't have the means to investigate the failure, and on previous problematic PRs I had the impression that reverting immediately would have been the better long-term solution -- in particular in terms of keeping our CI infrastructure effective.

I would be happy to merge this back after the 32bit-arm issue is understood and fixed.

Member

gasche commented Jul 27, 2016

I just reverted the merge (I didn't know github had a "revert" option...) because of a continuous integration failure on ARM 32bits:

List of failed tests:
    tests/backtrace/'backtrace2.ml' with ocamlopt

I don't have the means to investigate the failure, and on previous problematic PRs I had the impression that reverting immediately would have been the better long-term solution -- in particular in terms of keeping our CI infrastructure effective.

I would be happy to merge this back after the 32bit-arm issue is understood and fixed.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 27, 2016

Contributor

@gasche That was the right move. I can dig an ARM 32bits to investigate. Thanks.

Contributor

bobot commented Jul 27, 2016

@gasche That was the right move. I can dig an ARM 32bits to investigate. Thanks.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 27, 2016

Contributor

The bug is just that reraise have been added only in amd64 and i386 (164c307, 784b0b3) not in arm32 and arm64 (@gasche the test pass in arm64?).

In fact on arm32 and arm64 it is always a reraise, since caml_backtrace_pos is not reset at 0. @alainfrisch is there a problem for these two architectures?

Edit: fix commit for i386.

Contributor

bobot commented Jul 27, 2016

The bug is just that reraise have been added only in amd64 and i386 (164c307, 784b0b3) not in arm32 and arm64 (@gasche the test pass in arm64?).

In fact on arm32 and arm64 it is always a reraise, since caml_backtrace_pos is not reset at 0. @alainfrisch is there a problem for these two architectures?

Edit: fix commit for i386.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jul 27, 2016

Contributor

No problem, it's just that I'm not familiar with non-Intel assembly languages and I don't have easy access to non-Intel hardware for testing. I guess it's not just arm, but other backends as well. (I did not realize that it is always a reraise -- of course this is not very good.)

Contributor

alainfrisch commented Jul 27, 2016

No problem, it's just that I'm not familiar with non-Intel assembly languages and I don't have easy access to non-Intel hardware for testing. I guess it's not just arm, but other backends as well. (I did not realize that it is always a reraise -- of course this is not very good.)

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 28, 2016

Contributor

I did not realize that it is always a reraise -- of course this is not very good.

My affirmation was to strong, it is always a reraise if the exception is a constant exception (cf 6203). That's why the previous backtrace2.ml didn't spot the differences because it uses exception Error of string. The tests added by this PR use Not_found.

@alainfrisch Why do you wrote assembly for amd64 and i386, instead of just storing 0 in the symbol caml_backtrace_pos in the raise case? I think it is not a problem to loose the previous backtrace, even if caml_backtrace_active is not active just before a raise. And changing the status caml_backtrace_active loose the previous backtrace. I'm going to try that.

Contributor

bobot commented Jul 28, 2016

I did not realize that it is always a reraise -- of course this is not very good.

My affirmation was to strong, it is always a reraise if the exception is a constant exception (cf 6203). That's why the previous backtrace2.ml didn't spot the differences because it uses exception Error of string. The tests added by this PR use Not_found.

@alainfrisch Why do you wrote assembly for amd64 and i386, instead of just storing 0 in the symbol caml_backtrace_pos in the raise case? I think it is not a problem to loose the previous backtrace, even if caml_backtrace_active is not active just before a raise. And changing the status caml_backtrace_active loose the previous backtrace. I'm going to try that.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jul 28, 2016

Contributor

I guess this was to avoid one useless memory write when caml_backtrace_active is not set, but this was probably silly.

Contributor

alainfrisch commented Jul 28, 2016

I guess this was to avoid one useless memory write when caml_backtrace_active is not set, but this was probably silly.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 28, 2016

Contributor

Could you tell me why 7335d5f was needed? I though it should be an int, in cmm a Word_int.

Contributor

bobot commented Jul 28, 2016

Could you tell me why 7335d5f was needed? I though it should be an int, in cmm a Word_int.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jul 28, 2016

Contributor

I'm not sure I understand the question. That commit was needed because the version before it used to consider the variable as a 64-bit integer, but it is really 32-bit (int caml_backtrace_pos = 0; in backtrace.c). In Cmm, that would be Thirtytwo_signed I guess.

Contributor

alainfrisch commented Jul 28, 2016

I'm not sure I understand the question. That commit was needed because the version before it used to consider the variable as a 64-bit integer, but it is really 32-bit (int caml_backtrace_pos = 0; in backtrace.c). In Cmm, that would be Thirtytwo_signed I guess.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 28, 2016

Contributor

That was a silly question indeed, that's why I always use int32_t instead. Thank you.

Contributor

bobot commented Jul 28, 2016

That was a silly question indeed, that's why I always use int32_t instead. Thank you.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 28, 2016

Contributor

7be0a81 should fix the problem. @alainfrisch could you review it? I will test this evening on arm32.

(Unfortunately appveyor continuous integration is not running since the PR is considered merged...)

Contributor

bobot commented Jul 28, 2016

7be0a81 should fix the problem. @alainfrisch could you review it? I will test this evening on arm32.

(Unfortunately appveyor continuous integration is not running since the PR is considered merged...)

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 28, 2016

Contributor

I don't remember how to check that all the architecture compiles.

Contributor

bobot commented Jul 28, 2016

I don't remember how to check that all the architecture compiles.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jul 28, 2016

Contributor

make check_all_arches

About the last commit: I'd suggest to change the definition of the Linearize.Lraise constructor, since the distinction between raise and reraise is now handled before this representation. We only need to distinguish between with/without trace (=> Boolean, or introduce a new variant type).

Contributor

alainfrisch commented Jul 28, 2016

make check_all_arches

About the last commit: I'd suggest to change the definition of the Linearize.Lraise constructor, since the distinction between raise and reraise is now handled before this representation. We only need to distinguish between with/without trace (=> Boolean, or introduce a new variant type).

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 28, 2016

Contributor

I'd suggest to change the definition of the Linearize.Lraise since the distinction between raise and reraise is now handled before this representation.

Ok I did it since cmm since it is handled in cmmgen: bb4a1b4.

Contributor

bobot commented Jul 28, 2016

I'd suggest to change the definition of the Linearize.Lraise since the distinction between raise and reraise is now handled before this representation.

Ok I did it since cmm since it is handled in cmmgen: bb4a1b4.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 28, 2016

Contributor

@gasche now the branch works on arm 32bits.

Contributor

bobot commented Jul 28, 2016

@gasche now the branch works on arm 32bits.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jul 28, 2016

Member

What about power, s390x, sparc, do they also need to be adapted?

(Unfortunately I reverted as soon as I noticed the arm32 failure, so I cannot tell if those arches worked on the CI. We could have one more round of CI golf to know all architectures that fail.)

Edit: that was a silly question, I now see that your patch touches all architectures. Will merge again -- e6c779a .

Member

gasche commented Jul 28, 2016

What about power, s390x, sparc, do they also need to be adapted?

(Unfortunately I reverted as soon as I noticed the arm32 failure, so I cannot tell if those arches worked on the CI. We could have one more round of CI golf to know all architectures that fail.)

Edit: that was a silly question, I now see that your patch touches all architectures. Will merge again -- e6c779a .

gasche added a commit that referenced this pull request Jul 28, 2016

Merge remote-tracking branch 'github-bobot/feature/reraise_raw_backtr…
…ace_primitive' into trunk

This restores the merge of #378 which had been previously reverted to
fix reraise support in some architectures:
  #378 (comment)
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jul 29, 2016

Member

There is now a CI failure with msvc-32, and it is the only one.

List of failed tests:
    tests/backtrace/'backtrace3.ml' with ocamlopt
    tests/backtrace/'backtrace.ml' with ocamlopt and argument 'b'
    tests/backtrace/'backtrace_slots.ml' with ocamlopt
    tests/backtrace/'raw_backtrace.ml' with ocamlopt
    tests/backtrace/'backtrace2.ml' with ocamlopt
    tests/backtrace/'backtrace.ml' with ocamlopt and argument 'd'
    tests/backtrace/'backtrace_deprecated.ml' with ocamlopt

More precisely:

Running tests from 'tests/backtrace' ...
 ... testing 'backtrace.ml' with ocamlc and argument 'a': => passed
 ... testing 'backtrace.ml' with ocamlc and argument 'b': => passed
 ... testing 'backtrace.ml' with ocamlc and argument 'c': => passed
 ... testing 'backtrace.ml' with ocamlc and argument 'd': => passed
 ... testing 'backtrace.ml' with ocamlc and argument '': => passed
 ... testing 'backtrace2.ml' with ocamlc: => passed
 ... testing 'backtrace3.ml' with ocamlc: => passed
 ... testing 'raw_backtrace.ml' with ocamlc: => passed
 ... testing 'backtrace_deprecated.ml' with ocamlc: => passed
 ... testing 'backtrace_slots.ml' with ocamlc: => passed
 ... testing 'pr6920_why_at.ml' with ocamlc: => passed
 ... testing 'pr6920_why_swallow.ml' with ocamlc: => passed
 ... testing 'inline_test.ml' with ocamlc: => passed
 ... testing 'inline_traversal_test.ml' with ocamlc: => passed
 ... testing 'backtrace.ml' with ocamlopt and argument 'a': => passed
 ... testing 'backtrace.ml' with ocamlopt and argument 'b': => failed
 ... testing 'backtrace.ml' with ocamlopt and argument 'c': => passed
 ... testing 'backtrace.ml' with ocamlopt and argument 'd': => failed
 ... testing 'backtrace.ml' with ocamlopt and argument '': => passed
 ... testing 'backtrace2.ml' with ocamlopt: => failed
 ... testing 'backtrace3.ml' with ocamlopt: => failed
 ... testing 'raw_backtrace.ml' with ocamlopt: => failed
 ... testing 'backtrace_deprecated.ml' with ocamlopt: => failed
 ... testing 'backtrace_slots.ml' with ocamlopt: => failed
 ... testing 'pr6920_why_at.ml' with ocamlopt: => passed
 ... testing 'pr6920_why_swallow.ml' with ocamlopt: => passed
 ... testing 'backtraces_and_finalizers.ml' with ocamlopt: => passed
 ... testing 'inline_test.ml' with ocamlopt: => passed
 ... testing 'inline_test.ml' with ocamlopt -O3: => passed
 ... testing 'inline_traversal_test.ml' with ocamlopt: => passed
 ... testing 'inline_traversal_test.ml' with ocamlopt -O3: => passed
Member

gasche commented Jul 29, 2016

There is now a CI failure with msvc-32, and it is the only one.

List of failed tests:
    tests/backtrace/'backtrace3.ml' with ocamlopt
    tests/backtrace/'backtrace.ml' with ocamlopt and argument 'b'
    tests/backtrace/'backtrace_slots.ml' with ocamlopt
    tests/backtrace/'raw_backtrace.ml' with ocamlopt
    tests/backtrace/'backtrace2.ml' with ocamlopt
    tests/backtrace/'backtrace.ml' with ocamlopt and argument 'd'
    tests/backtrace/'backtrace_deprecated.ml' with ocamlopt

More precisely:

Running tests from 'tests/backtrace' ...
 ... testing 'backtrace.ml' with ocamlc and argument 'a': => passed
 ... testing 'backtrace.ml' with ocamlc and argument 'b': => passed
 ... testing 'backtrace.ml' with ocamlc and argument 'c': => passed
 ... testing 'backtrace.ml' with ocamlc and argument 'd': => passed
 ... testing 'backtrace.ml' with ocamlc and argument '': => passed
 ... testing 'backtrace2.ml' with ocamlc: => passed
 ... testing 'backtrace3.ml' with ocamlc: => passed
 ... testing 'raw_backtrace.ml' with ocamlc: => passed
 ... testing 'backtrace_deprecated.ml' with ocamlc: => passed
 ... testing 'backtrace_slots.ml' with ocamlc: => passed
 ... testing 'pr6920_why_at.ml' with ocamlc: => passed
 ... testing 'pr6920_why_swallow.ml' with ocamlc: => passed
 ... testing 'inline_test.ml' with ocamlc: => passed
 ... testing 'inline_traversal_test.ml' with ocamlc: => passed
 ... testing 'backtrace.ml' with ocamlopt and argument 'a': => passed
 ... testing 'backtrace.ml' with ocamlopt and argument 'b': => failed
 ... testing 'backtrace.ml' with ocamlopt and argument 'c': => passed
 ... testing 'backtrace.ml' with ocamlopt and argument 'd': => failed
 ... testing 'backtrace.ml' with ocamlopt and argument '': => passed
 ... testing 'backtrace2.ml' with ocamlopt: => failed
 ... testing 'backtrace3.ml' with ocamlopt: => failed
 ... testing 'raw_backtrace.ml' with ocamlopt: => failed
 ... testing 'backtrace_deprecated.ml' with ocamlopt: => failed
 ... testing 'backtrace_slots.ml' with ocamlopt: => failed
 ... testing 'pr6920_why_at.ml' with ocamlopt: => passed
 ... testing 'pr6920_why_swallow.ml' with ocamlopt: => passed
 ... testing 'backtraces_and_finalizers.ml' with ocamlopt: => passed
 ... testing 'inline_test.ml' with ocamlopt: => passed
 ... testing 'inline_test.ml' with ocamlopt -O3: => passed
 ... testing 'inline_traversal_test.ml' with ocamlopt: => passed
 ... testing 'inline_traversal_test.ml' with ocamlopt -O3: => passed
@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 29, 2016

Contributor

Thank you, indeed I forgot one part for i386nt.S the fix is at the tip of the branch, and it is the commit 2000ec7.

Contributor

bobot commented Jul 29, 2016

Thank you, indeed I forgot one part for i386nt.S the fix is at the tip of the branch, and it is the commit 2000ec7.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jul 29, 2016

Contributor

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.

Contributor

xavierleroy commented Jul 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 29, 2016

Contributor

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?

Contributor

bobot commented Jul 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 29, 2016

Contributor

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

Contributor

bobot commented Jul 29, 2016

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

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jul 29, 2016

Member

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? )

Member

gasche commented Jul 29, 2016

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? )

shindere added a commit to shindere/ocaml that referenced this pull request Aug 11, 2016

Revert "Merge pull request #378 from bobot/feature/reraise_raw_backtr…
…ace_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

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Dec 5, 2016

Contributor

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?

Contributor

bobot commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 5, 2016

Member

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

Member

gasche commented Dec 5, 2016

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

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Dec 5, 2016

Contributor

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?).

Contributor

bobot commented Dec 5, 2016

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?).

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

Revert "Revert "Merge pull request #378 from bobot/feature/reraise_ra…
…w_backtrace_primitive""

This reverts commit f1b63d4.

Conflicts:
	Changes

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

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

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

Conflicts:
	Changes
@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Dec 5, 2016

Contributor

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

Contributor

bobot commented Dec 5, 2016

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

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Dec 5, 2016

Contributor

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

Contributor

bobot commented Dec 5, 2016

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

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

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

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

Conflicts:
	Changes
@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Dec 5, 2016

Contributor

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 .

Contributor

bobot commented Dec 5, 2016

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 .

@bobot bobot referenced this pull request Dec 6, 2016

Closed

Fix the partial merge of #378 #951

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

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

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

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

Recover "Merge pull request #378 from bobot/feature/reraise_raw_backt…
…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 #378 from bobot/feature/reraise_raw_backtrace_prim…
…itive

Add reraise_raw_backtrace primitive

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

Revert "Merge pull request #378 from bobot/feature/reraise_raw_backtr…
…ace_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.

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

Merge remote-tracking branch 'github-bobot/feature/reraise_raw_backtr…
…ace_primitive' into trunk

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

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

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

This reverts commit f1b63d4.
It is the second time #378 tried to be recovered (e6c779a).
@hhugo

This comment has been minimized.

Show comment
Hide comment
@hhugo

hhugo Nov 4, 2017

Contributor

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

Contributor

hhugo commented Nov 4, 2017

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

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

This comment has been minimized.

@nojb

nojb Nov 14, 2017

Contributor

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.

@nojb

nojb Nov 14, 2017

Contributor

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.

This comment has been minimized.

@nojb

nojb Nov 20, 2017

Contributor

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

@nojb

nojb Nov 20, 2017

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment