Replace Arg.Bad with fatal in compenv.ml by bschommer · Pull Request #1011 · ocaml/ocaml

bschommer · 2017-01-13T19:23:42Z

Since there is no exception handler for Arg.Bad the exception is uncaught.
Now the compiler aborts with fatal and prints out the error message.

This should fix MPR#7460.

Since there is no exception handler for Arg.Bad the exception is uncaught. Now the compiler aborts with fatal and prints out the error message.

bschommer · 2017-01-13T19:25:32Z

I'm not sure if a Change entry is need and in my opinion the error message can be improved a bit but I couldn't come up with a better one at the moment.

gasche · 2017-01-13T19:32:09Z

I think that, if someone reported a bug for it, it deserves a Changes entry.

Could you maybe add a regression test among the tool-* tests, possibly in a new directory tool-command-line?

bschommer · 2017-01-13T19:37:25Z

No problem.

Octachron · 2017-01-13T20:32:06Z

Since this is an argument parsing error, I would argue that it makes sense to restore 4.03 behavior and fail with a standard Arg error message (with the addition of the usage message).

bschommer · 2017-01-13T20:45:53Z

I personally find the old error message not that usefull, however I'm not in strong favor of my new message and if it is wanted I can revert to the old message.

Printing the help and usage message would require either changing the interface of compenv.ml or a lot of code duplication in each of the tools that use the Arg module and compilenv.ml.

gasche · 2017-01-13T20:54:24Z

I think that the wording of the new error message is not very good. The older one is maybe a bit better -- it's not terribly useful, as you say, but it reads better as a sentence.

bschommer · 2017-01-14T09:46:03Z

I had a look and the function that raises the Exception Arg.Bad is only called in the bytecode and the native compiler. I will rename the warning and restore the behavior of 4.03.

The function raising Arg.Bad is only used in driver/mail.ml and driver/optmain.ml and so adding the two exception handlers restored the 4.03 behavior.

gasche · 2017-01-14T11:08:07Z

I don't understand the last change. I noticed with the CI that there was an issue with the patch, but what is the issue? Why is 7cf0cfa useful? I'm not so happy with the fact that you are changing the Compenv interface. I suppose you are trying to avoid some module dependencies, it's not Compenv->Arg (this dependency was already present), maybe it is Compenv->Options? Why is this dependency bad?

bschommer · 2017-01-14T11:10:47Z

I actually have no idea, since every function I used in main.ml and optmain.ml was used before or came from a the Arg module to which there was already a dependency.

gasche · 2017-01-14T11:15:37Z

Could we find a solution, then, that does not change the Compilenv interface? One may need to update the .depend file to reflect a new dependency from Compilenv on Options. If that does not work, we might drop the idea of showing the usage text in this case. But I would like to understand what the linking issue was (why would Dynlinkaux be affected?) in case it would reflect a bug in this patch (sounds unlikely) or another recent change.

Octachron · 2017-01-14T11:33:12Z

Since Compenv.process_deferred_actions is only called from main.ml and optmain.ml, I think that an alternative to the current patch would be to catch the Arg.Bad exception in these two modules without modifying Compenv, except for documenting the fact that Compenv.process_deferred_actions can raise an Arg.Bad exception.

bschommer · 2017-01-14T11:34:19Z

I actually tried this in commit eee2829 but I got some weird linking errors.

This reverts commit 7cf0cfa.

The problem was the missing begin...end block plus the missing exit 2.

bschommer · 2017-01-14T12:16:16Z

I think I found the source of my link problems (beside the missing begin ... end block) was the missing exit 2. However I'm still curious why this generated such strange link errors. The test need to be adopted a bit in order to reflect the different option set of flambda and non-flambda ocamlopt.

Removed whitespace

bschommer · 2017-01-14T14:03:09Z

It should work now as 4.03 and before. The test filters out all options beside the message for the unknown file, this should avoid breaking the test whenever a new option is introduced.

gasche · 2017-01-14T14:47:03Z

You could maybe test only ocamlc?

gasche · 2017-01-14T15:14:37Z

Changes


+- PR#7460, GPR#1011: catch exception thrown when unknown files are passed
+  as argument.
+  (Bernhard Schommer)


Maybe point out that @Octachron was helpful in reviewing? (Or maybe you had found the nasty syntax thing independently?) Otherwise it now seems good to merge.

No problem, will amend the change entry.

