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 12979 - Nothrow violation error is hidden by inline assembler #4033
Conversation
0000000
to
627661c
Compare
This triggers many warnings in druntime about missing returns, which probably happens because of the added blockExit check, so I'll have to revise the pull. |
Adding new grammar will also require documentation pulls. |
cool |
|
||
void foo() | ||
{ | ||
asm @trusted pure nothrow |
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.
For the language consistency, I think we should also support @trusted
, @nogc
and pure
attributes on iasm statement, and we need to change a bare iasm as impure, unsafe, gc-able, and throwable 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.
I think so too.
627661c
to
8f88c05
Compare
There is also a grammar pull dlang/dlang.org#667. |
8f88c05
to
8827e2e
Compare
f7181a8
to
8821a97
Compare
Requires dlang/druntime#974 first. |
How can this be marked as [enhancement] in the PR list? |
|
Thanks, @yebblies, though could someone else please do this as github doesn't like my laptop. |
This enhancement will fill the semantic gap between inline assembler and function attributes. |
Looks good. |
@@ -747,6 +747,20 @@ class AsmStatement : public Statement | |||
void accept(Visitor *v) { v->visit(this); } | |||
}; | |||
|
|||
// a complete asm {} block | |||
class AsmCompoundStatement : public CompoundStatement |
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.
In dmd code CompoundDeclarationStatement
already exists, so the name CompoundAsmStatement
might be better?
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 was actually thinking about AsmStatement
and renaming the old AsmStatement
to AsmInstruction
. That would be far less confusing.
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.
This is because AsmStatement::semantic
splits them up into one-per-instruction blocks right? Can't we just defer that split to the glue layer somehow?
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.
In that case, I should probably rename GDC's as ExtAsmTemplate. ;)
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.
This is because AsmStatement::semantic splits them up into one-per-instruction blocks right?
The parser already does that. I think the main reason for this are LabelStatements which can be accessed from outside asm blocks.
Can't we just defer that split to the glue layer somehow?
Maybe, but this isn't part of this pull request.
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.
Maybe, but this isn't part of this pull request.
Fair enough.
e9cdb3e
to
faa3bfc
Compare
@MartinNowak Druntime PR was merged, so this PR should be ready to merge. If others have no complaint about this enhancement, I'll merge this in this weekend (UTC 10/5 0:00). |
done |
if (mustNotThrow) | ||
s->error("asm statements are assumed to throw", s->toChars()); | ||
if (mustNotThrow && !(s->stc & STCnothrow)) | ||
s->deprecation("asm block is not nothrow"); |
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.
asm statement
, not asm block
, as we should consistently refer to things with the same phrase. I also don't like the double negative, and would prefer something along the lines of:
asm statement is assumed to throw - mark it with 'nothrow' attribute if it does not
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.
asm statement, not asm block, as we should consistently refer to things with the same phrase.
I mixed up the wording because AsmStatement
in the compiler is actually a single asm instruction.
@WalterBright You would have no objection about the enhancement feature itself, right?. @MartinNowak I'll defer the merging until your reaction comes back. |
Updated the wording of the error messages. |
@redstar, @AlexeyProkhin: When this is in, we can probably get rid of our |
LGTM. |
Auto-merge toggled on |
fix Issue 12979 - Nothrow violation error is hidden by inline assembler
From a practical reason, I'd propose an additional enhancement https://issues.dlang.org/show_bug.cgi?id=13589 |
deprecate using unattributed asm in attributed functions
add explicit attributes on asm statments (unverified by compiler)
asm pure nothrow @nogc @trusted {
//...
}
added new CompoundAsmStatement because the parser
already splits asm blocks into many AsmStatements