Skip to content
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

Enable -g and OCAMLRUNPARAM=b by default #6728

Open
vicuna opened this issue Dec 19, 2014 · 28 comments
Open

Enable -g and OCAMLRUNPARAM=b by default #6728

vicuna opened this issue Dec 19, 2014 · 28 comments

Comments

@vicuna
Copy link

vicuna commented Dec 19, 2014

Original bug ID: 6728
Reporter: @whitequark
Status: acknowledged (set by @damiendoligez on 2015-01-09T17:05:06Z)
Resolution: open
Priority: normal
Severity: feature
Category: compiler driver
Related to: #5855 #6238
Monitored by: @ygrek @hcarty @dbuenzli @yakobowski

Bug description

The size hit of -g is insignificant; a raw opam init checkout (not even any compilers or packages installed) is 60M, and a dozen packages blows it up to 500M. I haven't seen anyone complain.

The fact that -g enables several things unrelated to backtraces (see #6238) should be still addressed.

The rationale for enabling OCAMLRUNPARAM=b is well described in #5855. Additionally:

Not only most OCaml programs are unaffected by the slight performance hit due to collection of backtraces, but also since after 4.02, raise_notrace is available. I think it's time to stop caring about the performance hit at all. The code which require fast exceptions can (and should) well have them with raise_notrace.

@vicuna
Copy link
Author

vicuna commented Jan 9, 2015

Comment author: @damiendoligez

While I agree that no one should care much about the size increase, we have many users who do care a lot about performance.

In any case, if we enable backtraces by default, we'll have to change the parsing of OCAMLRUNPARAM to provide a syntax for disabling backtraces.

@vicuna
Copy link
Author

vicuna commented Jan 9, 2015

Comment author: @whitequark

I do agree, hence the note on raise_notrace. And if the backtraces are not dynamically necessary, the debug information is not read at all, so just always enabling -g would be a zero-cost change.

OCAMLRUNPARAM=b=0?

@vicuna
Copy link
Author

vicuna commented Jan 9, 2015

Comment author: @lpw25

There would definitely need to be a -no-g option (with a better name than that), manually setting OCAMLRUNPARAM is not a viable alternative.

@vicuna
Copy link
Author

vicuna commented Mar 1, 2017

Comment author: @damiendoligez

Note: the OCAMLRUNPARAM=b=0 part is already implemented as part of #6808.

@dbuenzli
Copy link
Contributor

I know changing defaults is contentious but I often see people having a lack of stacktraces when they use the toplevel which is a bit sad.

I have an export OCAMLRUNPARAM='b' in my .profile that I must have added when I started programming in OCaml but I think one should not be required to do so.

If people still worry too much making OCAMLRUNPARAM='b' the default maybe we could have a call to record_backtrace in theocaml initialisation procedure ?

@dbuenzli
Copy link
Contributor

If people still worry too much making OCAMLRUNPARAM='b' the default maybe we could have a call to record_backtrace in theocaml initialisation procedure ?

Sorry to insist with that. But since I released down I get crash reports from people who as far as I know have been programming in OCaml from quite some time. It's quite consistent: no stack trace which I find really silly. I'll gladly make a PR that does the above if I get an indication it will be merged.

@alainfrisch
Copy link
Contributor

If people still worry too much making OCAMLRUNPARAM='b' the default maybe we could have a call to record_backtrace in theocaml initialisation procedure ?

If you force a call to record_backtrace true, this would prevent disabling through OCAMLRUNPARAM=b=0, no?

FWIW, I personally agree that users should get backtraces "by default":

  1. Letting users choose at link-time whether backtraces should be recorded (keeping explicit overrides through OCAMLRUNPARAM and record_backtrace).
  2. Enable this by default, with a command-line flag to disable it.
  3. Also defaulting -g to true, with a new option to disable it.

@dbuenzli
Copy link
Contributor

If you force a call to record_backtrace true, this would prevent disabling through OCAMLRUNPARAM=b=0, no?

It can of course be made conditional on consulting the variable. But if people are willing to change the global default I'm all for it.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Aug 28, 2020
@whitequark
Copy link
Member

Still an issue.

@avsm avsm removed the Stale label Aug 28, 2020
@shindere
Copy link
Contributor

Willing to work on that one.

Am I correct that we are actually discussing two slightly independent
issues, here? Namely, keeping cabktraces and keeping debugging symbols.
Although, I guess, debugging symbols are need for backtraces, right?

If -g becomes the default, how about -strip to revert it?

@shindere shindere self-assigned this Aug 31, 2020
@gasche
Copy link
Member

gasche commented Aug 31, 2020

Note: -g does several things at once that are helpful for debugging (for example I believe that it disables some code transformations in the compiler). It would be nice to someday provide a finer-grained control with sub-options, but it still remains useful to have an umbrella option, and it is sensible that the option would be set by default.

I think that we could have -g and -no-g, or maybe add --debug as an alias to -g, and --no-debug. (The name is vague about what it does, but one could in the future have sub-options --(no-)debug-foo.

@dbuenzli
Copy link
Contributor

dbuenzli commented Aug 31, 2020

Note that I think most build system out there do add a -g by default (or make it easy to do so) now.

The actual problem is rather the runtime system default – i.e. that you need to specify OCAMLRUNPARAM=b to have them.

@gasche
Copy link
Member

gasche commented Aug 31, 2020

Yes, so we would accept b={0,1}, with =b an alias for =b=1, and set b=1 to be the new default.

@shindere
Copy link
Contributor

shindere commented Aug 31, 2020 via email

@dbuenzli
Copy link
Contributor

By "them" you mean the backtraces, right?

Yes.

@github-actions
Copy link

github-actions bot commented Sep 3, 2021

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Sep 3, 2021
@shindere
Copy link
Contributor

shindere commented Sep 6, 2021 via email

@dbuenzli
Copy link
Contributor

Rather than discussing this, wouldn't it be better to provide a generic mechanism that would let users specify the options they want as defaults, e.g. at compiler configure time or, if that's not flexible enough, at compiler / program run time?

No. Good defaults is what we need, not more configurability.

@lthls
Copy link
Contributor

lthls commented May 27, 2022

@dbuenzli I think that somewhere above you mentioned the toplevel specifically. Would you be in favour of a move to systematically record backtraces in the toplevel (and keep discussing the default for compiled programs) ?
Apparently backtraces in the toplevel leak a bit of the implementation details, but apart from that I think a small performance hit in the toplevel should be uncontroversial.

@dbuenzli
Copy link
Contributor

Why not, but if people obsessed by speed like JaneStreet set it by default (according to this comment on discuss). Can't we safely bet the performance hit is too minimal to consider ?

@nojb
Copy link
Contributor

nojb commented May 27, 2022

My sense from the discussion is that there seems to be a consensus in favour of changing the default everywhere. As far as I can see the only thing that needs to be added beyond that is the -no-g flag. Other improvements can be left for later.

Furthermore, the 5.0 release seems like a good point at which to make this change.

@dbuenzli
Copy link
Contributor

As far as I can see the only thing that needs to be added beyond that is the -no-g flag.

That's not even really necessary, the only default that really needs to be changed is OCAMLRUNPARAM's one, see this comment,

@nojb
Copy link
Contributor

nojb commented May 27, 2022

See #11282

@shindere
Copy link
Contributor

shindere commented May 30, 2022 via email

@shindere
Copy link
Contributor

shindere commented May 30, 2022 via email

@github-actions
Copy link

github-actions bot commented Jun 2, 2023

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Jun 2, 2023
@whitequark
Copy link
Member

whitequark commented Jun 2, 2023

Still relevant. And please disable the bot already.

@github-actions github-actions bot removed the Stale label Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants