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

Fix Issue 19635 - -checkaction=context not working with attributes #2479

Merged
merged 2 commits into from Mar 12, 2019

Conversation

@wilzbach
Copy link
Member

commented Jan 31, 2019

The program will be terminated after the assertion error message has been printed and its not considered part of the "main" program. Also, catching an AssertError is Undefined Behavior.

Hence, we can fake purity and @nogc-ness here.

An alternative would be to directly ignore the assert message expression directly in DMD's type checker (see e.g. https://issues.dlang.org/show_bug.cgi?id=15100 for this).

@dlang-bot

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Severity Description
19635 major -checkaction=context not working with attributes

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + druntime#2479"
@dlang-bot dlang-bot added the Bug Fix label Jan 31, 2019
@wilzbach wilzbach force-pushed the wilzbach:fix-19635 branch 2 times, most recently from fd5d690 to 5f17c85 Jan 31, 2019
@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

The program will be terminated after the assertion error message has been printed and its not considered part of the "main" program. Also, catching an AssertError is Undefined Behavior.

I'm not sure about catching AssertError, but if an assert fails inside a unittest block, the program is still considered in a valid state:

"Unlike AssertExpressions used elsewhere, the assert is not assumed to hold, and upon assert failure the program is still in a defined state." [1].

[1] https://dlang.org/spec/unittest.html

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

Hmm but making this work nicely with attributes leads to a lot of problems:

  • can we use assume that sprintf is pure (or maybe store at floating point register and reset them afterwards)?
  • allocate with sth. smarter than pureMalloc, s.t. it's still @nogc (though technically never freeing memory could be considered a "valid state" 🤔)
  • custom, user-defined toString methods will always be problematic (?)

I'm not sure about catching AssertError

https://dlang.org/library/object/exception.html

In principle, only thrown objects derived from this class [Exception] are safe to catch inside a catch block. Thrown objects not derived from Exception represent runtime errors that should not be caught, as certain runtime guarantees may not hold, making it unsafe to continue program execution.

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

https://dlang.org/library/object/exception.html

In principle, only thrown objects derived from this class [Exception] are safe to catch inside a catch >> block. Thrown objects not derived from Exception represent runtime errors that should not be >> >> caught, as certain runtime guarantees may not hold, making it unsafe to continue program execution.

I know, at the same time the spec says this [1]:

"Implementation Defined:
5. Whether the program stops on the first unit test failure, or continues running the unit tests."

Currently this is only possible to implemented by catching AssertError. Also druntime is catching all Throwable, so there are currently some contradictions:

druntime/src/rt/dmain2.d

Lines 480 to 484 in ca1be3f

catch (Throwable t)
{
_d_print_throwable(t);
result = EXIT_FAILURE;
}

[1] https://dlang.org/spec/unittest.html

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Does invertCompToken need to have attributes added?

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

can we use assume that sprintf is pure (or maybe store at floating point register and reset them afterwards)?

I'm no expert at this but it sounds like it could be pure.

allocate with sth. smarter than pureMalloc, s.t. it's still @nogc (though technically never freeing memory could be considered a "valid state" 🤔)

Alternatives would be a stack allocated buffer, either using alloca or a static buffer. If the result doesn't fit in the buffer it could fallback to the behavior when -checkaction=context is not used.

custom, user-defined toString methods will always be problematic (?)

Yeah, that sounds a bit problematic.

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

Alternatives would be a stack allocated buffer, either using alloca or a static buffer.

Stack allocated buffer won't work here because _d_assert_fail needs to return a string.
A static bufffer is problematic because if catching AssertError is valid then we would overwrite that user-code message.

