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

transpile: Make volatile post-increment assignment compile #1135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GPHemsley
Copy link
Contributor

@GPHemsley GPHemsley commented Sep 21, 2024

@kkysen kkysen self-requested a review September 22, 2024 12:52
tests/ints/src/volatile.c Outdated Show resolved Hide resolved
@kkysen kkysen changed the title transpile: Make volatile post-increment assignment compile transpile: Make volatile post-increment assignment compile Sep 22, 2024
Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! I'm still trying to understand what exactly is happening here and why the code was fine before for non-volatile writes, as I'm not too familiar with this part of the code. So still reviewing...

@GPHemsley
Copy link
Contributor Author

GPHemsley commented Sep 22, 2024

Thanks for fixing this! I'm still trying to understand what exactly is happening here and why the code was fine before for non-volatile writes, as I'm not too familiar with this part of the code. So still reviewing...

I also don't understand, but I've also added a test case to prove that non-volatile writes were indeed working before.

@GPHemsley GPHemsley requested a review from kkysen September 22, 2024 13:33
@GPHemsley
Copy link
Contributor Author

I think it's simply rare that volatile writes like this occur, and so this scenario was not commonly hit.

There isn't a comprehensive test suite for the operator code, so edge cases are not always being tested.

@GPHemsley
Copy link
Contributor Author

As for why assign_expr() includes a semicolon when passed through expr_stmt() and call_expr() doesn't, I suppose I haven't tracked that down yet.

    pub fn assign_expr(self, lhs: Box<Expr>, rhs: Box<Expr>) -> Box<Expr> {
        Box::new(Expr::Assign(ExprAssign {
            attrs: self.attrs,
            eq_token: Token![=](self.span),
            left: lhs,
            right: rhs,
        }))
    }
    pub fn call_expr(self, func: Box<Expr>, args: Vec<Box<Expr>>) -> Box<Expr> {
        let args = args.into_iter().map(|a| *a).collect();
        Box::new(parenthesize_if_necessary(Expr::Call(ExprCall {
            attrs: self.attrs,
            paren_token: token::Paren(self.span),
            func,
            args,
        })))
    }
    pub fn expr_stmt(self, expr: Box<Expr>) -> Stmt {
        Stmt::Expr(*expr)
    }

    pub fn semi_stmt(self, expr: Box<Expr>) -> Stmt {
        Stmt::Semi(*expr, Token![;](self.span))
    }
    #[cfg_attr(doc_cfg, doc(cfg(feature = "printing")))]
    impl ToTokens for Stmt {
        fn to_tokens(&self, tokens: &mut TokenStream) {
            match self {
                Stmt::Local(local) => local.to_tokens(tokens),
                Stmt::Item(item) => item.to_tokens(tokens),
                Stmt::Expr(expr) => expr.to_tokens(tokens),
                Stmt::Semi(expr, semi) => {
                    expr.to_tokens(tokens);
                    semi.to_tokens(tokens);
                }
            }
        }
    }

@GPHemsley
Copy link
Contributor Author

If it helps, this code:

	int i = 0;
	i++;
	int i2 = i++;
	int i3 = i--;
	i--;

	volatile int vi = 0;
	vi++;
	volatile int vi2 = vi++;
	volatile int vi3 = vi--;
	vi--;

generates this code:

    let mut i: libc::c_int = 0 as libc::c_int;
    i += 1;
    i;
    let fresh0 = i;
    i = i + 1;
    let mut i2: libc::c_int = fresh0;
    let fresh1 = i;
    i = i - 1;
    let mut i3: libc::c_int = fresh1;
    i -= 1;
    i;
    let mut vi: libc::c_int = 0 as libc::c_int;
    ::core::ptr::write_volatile(
        &mut vi as *mut libc::c_int,
        ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int) + 1,
    );
    ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int);
    let fresh2 = ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int);
    ::core::ptr::write_volatile(
        &mut vi as *mut libc::c_int,
        ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int) + 1,
    )
    let mut vi2: libc::c_int = fresh2;
    let fresh3 = ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int);
    ::core::ptr::write_volatile(
        &mut vi as *mut libc::c_int,
        ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int) - 1,
    )
    let mut vi3: libc::c_int = fresh3;
    ::core::ptr::write_volatile(
        &mut vi as *mut libc::c_int,
        ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int) - 1,
    );
    ::core::ptr::read_volatile::<libc::c_int>(&vi as *const libc::c_int);

convert_post_increment() is the only time a call to volatile_write() ends up in the stmts part of WithStmts; other calls have it end up in val, which is why the other lines don't have this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants