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.

Sign up for GitHub

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

Jump to bottom

gccrs: feat: Made changes to ensure no wrong assignments are done. #3300

Open

sriganeshres wants to merge 1 commit into Rust-GCC:master from sriganeshres:feat-wrong-assignment

+70 −0

sriganeshres commented Dec 11, 2024

gcc/rust/ChangeLog:

* backend/rust-compile-expr.cc (lvalue_p): Created a function that checks the lvalue. (CompileExpr::visit): Modified the function to check the lvalue using the above mentioned function and also checks the type of lvalue expression.

gcc/testsuite/ChangeLog:

* rust/compile/issue-3287.rs: New test for testing wrong assignments.

Fixes #3287

GCC development requires copyright assignment or the Developer's Certificate of Origin sign-off, see https://gcc.gnu.org/contribute.html or https://gcc.gnu.org/dco.html
Read contributing guidlines
make check-rust passes locally
Run clang-format
Added any relevant test cases to gcc/testsuite/rust/

sriganeshres force-pushed the feat-wrong-assignment branch from 41640eb to 167b909 Compare

December 11, 2024 17:25

philberty requested changes

View reviewed changes

Member

philberty left a comment

There are changes required here i think we can get away wit this check but not totally convinced yet

Also it will need to be applied to the other assignments such as:

x +=1 etc

gcc/rust/backend/rust-compile-expr.cc Outdated

@@ @@ -958,12 +958,71 @@ CompileExpr::visit (HIR::LiteralExpr &expr) @@
                   }
               }
+              bool
+              lvalue_p (const_tree ref)

Member

philberty Dec 11, 2024

this should be renamed and moved into rust-compile-base.cc or into rust-tree.cc call it is_lvalue_tree

gcc/rust/backend/rust-compile-expr.cc Outdated

+                  case COMPONENT_REF:
+                    return lvalue_p (TREE_OPERAND (ref, 0));
+                  case C_MAYBE_CONST_EXPR:

Member

philberty Dec 11, 2024

c_ TREE's dont exists here remove this case

gcc/rust/backend/rust-compile-expr.cc Outdated

+                  case RESULT_DECL:
+                  case ERROR_MARK:
+                    return (TREE_CODE (TREE_TYPE (ref)) != FUNCTION_TYPE
+              	      && TREE_CODE (TREE_TYPE (ref)) != METHOD_TYPE);

Member

philberty Dec 11, 2024

iirc METHOD_TYPE might not be a common tree type either

Author

sriganeshres Dec 11, 2024

Should i remove that

gcc/rust/backend/rust-compile-expr.cc Outdated

+                  case TRUNC_DIV_EXPR:
+                    return false;
+                  default:
+                    rust_debug ("unknown");

Member

philberty Dec 11, 2024

remove this useless debug

gcc/rust/backend/rust-compile-expr.cc Outdated

               void
               CompileExpr::visit (HIR::AssignmentExpr &expr)
               {
                 auto lvalue = CompileExpr::Compile (expr.get_lhs (), ctx);
                 auto rvalue = CompileExpr::Compile (expr.get_rhs (), ctx);
+                if (!lvalue_p (lvalue)

Member

philberty Dec 11, 2024

I dont like if statements like this its hard to read:

it should be:

bool valid_lvalue = XXX
if (!valid_lvalue) {
-- error diagnostic
}

gcc/rust/backend/rust-compile-expr.cc Outdated

+                    || expr.get_lhs ().get_expression_type ()
+              	   == HIR::Expr::ExprType::Operator)
+                  {
+                    rust_error_at (expr.get_lhs ().get_locus (),

Member

philberty Dec 11, 2024

you need to add in the rust error code ussing the proper interface

Author

sriganeshres Dec 11, 2024

Can you give me more clarity

Member

philberty Dec 12, 2024

you need to pass the rust error code into this function like we do in other places take a look through the other parts of the code to check

philberty assigned sriganeshres

philberty added the diagnostic label

sriganeshres force-pushed the feat-wrong-assignment branch 2 times, most recently from 9bd6356 to 5748d31 Compare

December 11, 2024 19:53

sriganeshres requested a review from philberty

December 11, 2024 19:54

sriganeshres force-pushed the feat-wrong-assignment branch 4 times, most recently from fc569e4 to 2a551f5 Compare

December 14, 2024 16:29

Author

sriganeshres commented Dec 14, 2024

There are changes required here i think we can get away wit this check but not totally convinced yet

Also it will need to be applied to the other assignments such as:

x +=1 etc

Can you give me more information about the case we are missing as x+= 1 is valid as long as x is mutable in rust.

sriganeshres force-pushed the feat-wrong-assignment branch 3 times, most recently from 944688f to 1089291 Compare

December 15, 2024 13:25


          gccrs: feat: Made changes to ensure no wrong assignments are done.

e6a88dc

gcc/rust/ChangeLog:

	* backend/rust-compile-base.cc (HIRCompileBase::lvalue_p): Created a function that
	checks for lvalue.
	* backend/rust-compile-base.h: Created the Signature for above function.
	* backend/rust-compile-expr.cc (CompileExpr::visit): Made changes to ensure
	proper readability and checking for wrong assignments.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-3297.rs: New test.

Signed-off-by: Sri Ganesh Thota <[email protected]>

sriganeshres force-pushed the feat-wrong-assignment branch from 1089291 to e6a88dc Compare

December 15, 2024 14:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels