-
-
Notifications
You must be signed in to change notification settings - Fork 416
Add runtime config option to change exception trapping behavior #1538
Conversation
Interesting, I thought Perhaps update its documentation to reflect that it is now also settable from a static constructor? |
Updated documentation for the variable |
Updated unit test to account for changed line numbers |
Ping. |
Ping |
It would be nice to get some feedback or at least any reaction to this. |
I have not tried, but I don't think it works to set the flag in a ctor: there is an outer |
That's what I thought too, but it appears to be (no longer?) so. |
e9bf524
to
c7df340
Compare
You were right, it didn't work. I did test it and something did work before but I guess not this.
Or outside GDB:
|
@MartinNowak trying a direct hilight |
Yes, yes, yes, merge this! BTW if it isn't merged, since it is an extern(C) variable, you can also redefine it yourself and instruct the linker to use your version instead of the druntime one. hacky sure but hey.
compile: but this is a trivial change that has clear value, looks very good to me. |
How do we test the command line switch? |
@klickverbot I wrote it above already: you pass |
@Marenz: I realised that. What I was pointed out is that this PR is currently missing regression tests. Due to the nature of the feature, you'd probably need to add a new standalone test executable to |
Do you want to test the switch itself or just whether the runtime behaves correctly when the option is enabled? |
I am asking because the parsing etc of that switch is all functionality that is already present. |
I want something that breaks if the code part of this PR is reverted (well, except for the line numbers^^). |
Will that test need to be cross-platform? |
@@ -477,6 +474,13 @@ extern (C) int _d_run_main(int argc, char **argv, MainFunc mainFunc) | |||
result = (result == EXIT_SUCCESS) ? EXIT_FAILURE : result; | |||
} | |||
|
|||
import rt.config : rt_configOption; | |||
|
|||
if (rt_configOption("noTrapExceptions") !is null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using options without negative: trapExceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gladly, but that means the default will be changed, right? Is that acceptable @MartinNowak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's not necessary (nor desirable, I think). Just make it default to 1, not 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly not desirable :) It would mean that programs will segfault instead of showing an exception stack trace on uncaught exceptions by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have different definitions of "desirable", but sure, I see if I can change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you're using the result of rt_configOption
like a bool here... which means that --DRT-noTrapExceptions=1
and --DRT-noTrapExceptions=0
will both do the same thing. This is not ideal.
I looked to see if we have a precedent for this, and here is how #1798 added callStructDtorsDuringGC
, also a boolean option:
Lines 49 to 58 in 639878d
extern (C) void lifetime_init() | |
{ | |
// this is run before static ctors, so it is safe to modify immutables | |
import rt.config; | |
string s = rt_configOption("callStructDtorsDuringGC"); | |
if (s != null) | |
cast() callStructDtorsDuringGC = s[0] == '1' || s[0] == 'y' || s[0] == 'Y'; | |
else | |
cast() callStructDtorsDuringGC = true; | |
} |
I suggest to parse this option in a similar way, which would also allow e.g. overriding the default behaviour of disabling rt_trapExceptions
when running under a debugger on Windows. That also solves the "double negation" issue. If you're feeling up for it, refactoring the bool checks into a new function in the rt.config
module, which also rejects invalid values, would be a bonus.
why is this still in limbo? |
Looks good to me. @MartinNowak ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superseded by #2035 |
It is superseded even if the title of the new issue says is for the CLI but also via a similar trick in the source code, right? |
AFAICT this stagnated/orphaned PR was about allowing |
This way you can do
to change the runtime behavior before main() is called.