So imho this trick is the best we got for now until rcstring becomes a thing (and even then we might not be able to use it because a string is expected for Error.message here :/

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

Does invertCompToken need to have attributes added?

No, it's called at compile-time (enum token = invertCompToken(comp);) (and there even was a bit of discussion on the NG about not inverting the token, so we might end up removing it entirely).

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

No, it's called at compile-time (enum token = invertCompToken(comp);)

Hmm, interesting. I didn't know that would bypass attributes.

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Stack allocated buffer won't work here because _d_assert_fail needs to return a string.

Not sure if it helps, but if alloca is used as the default value of parameter it's allocated in the caller's stack, i.e.:

void* foo(void* buffer = alloca(1))
{
    return buffer;
}
@anon17

This comment has been minimized.

Copy link

commented Feb 1, 2019

void f() @nogc
{
    assert(false,"");
}

This compiles and throws a GC allocated exception. I believe the general sentiment is that assert failure can bypass some language constraints, notably nothrow.

Copy link
Contributor

left a comment

Some nit picking

src/core/internal/dassert.d Outdated Show resolved Hide resolved
src/core/internal/dassert.d Outdated Show resolved Hide resolved
@edi33416

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Stack allocated buffer won't work here because _d_assert_fail needs to return a string.

Not sure if it helps, but if alloca is used as the default value of parameter it's allocated in the caller's stack, i.e.:

void* foo(void* buffer = alloca(1))
{
    return buffer;
}

This might be helpful, but the caller needs to be aware of this, or he'll get bitten

void *bar()
{
    return foo(); // oops.. returns ptr to local alloca buffer
}
@edi33416

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Alternatives would be a stack allocated buffer, either using alloca or a static buffer.

Stack allocated buffer won't work here because _d_assert_fail needs to return a string.
A static bufffer is problematic because if catching AssertError is valid then we would overwrite that user-code message.

The current implementation of Errors in core.exception use a static buffer. IIRC, this was accepted as ok because an error will be thrown when your program is in an unrecoverable state.

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

The current implementation of Errors in core.exception use a static buffer. IIRC, this was accepted as ok because an error will be thrown when your program is in an unrecoverable state.

As I mentioned previously:

"Unlike AssertExpressions used elsewhere, the assert is not assumed to hold, and upon assert failure the program is still in a defined state." [1].

[1] https://dlang.org/spec/unittest.html

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

"Unlike AssertExpressions used elsewhere, the assert is not assumed to hold, and upon assert failure the program is still in a defined state." [1].

Yeah it looks like we need some input from @andralex or @WalterBright on how to proceed here.

I wouldn't mind using a static buffer though it wouldn't fix e.g. user-generated toString:

struct MyStruct {
    string toString() { return "hello"; }
}
void main() @nogc nothrow {
    auto s = MyStruct();
    assert(s != s); // <- FAILS here
}

https://run.dlang.io/is/VGDpwn

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

it wouldn't fix e.g. user-generated toString:

What about the sink version of toString. If we only support that version would that help?

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

What about the sink version of toString. If we only support that version would that help?

Yeah that would help, though who uses a sink version of toString these days?
And as soon as the author does sth. with strings it wouldn't e.g. be nothrow [1] anymore

import std.stdio, std.range;
struct MyStruct {
    enum mystring = "test";
    auto toString(W)(W w) { return w.put(mystring.front); }
}
void main() @nogc nothrow {
    struct Sink {
        auto put(D)(D d) {}   
    }
    MyStruct().toString(Sink());
}

https://run.dlang.io/is/xQeV33

[1] And, of course, as exceptions still require the GC also not @nogc.

Thus, I'm more leaning forward to this "fake it" approach as long as we can guarantee that it plays nicely with unittest. So maybe it's better to allocate the memory with the GC (and just mark it as @nogc) as this would at least prevent memory leaks?

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

So maybe it's better to allocate the memory with the GC (and just mark it as @nogc) as this would at least prevent memory leaks?

What happens then when someone using betterC and doesn't link druntime?

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

What happens then when someone using betterC and doesn't link druntime?

Then they wouldn't be able to use D's exceptions + errors (e.g. AssertError) and even assert automatically gets rewritten to C's assert.

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

assert automatically gets rewritten to C's assert

Yes, but what happens then with the assert message?

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

-checkaction=context and -checkaction=c conflict. Also C's assert doesn't support a message.

@anon17

This comment has been minimized.

Copy link

commented Feb 12, 2019

I use C __assert function to print message from D assert :)

@wilzbach wilzbach force-pushed the wilzbach:fix-19635 branch from 5f17c85 to ac4e5b0 Feb 28, 2019
@nordlow

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Keep up the good work, @wilzbach and I'll disturb @andralex if this gets stalled ;)

