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

codegen_llvm: verify that inline assembly operands are scalars #54747

Merged
merged 1 commit into from Oct 11, 2018

Conversation

levex
Copy link
Contributor

@levex levex commented Oct 2, 2018

Another set of inline assembly fixes. This time let's emit an error message when the operand value cannot be coerced into the operand constraint.

Two questions:

  1. Should I reuse E0668 which was introduced in codegen_llvm: check inline assembly constraints with LLVM #54568 or just use E0669 as it stands because they do mean different things, but maybe that's not too user-friendly. Just a thought.
  2. The try_fold returns the operand which failed to be converted into a scalar value, any suggestions on how to use that in the error message?

Thanks!

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2018
@cramertj
Copy link
Member

cramertj commented Oct 2, 2018

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned cramertj Oct 2, 2018
span_err!(bx.sess(), statement.source_info.span, E0668,
"malformed inline assembly");
if input_vals.is_err() {
span_err!(bx.sess(), statement.source_info.span, E0669,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could use the span(s) of the operands to make it clearer to the user where the error lies.

@nagisa
Copy link
Member

nagisa commented Oct 3, 2018

Should I reuse E0668 which was introduced in #54568 or just use E0669 as it stands because they do mean different things, but maybe that's not too user-friendly. Just a thought.

It is fine to use different codes.

The try_fold returns the operand which failed to be converted into a scalar value, any suggestions on how to use that in the error message?

Would definitely be nice to point out the exact operand with a span as @parched mentioned, but you might have to carry through some extra information for it to work. I’m fine with making that a task for the future and landing this first.

@levex
Copy link
Contributor Author

levex commented Oct 4, 2018

I'll have a look at getting the span of the operand into the error message, but from what I can tell it's not trivial. I'd say we land this and then I'll add improving the error message to my TODO list :)

@nagisa
Copy link
Member

nagisa commented Oct 4, 2018

Please squash the commits as well.

@nagisa nagisa added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2018
Otherwise, LLVM translation will fail with a panic.

Signed-off-by: Levente Kurusa <lkurusa@acm.org>
@levex
Copy link
Contributor Author

levex commented Oct 6, 2018

Please squash the commits as well.

Done!

@nagisa
Copy link
Member

nagisa commented Oct 10, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Oct 10, 2018

📌 Commit 3d7476e has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2018
@bors
Copy link
Contributor

bors commented Oct 10, 2018

⌛ Testing commit 3d7476e with merge f42c510...

bors added a commit that referenced this pull request Oct 10, 2018
codegen_llvm: verify that inline assembly operands are scalars

Another set of inline assembly fixes. This time let's emit an error message when the operand value cannot be coerced into the operand constraint.

Two questions:

1) Should I reuse `E0668` which was introduced in #54568 or just use `E0669` as it stands because they do mean different things, but maybe that's not too user-friendly. Just a thought.
2) The `try_fold` returns the operand which failed to be converted into a scalar value, any suggestions on how to use that in the error message?

Thanks!
@bors
Copy link
Contributor

bors commented Oct 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing f42c510 to master...

bors added a commit that referenced this pull request Oct 11, 2018
Rollup of 9 pull requests

Successful merges:

 - #54747 (codegen_llvm: verify that inline assembly operands are scalars)
 - #54848 (Better Diagnostic for Trait Object Capture)
 - #54850 (Fix #54707 - parse_trait_item_ now handles interpolated blocks as function body decls)
 - #54858 (second round of refactorings for universes)
 - #54862 (Implement RFC 2539: cfg_attr with multiple attributes)
 - #54869 (Fix mobile docs)
 - #54870 (Stabilize tool lints)
 - #54893 (Fix internal compiler error on malformed match arm pattern.)
 - #54904 (Stabilize the `Option::replace` method)

Failed merges:

 - #54909 ( Add chalk rules related to associated type defs)

r? @ghost
@bors bors merged commit 3d7476e into rust-lang:master Oct 11, 2018
bors added a commit that referenced this pull request Oct 11, 2018
Rollup of 9 pull requests

Successful merges:

 - #54747 (codegen_llvm: verify that inline assembly operands are scalars)
 - #54848 (Better Diagnostic for Trait Object Capture)
 - #54850 (Fix #54707 - parse_trait_item_ now handles interpolated blocks as function body decls)
 - #54858 (second round of refactorings for universes)
 - #54862 (Implement RFC 2539: cfg_attr with multiple attributes)
 - #54869 (Fix mobile docs)
 - #54870 (Stabilize tool lints)
 - #54893 (Fix internal compiler error on malformed match arm pattern.)
 - #54904 (Stabilize the `Option::replace` method)

Failed merges:

 - #54909 ( Add chalk rules related to associated type defs)

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants