transpile: Use raw borrows instead of normal borrows

[Rua]

• Depends on transpile: Factor out common function for address-of and array-decay #1446.
• Fixes Potential unsoundness caused by emitting borrows #1352.
• Fixes C2Rust Emits Unsafe Reference to Packed Field Causing Rust Error E0793 #1239.
• Fixes Addressing static mut Function Pointer Translation in C2Rust for Safe Rust Semantics #1224.
• Fixes Static string is not borrowed correctly when used multiple times #743.

I noticed that there was some logic to determine whether reference-to-pointer decay happens automatically, or needs a cast. I don't know if that's still needed at all, I can strip it out if not. I never dealt with inline assembly before, so I wasn't sure how to handle references vs pointers in that context. I left it as it is for now.

[Rua]

Hmm, the CI failed on &raw mut not being allowed in statics. I thought I had tested that...

[Rua]

I asked around on The #dark-arts channel of the Rust Discord, and they assured me that &raw mut is allowed in statics. It seems that in current Rust, &raw mut is allowed in statics, but in the nightly being used for c2rust, it was not yet allowed?

[bungcip]

c2rust-transpile still use old nightly ( i think 2022-08-08), and in those version, &raw mut still unstable. I expect there is still bug in those version

[Rua]

In that nightly version, the addr_of and addr_of_mut macros are already stabilised. And internally, those macros just call &raw const and &raw mut. So any bugs would have affected stable code.

[Rua]

Sadly I am not able to make this work right now. I'm running into niche cases where a temporary is borrowed and its lifetime is extended. Rust does not extend the lifetime when using the raw borrow operator, only the normal borrow operator. This is making a few of the tests still fail.

If there is a way to know exactly in which cases the borrow operator references a temporary, then that could potentially solve the issue.

[Rua]

I think I found a way to deal with the issue, at least in the case of compound literals. That makes this pretty much ready I think, all the tests run successfully.

Two questions still remain:

• DecayRef doesn't seem needed anymore. Should I remove it?
• At https://github.com/immunant/c2rust/blob/master/c2rust-transpile/src/translator/named_references.rs#L127-L132 there is a bit of code that takes a ref mut. Changing that to a pointer may still be good, but if I do so, the binary_conditionals test stops compiling, so I've left it.

[kkysen]

Thanks for tackling this!

Does this fix #301, too?

Also, could you add the test cases from all of the issues? Preferably as snapshot tests.

[Rua]

Sorry for all the code churn. The commits kinda got messed up by rebasing and I decided to just redo everything from the start.

Currently, the CI is failing because it's trying to raw-borrow what is being translated to a const in Rust. In C, this is a macro that expands to a literal string. I think because it's a macro, the special exception for string literals here is no longer triggering, and it's instead being processed as a regular array-to-pointer decay. But because the macro expansion is being stored as a const, this doesn't work.
https://github.com/immunant/c2rust/blob/master/c2rust-transpile/src/translator/mod.rs#L4503

Is there a way to test whether an array-to-pointer decay expression is a macro expansion that was translated into a Rust const like this? Then I can add a special case to handle that situation.

[fw-immunant]

Sorry for all the code churn. The commits kinda got messed up by rebasing and I decided to just redo everything from the start.

Currently, the CI is failing because it's trying to raw-borrow what is being translated to a const in Rust. In C, this is a macro that expands to a literal string. I think because it's a macro, the special exception for string literals here is no longer triggering, and it's instead being processed as a regular array-to-pointer decay. But because the macro expansion is being stored as a const, this doesn't work. https://github.com/immunant/c2rust/blob/master/c2rust-transpile/src/translator/mod.rs#L4503

Is there a way to test whether an array-to-pointer decay expression is a macro expansion that was translated into a Rust const like this? Then I can add a special case to handle that situation.

ctx.is_const on the AstContext indicates whether translation is currently in a const context. Translation can branch on that to either choose a different (const-compatible) idiom for expressing the C computation or to fail (return Err(_)) and the caller will attempt to translate the code without creating a const.

[Rua]

I'm marking this as a draft for now, because there's still some issues to be figured out regarding temporaries being borrowed. Probably depends on a solution for #1217 and #1379.

[Rua]

ctx.is_const on the AstContext indicates whether translation is currently in a const context. Translation can branch on that to either choose a different (const-compatible) idiom for expressing the C computation or to fail (return Err(_)) and the caller will attempt to translate the code without creating a const.

I added a check for ctx.is_const, but it seems that the value is false, so the CI is generating raw-borrow errors instead of using as_ptr.

EDIT: I figured out I needed translate_as_macro instead. Now the tests are almost passing.

[Rua]

I haven't abandoned this PR, but as I worked on it I kept finding more things that needed fixing first before this can work:

• transpile: Resolve expression first in address-of and array-decay #1481
• transpile: Factor out common function for address-of and array-decay #1446
• transpile: Translate va_list with correct types #1443
• transpile: Use literal directly when taking address of string literal #1434
• transpile: Extend lifetime of compound literals whose address is taken #1407

[kkysen]

I haven't abandoned this PR, but as I worked on it I kept finding more things that needed fixing first before this can work:

Thanks for the continued work! This is definitely a big issue, and fixing it would make our output significantly more sound, so it's very much appreciated!

[Rua]

At long last this is finally passing all the tests! I think it's ready to be reviewed now.

[kkysen]

I'm rebasing this after I merged #1494 since they overlap a bunch, and it's easier to see what's happening here post #1494.

[kkysen]

Awesome! LGTM. There's just one little comment about the comment. I can fix that, though, and merge.

[kkysen]

Oh, I also think this fixes #301. You or I should also add a test for that.

[kkysen]

I added the test from #301, but it still triggers UB in miri (playground) even with the &raw mut instead of &mut.

[workingjubilee]

Per Ralf:

Specifically, when you have a local variable f and write f.len = ..., then (like if you had used a mutable reference) this asserts to the compiler that f is the unique way to access this memory, so any outstanding other pointers (safe references or unsafe raw pointers) now become invalid. This is crucial to enable the compiler to perform optimizations based on these aliasing rules.

pointers have to be recreated after that field access.

[kkysen]

Per Ralf:

Specifically, when you have a local variable f and write f.len = ..., then (like if you had used a mutable reference) this asserts to the compiler that f is the unique way to access this memory, so any outstanding other pointers (safe references or unsafe raw pointers) now become invalid. This is crucial to enable the compiler to perform optimizations based on these aliasing rules.

pointers have to be recreated after that field access.

What about if it was (*f).len = instead (where f is a *mut _)? Is there no way to have multiple *muts that stay valid as long as there is no concurrent access?