gasche · 2017-01-14T21:50:41Z

Thanks to all, I merged in trunk and 4.04. (I was a bit hesitant to merge in 4.04 as this is admittedly a very minor regression, but the final patch is also very easy to check for non-intrusiveness so I went for it.)

gasche · 2017-01-19T07:54:02Z

In a48b2cb I updated the new test to work also on bytecode-only configurations. (I also forget about those when I write tests.)

Adds builtins for the equivalent of GCC's __sync_fetch_and_add/sub and __sync_bool_compare_and_swap. The pointer type may be native_pointer, ext_pointer, or bigstring with offset. The integer type may be untagged int, unboxed int64, unboxed int32, or unboxed nativeint. The builtins are not translated on ARM (but it would now be easy to add ARM backend support - armv8 provides single instructions for each of these operations). The unboxed int64 builtins are not translated on 32 bit architectures.

* Create init-cache script * Reduce startup time - Sync opam repo during build - git pull && opam update - Copy from build in run instead of git clone - Precompute package state cache - Launch server once, at the end of build - Build is complete when package.state is closed after write - Copy package.state from build in run - Misc required details - Make /var/opam-repository "safe", whatever it means - Use a shell script to wait until package.state is complete - Add inotify-tools as build dependency - Unrelated - Make docker image name the same in Makefile & HACKING.md * Add branch to git pull when polling opam repo * Formatting * Cleanup --------- Co-authored-by: Cuihtlauac ALVARADO <[email protected]>

Replace Arg.Bad with fatal in compenv.ml

f62553c

Since there is no exception handler for Arg.Bad the exception is uncaught. Now the compiler aborts with fatal and prints out the error message.

bschommer mentioned this pull request Jan 13, 2017

Fix Stdlib.Arg: do no repeat usage_msg thrice #999

Merged

bschommer added 3 commits January 13, 2017 20:40

Added Chang entry.

bea6cdb

Improved error message.

b314eba

Added test and reference.

8bb8fbb

gasche added the approved label Jan 13, 2017

Update reference.

69ea3a0

bschommer added 2 commits January 14, 2017 11:15

Changed implementation to 4.03 behavior.

eee2829

The function raising Arg.Bad is only used in driver/mail.ml and driver/optmain.ml and so adding the two exception handlers restored the 4.03 behavior.

Fix linking issue.

7cf0cfa

gasche removed the approved label Jan 14, 2017

bschommer added 2 commits January 14, 2017 12:57

Revert "Fix linking issue."

9d736a4

This reverts commit 7cf0cfa.

Revereted changes to interface.

8252aad

The problem was the missing begin...end block plus the missing exit 2.

bschommer and others added 2 commits January 14, 2017 14:29

Fixed test and removed whitespace commit.

6687dcb

Upated compend.ml

4045ef6

Removed whitespace

gasche reviewed Jan 14, 2017

View reviewed changes

Amend Change entry with people involved.

9578789

gasche closed this Jan 14, 2017

bschommer deleted the unknown-file branch January 16, 2017 07:45

vicuna mentioned this pull request Mar 14, 2019

Uncaught Arg.Bad with some commandlines #7460

Closed

Conversation

bschommer commented Jan 13, 2017

Uh oh!

bschommer commented Jan 13, 2017

Uh oh!

gasche commented Jan 13, 2017

Uh oh!

bschommer commented Jan 13, 2017

Uh oh!

Octachron commented Jan 13, 2017

Uh oh!

bschommer commented Jan 13, 2017

Uh oh!

gasche commented Jan 13, 2017

Uh oh!

bschommer commented Jan 14, 2017

Uh oh!

gasche commented Jan 14, 2017

Uh oh!

bschommer commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasche commented Jan 14, 2017

Uh oh!

Octachron commented Jan 14, 2017

Uh oh!

bschommer commented Jan 14, 2017

Uh oh!

bschommer commented Jan 14, 2017

Uh oh!

bschommer commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasche commented Jan 14, 2017

Uh oh!

gasche Jan 14, 2017

Choose a reason for hiding this comment

Uh oh!

bschommer Jan 14, 2017

Choose a reason for hiding this comment

Uh oh!

gasche commented Jan 14, 2017

Uh oh!

gasche commented Jan 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bschommer commented Jan 14, 2017 •

edited

Loading

bschommer commented Jan 14, 2017 •

edited

Loading