@wilzbach wilzbach force-pushed the wilzbach:fix-19635 branch from 219328e to 5b14d28 Mar 5, 2019
@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

Keep up the good work, @wilzbach and I'll disturb @andralex if this gets stalled ;)

Hehe as far as I can tell, this already got stalled. At least, I don't see anything else I can do to help this PR.

@andralex

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Isn't this more of a @WalterBright thing?

@nordlow

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

You are right.

Copy link
Contributor

left a comment

Otherwise looks good.

@wilzbach wilzbach force-pushed the wilzbach:fix-19635 branch 2 times, most recently from 1de725f to 38781d5 Mar 7, 2019
@wilzbach wilzbach requested a review from Burgos as a code owner Mar 7, 2019
@wilzbach wilzbach changed the base branch from master to stable Mar 7, 2019
@wilzbach wilzbach force-pushed the wilzbach:fix-19635 branch from 38781d5 to afe9293 Mar 7, 2019
@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

Rebased to stable as without this PR -checkaction=assert is barely usable. Also, added the 72h objection label as no one really has provided a better way and so far it looks like this is the only option we have for now.

been printed and its not considered part of the "main" program.
Also, catching an AssertError is Undefined Behavior
Hence, we can fake purity and @nogc-ness here.
*/

This comment has been minimized.

Copy link
@jacob-carlborg

jacob-carlborg Mar 7, 2019

Contributor

I disagree with this comment. There are several places in the D code bases where AssertError is caught.

@wilzbach wilzbach force-pushed the wilzbach:fix-19635 branch from afe9293 to 6c99566 Mar 7, 2019
@veelo

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Rebased to stable as without this PR -checkaction=assert is barely usable.

At the risk of going OT: apropos barely usable, are you aware of https://issues.dlang.org/show_bug.cgi?id=19711?

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

Rebased to stable as without this PR -checkaction=assert is barely usable.

At the risk of going OT: apropos barely usable, are you aware of https://issues.dlang.org/show_bug.cgi?id=19711?

Now I'm, but the issue is very unrelated.

This is just a run.dlang.io issue as it just matches for -c.
See e.g. https://run.dlang.io/is/8eUzXP (by adding -run yourself you workaround the run.dlang.io issue)).

Anyhow, the fix is here -> dlang-tour/core-exec#44

@anon17

This comment has been minimized.

Copy link

commented Mar 7, 2019

Yeah, it's that thing, "don't catch Errors" is a rule for business logic, which one might know how to break, druntime code that catches Throwable has code tailored to print a message without much relying on the rest of application code.

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

druntime code that catches Throwable has code tailored to print a message without much relying on the rest of application code.

The default unit test runner in druntime will catch all Throwable and continue executing remaining tests. One of the test runners in DMD will do the same.

@anon17

This comment has been minimized.

Copy link

commented Mar 11, 2019

It won't execute the application after the tests, will it? If a failing test corrupted memory, then following tests can experience heisenbugs.

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

I'm not sure where the discussion is going about. The runtime already uses the GC to allocate other parts of the error handling and formatting. This just makes the new context-aware switch work similar.
And so far no one has come up with a better solution.

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

I'm not sure where the discussion is going about. The runtime already uses the GC to allocate other parts of the error handling and formatting. This just makes the new context-aware switch work similar.
And so far no one has come up with a better solution.

I disagree with the following reasoning, which you have in a comment:

The program will be terminated after the assertion error message has been printed and its not considered part of the "main" program. Also, catching an AssertError is Undefined Behavior. Hence, we can fake purity and @nogc-ness here.

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

It won't execute the application after the tests, will it?

No.

If a failing test corrupted memory, then following tests can experience heisenbugs.

Yes, that might happen. But I assume you would rerun all tests once the failing test is fixed.

@thewilsonator thewilsonator merged commit 052fda0 into dlang:stable Mar 12, 2019
6 checks passed
6 checks passed
CyberShadow/DAutoTest Documentation OK (no changes)
Details
auto-tester Pass: 10
Details
buildkite/druntime Build #1341 passed (18 minutes, 12 seconds)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 47.826% of diff hit (target 73.666%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.