| Anonymous | Login | Signup for a new account | 2013-05-25 09:14 CEST | ![]() |
| Main | My View | View Issues | Change Log | Roadmap |
| View Issue Details [ Jump to Notes ] | [ Issue History ] [ Print ] | ||||||||||
| ID | Project | Category | View Status | Date Submitted | Last Update | ||||||
| 0005899 | OCaml | OCaml standard library | public | 2013-01-22 15:19 | 2013-04-23 11:52 | ||||||
| Reporter | frisch | ||||||||||
| Assigned To | jacques-henri.jourdan | ||||||||||
| Priority | normal | Severity | feature | Reproducibility | have not tried | ||||||
| Status | assigned | Resolution | open | ||||||||
| Platform | OS | OS Version | |||||||||
| Product Version | |||||||||||
| Target Version | Fixed in Version | ||||||||||
| Summary | 0005899: Expose a way to inspect the current call stack | ||||||||||
| Description | The machinery required to support exception traces actually makes it possible to return, at a given point in the program, (a fragment of) the call stack of the current context. This can be useful on its own. I can think of the following uses: - debug; - log the context of an error, where it is detected, but without failing; - run the code with exception traces disabled, for performance reasons, but manually extract the stack and put it as an argument of some specific exception, when it is raised; - instrument the code to perform context-sensitive profiling (how many times is this function called and from which call sites). I attach a patch which concretely reuses the mechanism for exception traces. With more effort, it would be possible to do something cleaner. And there is no reason to put the new feature in Printexc, since it is not related to exceptions at all. | ||||||||||
| Tags | No tags attached. | ||||||||||
| Attached Files | |||||||||||
Relationships |
||||||
|
||||||
Notes |
|
|
(0008779) dbuenzli (reporter) 2013-01-22 15:48 |
A little bit OT but could we at the same time have the same resulting type as Printexc.current_call_stack for a variant of Printexc.get_backtrace ? I already had to parse this string more than once (notably to be able to pretty print it). Thanks. |
|
(0008780) frisch (developer) 2013-01-22 15:48 |
Daniel: agreed! |
|
(0008781) shinwell (developer) 2013-01-22 15:54 |
We already have something similar in Core (see module [Backtrace]). I was planning to clean this up; maybe we could merge these patches. |
|
(0008782) frisch (developer) 2013-01-22 16:01 |
Mark: great! Let's wait to see if there is any theoretical opposition against the proposal from other core developers. I really believe the feature deserves to be in the core system. |
|
(0008783) dim (developer) 2013-01-22 16:47 |
I tried that too for Lwt once ;-) But it wasn't enough so I didn't proposed it. I'm also for having this. |
|
(0009129) jacques-henri.jourdan (manager) 2013-04-16 11:14 |
What I propose is the following : 1- Two functions set_backtrace_limit : option int -> unit and get_backtrace_limit : unit -> option int. set_backtrace_limit changes the mode of the backtrace recorder. * If called with None, we have the current behavior * If called with Some l, the l last stack cells will be recorded, even if they are beyond the exception handler. This allows the client code to easily implement the current_call_stack function itself, while allowing to record a full backtrace when an exception is raised for debugging purposes. 2- Exposing the backtrace internal type of the Printexc module, and the function convert_raw_backtrace : raw_backtrace -> backtrace option, in order to make the client code able to manipulate the backtrace programatically. I am not really in favor of 2), because what is recorded in the backtrace is not well specified. It has subtle interaction with the "-g" flag, with whether the code is executed in bytecode mode or in native mode... |
|
(0009131) gasche (developer) 2013-04-16 11:54 edited on: 2013-04-16 12:00 |
So the way to implement current_call_stack would be to raise an exception and immediately catch it back, with a longer backtrace limit? I think that's a bit ugly, and besides you have no way to tell in your interface that you want the trace upto the beginning of the application (sure you can put max_int there, but urgh.). On the other hand, if you had a feature to just return the N last cells (or all upto the start if None is passed), you could also express the behavior of recording "beyond the exception handler" in user-code: just use this new feature in the "with" block, that by construction will see the same cell history as the "raise" it catched (I mean "get 10 more cells higher than my try" at raise point is equivalent to "get 10 cells" at the handler point). So while I agree that "get farther than my raise" and "get me some cells" are equivalently expressive, I think the latter is a less convoluted primitive to start with. |
|
(0009137) frisch (developer) 2013-04-16 13:18 |
> On the other hand, if you had a feature to just return the N last cells (or all upto the start if None is passed) I agree that this is indeed more user-friendly. |
|
(0009140) gasche (developer) 2013-04-16 14:36 |
Another point about the different interface: it allows to have the number of desired cell in the function call, instead of as a global mutable variable (which was necessary with the previous model, as "raise" cannot be parametrized). |
|
(0009146) jacques-henri.jourdan (manager) 2013-04-16 15:54 |
I understand that not using a global variable seems to be a good thing. However, for debugging, using a global variable is convenient, as it does not imply modifying the handler code. Concerning the fact that there is no way to ask for the whole trace, I am in favor of that : the user has to know that putting a big value here could be a performance problem in case of a big backtrace. |
|
(0009147) frisch (developer) 2013-04-16 16:38 |
> However, for debugging, using a global variable is convenient, as it does not imply modifying the handler code. I don't known what kind of debugging you have in mind, but for many uses cases of the proposed feature ("get a description of the top N slots of the current call stack"), there won't even be any exception at all in the client code. |
|
(0009150) jacques-henri.jourdan (manager) 2013-04-16 17:18 |
> there won't even be any exception at all in the client code. This is not a real problem: you can create one as in the proposed patch. I agree that for the use cases of the proposed feature, the problem does not change. However, I see an other use case : what I have in mind is that sometimes it is not enough to see the backtrace of an exception, and having some context on where it has been catched is usefull. Both approaches can help. It can be achieved with the global variable approach by modifying the global variable, and in the gasche's approach by modifying the handler's code. My point is that while debugging, modifying a global variable is maybe more convenient than modifying a handler : there might be several interesting handlers, you have to keep track where they are... By the way, I am thinking of an other feature that could be interesting : why not adding a record_and_raise primitive of the Printexc module, that raises an exception, recording its backtrace even if the backtrace flag is deactivated. That could be interesting, for example, when a performance critical code raises an exception signaling an error, while this code uses exceptions for control (so that you do not want to activate backtraces globally). What do you think ? |
|
(0009151) frisch (developer) 2013-04-16 17:32 |
> This is not a real problem: you can create one as in the proposed patch. I'm not against the global variables (in Printexc) if you think it can simplify debugging existing code, but the standard library should also expose a nice API (maybe not in Printexc) to retrieve with top of the stack without a hack (changing a global value, raising a dummy exception, filtering the result, restoring the global value). > why not adding a record_and_raise primitive of the Printexc module, that raises an exception, recording its backtrace even if the backtrace flag is deactivated. I don't think you can easily traverse intermediate exception handlers which re-raise the exception. > for example, when a performance critical code raises an exception signaling an error I don't think that in practice people will segregate their code into performance-critical modules (compiled without -g) and others (compiled with -g). Tweaking compilation flags on a file-by-file basis is not realistic for large code bases. The safe assumption is rather that code running production will be entirely compiled with -g and executed with stacktrace enabled, and then it makes more sense to provide a light "raise" for those cases where performance matters. |
|
(0009169) jacques-henri.jourdan (manager) 2013-04-21 17:01 |
I uploaded a patch to implement the feature. It brings the following advantages from the original one : - It does not use an exception in the standard library code. That is, it does not erase the exception backtrace buffer. - It is possible to give a maximum number of frames to record. It is still needed to provide a way to programmatically inspect raw_bactrace inhabitants. On this question, I do not know what is the best thing to do : we can either make public the "backtrace" private type of the Printexc module, or provide an interface in the style of the alain's patch. |
|
(0009170) gasche (developer) 2013-04-21 19:07 edited on: 2013-04-21 19:13 |
Call me superstitious, but I would feel better without the name change and corresponding asm-level renames. What about just keeping ..._backtrace and adding ..._backtrace_custom? Regarding programmatic inspection, I think that the "backtrace" type inside the Printexc module should remain abstract (to allow for easy change of implementation later), with a conversion function (that may be just the identity) to a public type, that may be either the current type or Alain's simpler interface. Of course, so far we aren't exposing any of the functions manipulating this middle-end type, so the distinction between this and a fully public type does not exist yet. |
|
(0009178) jacques-henri.jourdan (manager) 2013-04-22 12:14 |
New version : keeping the names of the ASM functions, added list_of_raw_backtrace. |
|
(0009179) frisch (developer) 2013-04-22 12:17 |
I believe the new [current_backtrace] function should rather be in Pervasives since it is not related to exceptions. |
|
(0009182) gasche (developer) 2013-04-22 13:49 |
I'm worried about the (<> "printexc.ml") hack coming from Alain's original patch. What is that supposed to do (it would deserve a comment in the code at least), and won't it blow in our face in some scenarios? (You really want to trust the backtrace-reporting functions.) Other than that, I think the documentation for current_backtrace should mention the fact that it will collecth the [n] last frames, or less if less are available. I understand the idea of not favoring the "get me all traces" scenario (though frankly I'm still not fond of having to use a high constant in this case), but the documentation should at least make clear that it is possible and won't fail instead. Alain > moving current_backtrace to Pervasives may require a bit of work, as it currently depends an the abstract type defined in Printexc, and we can't have Pervasives depending on Printexc. Before Jacques-Henri gently does the work, are you sure this is desirable, and do you have more precise guidelines on what you want (eg. should we have raw_backtrace defined in Pervasives and re-exported in Printexc? Only in pervasives? What about an eventual exposed 'backtrace' type? Should 'record_backtrace' also move?). My gut feeling is that it's a bit perfectionist. |
|
(0009183) frisch (developer) 2013-04-22 14:05 |
> What is that supposed to do In the original form, it was supposed to remove the top stack slots corresponding to the fake exception handler used to trigger the stack capture. (IIRC, we needed to drop 1 or 2 slots depending on whether we were running bytecode vs. native). > are you sure this is desirable, Desirable, yes, important no. > we have raw_backtrace defined in Pervasives and re-exported in Printexc If the two features share the same type, then yes, raw_backtrace could go to Pervasives. I don't think it is then required to re-export it from Printexc. Or maybe we could introduce a new > What about an eventual exposed 'backtrace' type Same as raw_backtrace, I guess. > Should 'record_backtrace' also move This is related to exceptions (even if not to printing them), so Printexc seems fine. An alternative could be to introduce a new stdlib module "Backtrace", where everything related to stack traces (even for exceptions) would go. Printexc would only keep the exception-printing functions (plus the one which exist in 4.00, marked as deprecated, for compatibility). But this might be overkill for a small feature, hence my suggestion to move the new things to Pervasives. But you should definitely check with Xavier! |
|
(0009184) jacques-henri.jourdan (manager) 2013-04-22 14:21 |
> In the original form, it was supposed to remove the top stack slots corresponding to the fake exception handler used to trigger the stack capture. (IIRC, we needed to drop 1 or 2 slots depending on whether we were running bytecode vs. native). I removed it. It is no longer necessary in this context. Concerning Printexc vs. Pervasives vs. Backtrace, I would be definitely in favor of putting it in a new Backtrace module, and this is not very difficult. But this is a rather more philosophical question. There is another perfectionists-kind problem : now, the printer of backtraces is shared between exception backtraces and this new kind of backtraces. This causes the first line of the backtrace to be : "Raised by primitive operation at ...." instead of "Called at ..." I do not know what kind of "primitive operation" can cause this message appear in the case of an exception (array accesses ? match failures ? static raises ? ... ?), but I wonder whether it is possible to "re-raise by primitive operation", in which case the message won't be displayed correctly anyway... |
|
(0009185) gasche (developer) 2013-04-22 15:23 |
Xavier remarks that a Backtrace module would risk conflicting with userspace modules of the same name (that exist in some dark place with probability 1). Alain > none of Jacques-Henri's recent raw_backtrace work has been released yet; as far as I know, there was no change in Printexc since 3.12, so we have no deprecation to worry about: the raw_backtrace stuff can be moved, and even renamed, if deemed necessary. After some more reflection, I find it weird that list_of_raw_backtrace may lose information if called on a trace recorded from an exception: as the re-raise information is ignored, it is only an injection if it is only used on traces produced by current_backtrace. I think we should either have two types (error_trace and call_trace) and two printing functions, or expose a representation that can fully handle both situations (eg. the one currently used in printexc). A conceptual test for any API design would be to check that the printing function(s) can be reimplemented from the user side, using only the exposed program-friendly representation. |
|
(0009192) frisch (developer) 2013-04-23 11:52 |
> none of Jacques-Henri's recent raw_backtrace work has been released yet; as far as I know, there was no change in Printexc since 3.12, so we have no deprecation to worry about I know, but there was already some backtrace-oriented functions in Printexc in 4.00 (like record_backtrace). > Xavier remarks that a Backtrace module would risk conflicting with userspace modules of the same name (that exist in some dark place with probability 1). Ah, I guess we desperately need namespaces, then. We could use OCaml_backtrace for now and hope that this namespace system will allow it to be re-mapped to just "Backtrace" in client code :-) Or just put everything in Pervasives. Note that technically, any addition to the stdlib could break existing code (with probability < 1, I agree). |
Issue History |
|||
| Date Modified | Username | Field | Change |
| 2013-01-22 15:19 | frisch | New Issue | |
| 2013-01-22 15:19 | frisch | File Added: current_call_stack.diff | |
| 2013-01-22 15:21 | frisch | Description Updated | View Revisions |
| 2013-01-22 15:48 | dbuenzli | Note Added: 0008779 | |
| 2013-01-22 15:48 | frisch | Note Added: 0008780 | |
| 2013-01-22 15:54 | shinwell | Note Added: 0008781 | |
| 2013-01-22 16:01 | frisch | Note Added: 0008782 | |
| 2013-01-22 16:47 | dim | Note Added: 0008783 | |
| 2013-01-23 09:42 | frisch | Severity | minor => feature |
| 2013-04-15 11:31 | frisch | Assigned To | => jacques-henri.jourdan |
| 2013-04-15 11:31 | frisch | Status | new => assigned |
| 2013-04-16 11:14 | jacques-henri.jourdan | Note Added: 0009129 | |
| 2013-04-16 11:54 | gasche | Note Added: 0009131 | |
| 2013-04-16 12:00 | gasche | Note Edited: 0009131 | View Revisions |
| 2013-04-16 13:18 | frisch | Note Added: 0009137 | |
| 2013-04-16 14:36 | gasche | Note Added: 0009140 | |
| 2013-04-16 15:54 | jacques-henri.jourdan | Note Added: 0009146 | |
| 2013-04-16 16:38 | frisch | Note Added: 0009147 | |
| 2013-04-16 17:18 | jacques-henri.jourdan | Note Added: 0009150 | |
| 2013-04-16 17:24 | gasche | Relationship added | related to 0005935 |
| 2013-04-16 17:32 | frisch | Note Added: 0009151 | |
| 2013-04-21 16:54 | jacques-henri.jourdan | File Added: 0001-PR-0005899-exposing-a-way-to-inspect-current-call-st.patch | |
| 2013-04-21 17:01 | jacques-henri.jourdan | Note Added: 0009169 | |
| 2013-04-21 19:07 | gasche | Note Added: 0009170 | |
| 2013-04-21 19:13 | gasche | Note Edited: 0009170 | View Revisions |
| 2013-04-22 12:12 | jacques-henri.jourdan | File Deleted: 0001-PR-0005899-exposing-a-way-to-inspect-current-call-st.patch | |
| 2013-04-22 12:13 | jacques-henri.jourdan | File Added: 0001-PR-0005899-exposing-a-way-to-inspect-current-call-st.patch | |
| 2013-04-22 12:14 | jacques-henri.jourdan | Note Added: 0009178 | |
| 2013-04-22 12:17 | frisch | Note Added: 0009179 | |
| 2013-04-22 13:49 | gasche | Note Added: 0009182 | |
| 2013-04-22 14:05 | frisch | Note Added: 0009183 | |
| 2013-04-22 14:06 | jacques-henri.jourdan | File Deleted: 0001-PR-0005899-exposing-a-way-to-inspect-current-call-st.patch | |
| 2013-04-22 14:07 | jacques-henri.jourdan | File Added: 0001-PR-0005899-exposing-a-way-to-inspect-current-call-st.patch | |
| 2013-04-22 14:21 | jacques-henri.jourdan | Note Added: 0009184 | |
| 2013-04-22 15:23 | gasche | Note Added: 0009185 | |
| 2013-04-23 11:52 | frisch | Note Added: 0009192 | |
| Copyright © 2000 - 2011 MantisBT Group |