Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion c2rust-ast-exporter/src/AstExporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1641,10 +1641,29 @@ class TranslateASTVisitor final

bool VisitUnaryOperator(UnaryOperator *UO) {
std::vector<void *> childIds = {UO->getSubExpr()};
encode_entry(UO, TagUnaryOperator, childIds, [UO](CborEncoder *array) {

encode_entry(UO, TagUnaryOperator, childIds, [this, UO](CborEncoder *array) {
cbor_encode_string(array, UO->getOpcodeStr(UO->getOpcode()).str());
cbor_encode_boolean(array, UO->isPrefix());

if (UO->getOpcode() == UO_Deref) {
QualType eltTy = UO->getSubExpr()->getType()->getPointeeType();
CharUnits align = Context->getTypeAlignInChars(eltTy);

if (auto *TD = eltTy->getAs<TypedefType>()) {
QualType naturalTy = TD->getDecl()->getUnderlyingType();
CharUnits naturalAlign = Context->getPreferredTypeAlignInChars(naturalTy);

if (align < naturalAlign) {
cbor_encode_int(array, align.getQuantity());
} else {
cbor_encode_int(array, -1);
}
Comment on lines +1657 to +1661
Copy link
Contributor

Choose a reason for hiding this comment

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

Overalignment can be useful to handle as well, so maybe just cbor_encode_uint both the actual and natural alignment?


}
}
});

Comment on lines +1645 to +1666
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have LLM'd my way to a solution here, this is probably terrible c++ code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super familiar with libclang either. Looks like it might work? I can look into it more later.

return true;
}

Expand Down
9 changes: 8 additions & 1 deletion c2rust-transpile/src/c_ast/conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1250,7 +1250,14 @@ impl ConversionContext {
.as_str()
{
"&" => UnOp::AddressOf,
"*" => UnOp::Deref,
"*" => {
let align: i32 = from_value(node.extras[2].clone())
.expect("Expected prefix information");

UnOp::Deref {
unaligned: align != -1,
}
}
"+" => UnOp::Plus,
"-" => UnOp::Negate,
"~" => UnOp::Complement,
Expand Down
30 changes: 15 additions & 15 deletions c2rust-transpile/src/c_ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1230,28 +1230,28 @@ pub enum CastKind {
/// Represents a unary operator in C (6.5.3 Unary operators) and GNU C extensions
#[derive(Debug, Clone, Copy)]
pub enum UnOp {
AddressOf, // &x
Deref, // *x
Plus, // +x
PostIncrement, // x++
PreIncrement, // ++x
Negate, // -x
PostDecrement, // x--
PreDecrement, // --x
Complement, // ~x
Not, // !x
Real, // [GNU C] __real x
Imag, // [GNU C] __imag x
Extension, // [GNU C] __extension__ x
Coawait, // [C++ Coroutines] co_await x
AddressOf, // &x
Deref { unaligned: bool }, // *x
Plus, // +x
PostIncrement, // x++
PreIncrement, // ++x
Negate, // -x
PostDecrement, // x--
PreDecrement, // --x
Complement, // ~x
Not, // !x
Real, // [GNU C] __real x
Imag, // [GNU C] __imag x
Extension, // [GNU C] __extension__ x
Coawait, // [C++ Coroutines] co_await x
Comment on lines +1233 to +1246
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting here is annoying 😩. Could you add another commit that just makes all of the same-line // comments into proper doc comments above the variant with a newline after each one? Or I could if you prefer.

}

impl UnOp {
pub fn as_str(&self) -> &'static str {
use UnOp::*;
match self {
AddressOf => "&",
Deref => "*",
Deref { .. } => "*",
Plus => "+",
PreIncrement => "++",
PostIncrement => "++",
Expand Down
11 changes: 11 additions & 0 deletions c2rust-transpile/src/translator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3045,6 +3045,17 @@ impl<'c> Translation<'c> {
Ok(mk().call_expr(read_volatile_expr, vec![addr_lhs]))
}

/// Write to a `lhs` that may be unaligned
pub fn unaligned_write(
&self,
lhs: Box<Expr>,
lhs_type: CQualTypeId,
rhs: Box<Expr>,
) -> TranslationResult<Box<Expr>> {
let addr_lhs = self.addr_lhs(lhs, lhs_type, true)?;
Ok(mk().method_call_expr(addr_lhs, "write_unaligned", vec![rhs]))
}

// Compute the offset multiplier for variable length array indexing
// Rust type: usize
pub fn compute_size_of_expr(&self, type_id: CTypeId) -> Option<Box<Expr>> {
Expand Down
37 changes: 34 additions & 3 deletions c2rust-transpile/src/translator/operators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,11 @@ impl<'c> Translation<'c> {
.get_qual_type()
.ok_or_else(|| format_err!("bad initial lhs type"))?;

let is_unaligned = matches!(
initial_lhs,
CExprKind::Unary(_, c_ast::UnOp::Deref { unaligned: true }, _, _)
);

let bitfield_id = match initial_lhs {
CExprKind::Member(_, _, decl_id, _, _) => {
let kind = &self.ast_context[*decl_id].kind;
Expand Down Expand Up @@ -357,7 +362,17 @@ impl<'c> Translation<'c> {
use c_ast::BinOp::*;
let assign_stmt = match op {
// Regular (possibly volatile) assignment
Assign if !is_volatile => WithStmts::new_val(mk().assign_expr(write, rhs)),
Assign if !is_volatile => {
if is_unaligned {
WithStmts::new_val(self.unaligned_write(
write,
initial_lhs_type_id,
rhs,
)?)
} else {
WithStmts::new_val(mk().assign_expr(write, rhs))
}
}
Assign => WithStmts::new_val(self.volatile_write(
write,
initial_lhs_type_id,
Expand Down Expand Up @@ -822,7 +837,7 @@ impl<'c> Translation<'c> {

match arg_kind {
// C99 6.5.3.2 para 4
CExprKind::Unary(_, c_ast::UnOp::Deref, target, _) => {
CExprKind::Unary(_, c_ast::UnOp::Deref { unaligned: _ }, target, _) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CExprKind::Unary(_, c_ast::UnOp::Deref { unaligned: _ }, target, _) => {
CExprKind::Unary(_, c_ast::UnOp::Deref { .. }, target, _) => {

If we don't care about the alignment, could it just be this? Like you did for the other match.

return self.convert_expr(ctx, *target)
}
// An AddrOf DeclRef/Member is safe to not decay if the translator isn't already giving a hard
Expand Down Expand Up @@ -889,7 +904,7 @@ impl<'c> Translation<'c> {
c_ast::UnOp::PreDecrement => self.convert_pre_increment(ctx, cqual_type, false, arg),
c_ast::UnOp::PostIncrement => self.convert_post_increment(ctx, cqual_type, true, arg),
c_ast::UnOp::PostDecrement => self.convert_post_increment(ctx, cqual_type, false, arg),
c_ast::UnOp::Deref => {
c_ast::UnOp::Deref { unaligned } => {
match self.ast_context[arg].kind {
CExprKind::Unary(_, c_ast::UnOp::AddressOf, arg_, _) => {
self.convert_expr(ctx.used(), arg_)
Expand All @@ -903,7 +918,23 @@ impl<'c> Translation<'c> {
Ok(unwrap_function_pointer(val))
} else if let Some(_vla) = self.compute_size_of_expr(ctype) {
Ok(val)
} else if unaligned {
// We should use read_unaligned here:
// mk().method_call_expr(val, "read_unaligned", vec![]);
// but that interferes with `write_unaligned`

let mut val =
mk().unary_expr(UnOp::Deref(Default::default()), val);

// If the type on the other side of the pointer we are dereferencing is volatile and
// this whole expression is not an LValue, we should make this a volatile read
if lrvalue.is_rvalue() && cqual_type.qualifiers.is_volatile
{
val = self.volatile_read(val, cqual_type)?
}
Ok(val)
Comment on lines +921 to +935
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused by the value of lrvalue. I'd expect ptr to be an lvalue in e.g. *ptr = value, and an rvalue in the case return *ptr, but both are apparently lvalues. Hence, I don't see how to differentiate between reads and writes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding to my confusion, this will not actually emit a read_volatile. Maybe I misunderstand what the attribute does here but I think that's a bug?

uint32_t volatile_read(volatile void* ptr) {
    return *((volatile unaligned_uint32*)ptr);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm yeah this just seems wrong https://c.godbolt.org/z/sPcMM4b1T. The lrvalue is always LValue (I think a pointer dereference is just always an lvalue currently?), so that volatile read is never emitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's a bug, right? I can open an issue for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the volatile code here just for trying to figure out how to emit read_aligned correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, nevermind, it's already existing code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in return *ptr, both ptr and *ptr are lvalues in C. See https://godbolt.org/z/bxrs7dczs. return *ptr is the rvalue. So the rvalue part of it comes not from the dereference, but from what's done with the dereference, if that makes sense. Like in *p + 1, the whole *p + 1 expression is an rvalue, but the *p itself is still an lvalue because *p is writable as well, i.e., *p = 2 is well-formed while *p + 1 = 2 is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that is what makes this code confusing: I don't think it can ever be an rvalue in a way that is relevant, so that branch is unreachable.

Also in general that means we don't have great way of distinguishing whether a * means a read, write, or both. That is fine when a C * is translated with a rust *, but when you want to use the methods, it's a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also there are already a bunch of open issues wrt volatile reads and writes. They just don't actually currently work as far as I can tell.

} else {
dbg!("otherwise");
let mut val =
mk().unary_expr(UnOp::Deref(Default::default()), val);

Expand Down
19 changes: 19 additions & 0 deletions c2rust-transpile/tests/snapshots/aligned_read_write.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#include <stdint.h>

typedef __attribute__((aligned(1))) uint32_t unaligned_uint32;

uint32_t unaligned_read(const void* ptr) {
return *((const unaligned_uint32*)ptr);
}

uint32_t aligned_read(const void* ptr) {
return *((const uint32_t*)ptr);
}

void unaligned_write(void* ptr, uint32_t value) {
*((unaligned_uint32*)ptr) = value;
}

void aligned_write(const void* ptr, uint32_t value) {
*((uint32_t*)ptr) = value;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
---
source: c2rust-transpile/tests/snapshots.rs
expression: "&rust"
input_file: c2rust-transpile/tests/snapshots/aligned_read_write.c
---
#![allow(
dead_code,
mutable_transmutes,
non_camel_case_types,
non_snake_case,
non_upper_case_globals,
unused_assignments,
unused_mut
)]
pub type __uint32_t = std::ffi::c_uint;
pub type uint32_t = __uint32_t;
pub type unaligned_uint32 = uint32_t;
#[no_mangle]
pub unsafe extern "C" fn unaligned_read(mut ptr: *const std::ffi::c_void) -> uint32_t {
return *(ptr as *const unaligned_uint32);
}
Comment on lines +19 to +21
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this is still wrong. I can make this one work, but then the unaligned write below is no longer correct.

#[no_mangle]
pub unsafe extern "C" fn aligned_read(mut ptr: *const std::ffi::c_void) -> uint32_t {
return *(ptr as *const uint32_t);
}
#[no_mangle]
pub unsafe extern "C" fn unaligned_write(
mut ptr: *mut std::ffi::c_void,
mut value: uint32_t,
) {
(ptr as *mut unaligned_uint32).write_unaligned(value);
}
#[no_mangle]
pub unsafe extern "C" fn aligned_write(
mut ptr: *const std::ffi::c_void,
mut value: uint32_t,
) {
*(ptr as *mut uint32_t) = value;
}
Loading