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

Meaningfull compiler backtraces #6574

Closed
vicuna opened this issue Sep 23, 2014 · 6 comments
Closed

Meaningfull compiler backtraces #6574

vicuna opened this issue Sep 23, 2014 · 6 comments
Labels
Milestone

Comments

@vicuna
Copy link

vicuna commented Sep 23, 2014

Original bug ID: 6574
Reporter: @chambart
Status: closed (set by @damiendoligez on 2016-01-12T15:53:44Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.03.0+dev / +beta1
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: ~DO NOT USE (was: OCaml general)

Bug description

The main functions of the compiler have a catch-all try-with that cut the exception backtraces:

  • In Compile (Optcompile), the remove_file function cut the stack trace.
    patch_misc.diff, avoid raising in remove_file in the common case.
  • In Main (Optmain), the report_error function cut the stack trace.
    It can either be solved by hand-inlining it: patch_main.diff
    or by using the reraise primitive in report_error: patch_location.diff

File attachments

@vicuna
Copy link
Author

vicuna commented Sep 23, 2014

Comment author: @damiendoligez

patch_location: ok

patch_misc:
Note that this is not a neutral change: if the file is a symlink to a nonexistent file, file_exists will return false and you don't remove it, while a straight remove would have removed it. This is relevant, for example at lines 293 and 414 of bytelink.ml and line 67 of ccomp.ml. We might decide that we don't care for users who mess up with symlinks, but it's still a change of behaviour.

@vicuna
Copy link
Author

vicuna commented Sep 24, 2014

Comment author: @chambart

If we don't want to change the behavior we could either reraise also in remove_file (leading to quite surprising, but complete stack traces), or check the presence of the file with Sys.readdir.

@vicuna
Copy link
Author

vicuna commented Sep 24, 2014

Comment author: @alainfrisch

We should probably expose reraise in the stdlib.

@vicuna
Copy link
Author

vicuna commented Dec 20, 2015

Comment author: @gasche

#358 is a new attempt at solving the same problem.

@vicuna
Copy link
Author

vicuna commented Dec 31, 2015

Comment author: @gasche

Looking at patch_misc again, I am not sure that doligez's remark about symlinks applies: this is Misc.remove_file, a function that is used only in the compiler codebase, not by external users. Looking at its call sites, I see it used only to remove a file that is, on non-error paths, created by the compiler itself, so we know that:

  • if it was successfully created by the compiler, it is not a weird symlink and the patch is correct
  • if the compiler failed to create it, it is a user file, and not removing it is not wrong

So I would argue that the patch is correct (the behavior is changed, but it is more correct than it was before). It is also simpler than adding a separate primitive for file removal without exceptions, as proposed in
#358

@vicuna
Copy link
Author

vicuna commented Jan 12, 2016

Comment author: @damiendoligez

Merged in trunk (commit 355c982), choosing the "reraise" version of the patch.

@vicuna vicuna closed this as completed Jan 12, 2016
@vicuna vicuna added this to the 4.03.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant