Conversation
d40f4f6 to
b379b60
Compare
| rust_error_at (expr.get_locus (), | ||
| "overflow in constant expression"); |
There was a problem hiding this comment.
This needs to use error code E0080 like rustc does. The message also does not have to conform to what GCC does and can be more explicit
|
Even if the const folder from C++ catches the error for signed types during const evals, the fact that we don't have a location is pretty annoying. It would be easy to change this to properly catch both signed and unsigned overflows. I would also suggest moving this into a separate function, and putting this in the typechecker rather than in the backend. |
There was a problem hiding this comment.
This test needs to be split between checks for unsigned overflows and signed overflows
|
@CohenArthur Two things I'd like to confirm before implementing: The const-context tracking that covers consts, statics, enum discriminants and array lengths lives in ConstChecker, so I'm planning to put the overflow check there. This requires using query_compile_const_expr from checks/errors/, which would be a new checks/ backend/ dependency. |
philberty
left a comment
There was a problem hiding this comment.
This isnt the right place to add this check we should be doing this overflow check inside the const-expr code directly as a more general case.
| return; | ||
| } | ||
|
|
||
| // Rust rejects arithmetic overflow during constant evaluation, but GCC only |
There was a problem hiding this comment.
This is the wrong place to implement this there are 2 places:
- Constant items
- Constant Blocks
Constant items will do constexpr eval which is not signalling overflow errors at the moment
See
tree
HIRCompileBase::compile_constant_item (
HirId coercion_id, TyTy::BaseType *resolved_type,
TyTy::BaseType *expected_type, const Resolver::CanonicalPath &canonical_path,
HIR::Expr &const_value_expr, location_t locus, location_t expr_locus)
{
it calls fold_expr to call into our constant folder but we are missing this in fold_expr call in
void
CompileExpr::visit (HIR::AnonConst &expr)
{
expr.get_inner_expr ().accept_vis (*this);
}
void
CompileExpr::visit (HIR::ConstBlock &expr)
{
expr.get_const_expr ().accept_vis (*this);
}
Those need to do full const expr for inline const supprt.
We cant inline overflow checks like this it needs to be done in the constexpr.cc code.
There was a problem hiding this comment.
before i forget we do const eval in a few other places like array capacity expr and statics
|
Thanks @philberty that makes sense, I'll move it into constexpr.cc. Two scope questions: should AnonConst/ConstBlock full const-eval go in this PR or a follow-up? And is E0080 acceptable for const fn bodies, where rustc uses the arithmetic_overflow lint instead? |
Rust rejects overflow in const eval for signed and unsigned alike, but GCC only sets TREE_OVERFLOW for signed ops, so unsigned just wrapped silently. The signed case did get caught, but only after folding, against an INTEGER_CST that carries no location - hence the locationless error. So don't fold const arithmetic when building it. The const folder then sees the operation and its operands, and can report overflow on both signednesses with a proper location. gcc/rust/ChangeLog: * rust-backend.h (arithmetic_or_logical_expression): Add FOLD_P. * rust-gcc.cc (arithmetic_or_logical_expression): Only fold when FOLD_P. * backend/rust-compile-expr.cc (CompileExpr::visit): Do not fold arithmetic built in a constant context. * backend/rust-constexpr.cc (eval_check_overflow_p): New function. (eval_binary_expression): Use it. gcc/testsuite/ChangeLog: * rust/compile/const_overflow_unsigned.rs: Rewrite for E0080. * rust/compile/const_overflow_signed.rs: New test. * rust/compile/torture/arrays5.rs: Require lp64 target. * rust/compile/torture/arrays6.rs: Likewise. Signed-off-by: Utkarsh Bahuguna <utkarshbahuguna10@gmail.com>
b379b60 to
302d1d8
Compare
|
Pushed a rewrite after iterating on the review comments. The @CohenArthur E0080/rustc wording is in place, tests are split, and the line-0 workaround is gone. Uncalled
|
|
yeah we should make a follow up for const block and anon const to do a full const expr eval cant think off top of my head best way to implement that though because thats a block expr in consts and statics i basically turn it into a little function to const expr on hmm would have to take a look to remind myself of simplest way to implement that |
|
yeah and "nd is E0080 acceptable for const fn bodies, where rustc uses the arithmetic_overflow lint instead?" seems grand it can be improved later on in my opinion if we want more granular error codes and flags |
|
@philberty gentle ping on this one you answered both questions above (follow-up for const block/anon const, and E0080 being fine for const fn bodies for now), but the review is still marked as requesting changes so it's blocked. Could you take another look when you get a chance? CI is green. |
Rust errors on const-eval overflow even for unsigned, but GCC only sets TREE_OVERFLOW for signed ops (unsigned wraparound is defined in C), so the rust-constexpr.cc check never catches it.
Check before folding instead; this covers addition, subtraction and multiplication on statically-known operands in const context.
gcc/rust/ChangeLog:
gcc/testsuite/ChangeLog: