Skip to content

PR#5899: user-friendly representation of backtraces#28

Closed
gasche wants to merge 9 commits intoocaml:trunkfrom
gasche:backtraces
Closed

PR#5899: user-friendly representation of backtraces#28
gasche wants to merge 9 commits intoocaml:trunkfrom
gasche:backtraces

Conversation

@gasche
Copy link
Member

@gasche gasche commented Apr 8, 2014

Joint work with Jacques-Henri Jourdan, this is the latest patchset to improve backtrace collection and inspection. A previous version of this patch was submitted by Jacques-Henri on mantis (PR#5899); the new version has a few change suggestions, and we use the fancy github PR process in the hope of getting more eyeballs on the patch.

Main changes:

  • raw_backtrace is no longer an abstract type, but rather an
    raw_backtrace_slot array, where raw_backtrace_slot is a new
    abstract type. raw_backtrace_slot elements are hashable and
    comparable. At runtime, values of this type contain either
    a bytecode pointer or a frame_descr pointer. In order to prevent the
    GC from walking through this pointer, the low-order bit is set to
    1 when stored in the array.

  • The old loc_info type is know public, renamed into backtrace_slot:

        type backtrace_slot =
          | Known_location of bool   (* is_raise *)
                            * string (* filename *)
                            * int    (* line number *)
                            * int    (* start char *)
                            * int    (* end char *)
          | Unknown_location of bool (*is_raise*)
  • new primitive :

    val convert_raw_backtrace_slot: raw_backtrace_slot -> backtrace_slot

    Rather than returning an option, it raises Failure when it is not
    possible to get the debugging information. It seems more idiomatic,
    especially because the exceptional case cannot appear only for a part
    of the executable.

  • the caml_convert_raw_backtrace primitive is removed; it is more
    difficult to implement in the C side because of the new exception
    interface described above.

  • In the bytecode runtime, the events are no longer deserialized once
    for each conversion, but once and for all at the first conversion,
    and stored in a global array (outside the OCaml heap), sorted by
    program counter value. I believe this information should not take
    much memory in practice (it uses the same order of magnitude memory
    as the bytecode executable). It also makes location lookup much more
    efficient, as a dichomoty is used instead of linear search as
    previously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is not documented, it should especially since it exposes a tuple rather than a record. Something Like

(** The type for backtrace slots. 
  {ul 
  {- [Known_location (is_raise, filename, line, schar, echar)] is for known 
     location where [is_raise] is [true] if ....}
  {- [Unknown_location is_raise] is for unknown location where [is_raise] 
     is [true if] ... }}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will do.

@alainfrisch
Copy link
Contributor

I'm undecided about exposing backtrace_slot as a concrete type, since it will make it more difficult to extend the payload later with more information. What about exposing "getter" functions for each field instead? Or if we want it to be concrete (without waiting for inline records :-)), we could return it as a record type (using dummy values such as filename = "" when the location is unknown).

@gasche
Copy link
Member Author

gasche commented Apr 17, 2014

I'm fine with getter functions. What about the following interface?

module Slot : sig
  type t

  val convert : raw_slot -> t

  val is_raise : t -> bool
  val known_location : t -> bool

  type location = {
    filename : string;
    line_number : int;
    start_char : int;
    end_char : int;
  }
  val location : t -> location option

  (* the following lower-level functions raise a Not_found exception
      if the slot's location is unknown *)
  val filename : t -> string
  val line_number : t -> int
  val start_char : t -> int
  val end_char : t -> int        
end

@alainfrisch
Copy link
Contributor

What would "location" return when the location is unknown? It it returns a dummy value, we should probably do the same for lower-level functions, no?

@alainfrisch
Copy link
Contributor

Also: the user might wonder about the purpose for Slot.t since getter functions could directly operate on row_slot. I understand the reason (avoiding to do the lookup for every call of a getter function). One possibility could be to hide this as an implementation detail by memoizing the last lookup, internally. But while this makes the API textually simpler, it also hides some performance properties that advanced users might prefer to now about. What do you think?

@gasche
Copy link
Member Author

gasche commented Apr 17, 2014

Sorry, I meant the location function to return an option -- I edited the code accordingly.

I think it's good to make people think about which kind of processing they intend to do on traces (bulk collection tasks or user information), and adapt the moment of conversion from raw to usable accordingly. @dbuenzli , as our API designer expert, do you have any opinion?

@dbuenzli
Copy link
Contributor

dbuenzli commented May 5, 2014

Somehow I didn't get the messages from the further discussion. Regarding Slot, it should IMHO follow the of_ convention, so rather than convert it should be of_raw_slot.

I would prefer has_location rather than known_location (in the sense that location is a property/field of the slot). We could also remove the function altogheter since location returns an option.

Somehow I miss the point of having both a record and accessors. Besides accessors should not raise Not_found, I think this historical error in the ocaml stdlib has to go for further stdlib developments. Safe is better than being consistent here.

@nrlucaroni
Copy link

What's the point of the getter functions if the location record is not abstract? Should't the getters be left for the components of t?

jhjourdan and others added 9 commits May 8, 2014 15:15
There are several changes:

- `raw_backtrace` is no longer an abstract type, but rather an
  `raw_backtrace_slot array`, where `raw_backtrace_slot` is a new
  abstract type. `raw_backtrace_slot` elements are hashable and
  comparable. At runtime, values of this type contain either
  a bytecode pointer or a frame_descr pointer. In order to prevent the
  GC from walking through this pointer, the low-order bit is set to
  1 when stored in the array.

- The old `loc_info` type is know public, renamed into `backtrace_slot`:

      type backtrace_slot =
        | Known_location of bool   (* is_raise *)
                          * string (* filename *)
                          * int    (* line number *)
                          * int    (* start char *)
                          * int    (* end char *)
        | Unknown_location of bool (*is_raise*)

- new primitive :

    val convert_raw_backtrace_slot: raw_backtrace_slot -> backtrace_slot

  Rather than returning an option, it raises Failure when it is not
  possible to get the debugging information. It seems more idiomatic,
  especially because the exceptional case cannot appear only for a part
  of the executable.

- the caml_convert_raw_backtrace primitive is removed; it is more
  difficult to implement in the C side because of the new exception
  interface described above.

- In the bytecode runtime, the events are no longer deserialized once
  for each conversion, but once and for all at the first conversion,
  and stored in a global array (*outside* the OCaml heap), sorted by
  program counter value. I believe this information should not take
  much memory in practice (it uses the same order of magnitude memory
  as the bytecode executable). It also makes location lookup much more
  efficient, as a dichomoty is used instead of linear search as
  previously.
Jacques-Henri initially removed the primitive, which is deprecated
since 4.01, but I suspect there still are uses in the wild. I guess we
should wait for a few more versions.
Test the behavior of the deprecated primitive [caml_get_exception_backtrace],
and minimal tests for hashing/comparison of raw backtrace slots.
- The internal [backtrace_slot] type is not exposed anymore, instead
  accessors function return orthogonal information
  (is_raise, location). This is both more extensible and more
  user-friendly.

- The [raw_backtrace_slot] is exposed separately as a low-level type
  that most users should never use. The unsafety of marshalling is
  documented. Instead of defining
  [raw_backtrace = raw_backtrace_slot array], I kept [raw_backtrace]
  an abstract type with [length] and [get] functions for
  random-access. This should allow us to change the implementation in
  the future to be more robust wrt. marshalling (boxing the trace in
  a Custom block, or even possibly the raw slots at access time).
@gasche
Copy link
Member Author

gasche commented May 9, 2014

The most recent commit ( baa26a1 ) implements the discussed API, along with a few changes to take the remarks into account (no exceptions anymore). The type raw_backtrace is now private, so that we have some leeway to work on the marshalling issue (marshalling raw bactrace or raw backtrace slot is strongly usafe right now -- this is documented) without breaking the API.

My plan is to merge this really soon (possibly tomorrow) so that 4.02 gets the runtime improvement (in particular the solution to PR#6302 that Alain needs). Luckily, the runtime side (the C code) hasn't been touched by the last commit, so it's a relatively low-risk change.

lthls pushed a commit to lthls/ocaml that referenced this pull request Aug 13, 2019
Correctly iter on static set of closures definitions
anmolsahoo25 pushed a commit to anmolsahoo25/ocaml that referenced this pull request Aug 25, 2020
Cleanup external calls to caml_stw_empty_minor_heap
lpw25 added a commit to lpw25/ocaml that referenced this pull request Oct 31, 2021
* Add support for "global" fields

* Bootstrap
lpw25 added a commit to lpw25/ocaml that referenced this pull request Nov 12, 2021
* Add support for "global" fields

* Bootstrap
stedolan added a commit to stedolan/ocaml that referenced this pull request May 24, 2022
173842c Merge flambda-backend changes
ed7eba2 Remove leading space from LINE. (oxcaml/oxcaml#484)
bd61170 Bump magic numbers (ocaml#5)
c50c47d Add CI builds with local allocations enabled
1412792 Move local allocations support behind '-extension local'
6d8e42a Better tail call behaviour in caml_applyN
c7dac3d Typemod: toplevel bindings escape even if no variables are bound
82d6c3e Several fixes for partial application and currying
d05c70c Pprintast support for new local syntax
e0e62fc Typecheck x |> f y as (f y x), not ((f y) x)
d7e34ce Remove autogeneration of @ocaml.curry
b9a0593 Port oxcaml/oxcaml#493
0a872d9 Code review fixes from oxcaml/oxcaml#491
6c168bb Remove local allocation counting
3c6e7f0 Code review fixes from oxcaml/oxcaml#478
bb97207 Rename Lambda.apply_position
a7cb650 Quieten Makefile when runtime dep files are not present
c656dc9 Merge flambda-backend changes
11b5424 Avoid printing double spaces in function argument lists
7751faa Restore locations to Typedtree.{pat,let}_bound_idents_full
e450b6c add build_ocaml_compiler.sexp
0403bb3 Revert PR 9895 to continue installing VERSION
b3447db Ensure new local attributes are namespaced properly
7f213fc Allow empty functions again
8f22ad8 Bugfix: ensure local domain state is initialised
80f54dd Bugfix for Selectgen with regions
e8133a1 Fix external-external signature inclusion
9840051 Bootstrap
d879f23 Merge remote-tracking branch 'jane/local-reviewed' into local-merge
94454f5 Use Local_store for the local allocations ref
54a164c Create fewer regions, according to typechecking (ocaml#59)
1c2479b Merge flambda-backend changes
ce34678 Fix printing of modes in return types
91f2281 Hook mode variable solving into Btype.snapshot/backtrack
54e4b09 Move Alloc_mode and Value_mode to Btype
ff4611e Merge flambda-backend changes
ce62e45 Ensure allocations are initialised, even dead ones
6b6ec5a Fix the alloc.ml test on 32-bit builds
81e9879 Merge flambda-backend changes
40a7f89 Update repo URL for ocaml-jst, and rename script.
0454ee7 Add some new locally-allocating primitives (ocaml#57)
8acdda1 Reset the local stack pointer in exception handlers (ocaml#56)
8dafa98 Improve typing for (||) and (&&) (ocaml#55)
8c64754 Fix make_check_all_arches (ocaml#54)
b50cd45 Allow arguments to primitives to be local even in tail position (ocaml#53)
cad125d Fix modes from or-patterns (ocaml#50)
4efdb72 Fix tailcalls tests with inlining (ocaml#52)
4a795cb Flambda support (ocaml#49)
74722cb Add [@ocaml.principal] and [@ocaml.noprincipal] attributes, and use in oo.mli
6d7d3b8 Ensure that functions are evaluated after their arguments (flambda-backend ocaml#353)
89bda6b Keep Sys.opaque_identity in Cmm and Mach (port upstream PR 9412)
a39126a Fix tailcalls within regions (ocaml#48)
4ac4cfd Fix stdlib manpages build
3a95f5e Merge flambda-backend changes
efe80c9 Add jane/pull-flambda-patches script
fca94c4 Register allocations for Omitted parameter closures (ocaml#47)
103b139 Remove various FIXMEs (ocaml#46)
62ba2c1 Bootstrap
a0062ad Allow local allocations for various primitives (ocaml#43)
7a2165e Allow primitives to be poly-moded (ocaml#43)
2af3f55 Fix a flaky test by refactoring TypePairs (ocaml#10638)
58dd807 Bootstrap
ee3be10 Fix modes in build_apply for partial applications
fe73656 Tweak for evaluation order of labelled partial applications (ocaml#10653)
0527570 Fix caml_modify on local allocations (ocaml#40)
e657e99 Relax modes for `as` patterns (ocaml#42)
f815bf2 Add special mode handling for tuples in matches and let bindings (ocaml#38)
39f1211 Only take the upper bounds of modes associated with allocations (ocaml#37)
aec6fde Interpret arrow types in "local positions" differently
c4f3319 Bootstrap
ff6fdad Add some missing regions
40d586d Bootstrap
66d8110 Switch to a system with 3 modes for values
f2c5a85 Bugfix for Comballoc with local allocations. (ocaml#41)
83bcd09 Fix bug with root scanning during compaction (ocaml#39)
1b5ec83 Track modes in Lambda.lfunction and onwards (ocaml#33)
f1e2e97 Port ocaml#10728
56703cd Port ocaml#10081
eb66785 Support local allocations in i386 and fix amd64 bug (ocaml#31)
c936b19 Disallow local recursive non-functions (ocaml#30)
c7a193a GC support for local allocations (ocaml#29)
8dd7270 Nonlocal fields (ocaml#28)
e19a2f0 Bootstrap
694b9ac Add syntax to the parser for local allocations (ocaml#26)
f183008 Lower initial stack size
918226f Allow local closure allocations (ocaml#27)
2552e7d Introduce mode variables (ocaml#25)
bc41c99 Minor fixes for local allocations (ocaml#24)
a2a4e60 Runtime and compiler support for more local allocations (ocaml#23)
d030554 Typechecking for local allocations (ocaml#21)
9ee2332 Bugfix missing from ocaml#20
02c4cef Retain block-structured local regions until Mach.
86dbe1c amd64: Move stack realloc calls out-of-line
324d218 More typing modes and locking of environments
a4080b8 Initial version of local allocation (unsafe)

git-subtree-dir: ocaml
git-subtree-split: 173842c
lukemaurer pushed a commit to lukemaurer/ocaml that referenced this pull request Jul 19, 2022
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
0b0aefb Turn some partial application warnings into hints (ocaml#11338) (ocaml#30)
2caa9ee Add [@tail] and [@nontail] annotations on applications to control tailcalls (ocaml#31)
9fb218a Update `promote` target to use the `one` machinery (ocaml#28)
b5ea912 Make empty types immediate
bc08236 Add failing test of an empty type being immediate
f2d439f Propagate escaping_context to Env locks to hint about errors (ocaml#25)
35569e1 Allow warning 68 to be controlled by attributes (ocaml#16)
28a6243 Allow type_argument to weaken return modes of expected function types (ocaml#24)
cdc728f Fix 'make alldepend' in otherlibs/dynlink
7807d18 make alldepend
2d6af2f Merge flambda-backend changes

git-subtree-dir: ocaml
git-subtree-split: 0b0aefb
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Add breadcrumb and header to docs pages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants