Add runtime config option to change exception trapping behavior - #1538
Add runtime config option to change exception trapping behavior#1538Marenz wants to merge 1 commit into
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? |
|
|
||
| import rt.config : rt_configOption; | ||
|
|
||
| if (rt_configOption("noTrapExceptions") !is null) |
There was a problem hiding this comment.
I suggest using options without negative: trapExceptions
There was a problem hiding this comment.
Gladly, but that means the default will be changed, right? Is that acceptable @MartinNowak
There was a problem hiding this comment.
No, that's not necessary (nor desirable, I think). Just make it default to 1, not 0.
There was a problem hiding this comment.
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.
We have different definitions of "desirable", but sure, I see if I can change it
There was a problem hiding this comment.
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
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 ? |
|
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.