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

Support align and packed repr layout on structs #1188

Merged
merged 1 commit into from
Apr 29, 2022
Merged
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
20 changes: 20 additions & 0 deletions gcc/rust/backend/rust-compile-type.cc
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,26 @@ TyTyResolveCompile::visit (const TyTy::ADTType &type)
type_record = ctx->get_backend ()->union_type (enum_fields);
}

// Handle repr options
// TODO: "packed" should only narrow type alignment and "align" should only
// widen it. Do we need to check and enforce this here, or is it taken care of
// later on in the gcc middle-end?
TyTy::ADTType::ReprOptions repr = type.get_repr_options ();
if (repr.pack)
{
TYPE_PACKED (type_record);
if (repr.pack > 1)
{
SET_TYPE_ALIGN (type_record, repr.pack * 8);
TYPE_USER_ALIGN (type_record) = 1;
}
}
else if (repr.align)
{
SET_TYPE_ALIGN (type_record, repr.align * 8);
TYPE_USER_ALIGN (type_record) = 1;
}

std::string named_struct_str
= type.get_ident ().path.get () + type.subst_as_string ();
tree named_struct
Expand Down
67 changes: 67 additions & 0 deletions gcc/rust/typecheck/rust-hir-type-check-base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -260,5 +260,72 @@ TypeCheckBase::resolve_literal (const Analysis::NodeMapping &expr_mappings,
return infered;
}

TyTy::ADTType::ReprOptions
TypeCheckBase::parse_repr_options (const AST::AttrVec &attrs, Location locus)
{
TyTy::ADTType::ReprOptions repr;
repr.pack = 0;
repr.align = 0;

for (const auto &attr : attrs)
{
bool is_repr = attr.get_path ().as_string ().compare ("repr") == 0;
if (is_repr)
{
const AST::AttrInput &input = attr.get_attr_input ();
bool is_token_tree = input.get_attr_input_type ()
== AST::AttrInput::AttrInputType::TOKEN_TREE;
rust_assert (is_token_tree);
const auto &option = static_cast<const AST::DelimTokenTree &> (input);
AST::AttrInputMetaItemContainer *meta_items
= option.parse_to_meta_item ();

const std::string inline_option
= meta_items->get_items ().at (0)->as_string ();

// TODO: it would probably be better to make the MetaItems more aware
// of constructs with nesting like #[repr(packed(2))] rather than
Copy link
Member

Choose a reason for hiding this comment

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

Yeah i think your right here the AST Meta stuff is not very helpful to work with at the moment.

// manually parsing the string "packed(2)" here.

size_t oparen = inline_option.find ('(', 0);
bool is_pack = false, is_align = false;
unsigned char value = 1;

if (oparen == std::string::npos)
{
is_pack = inline_option.compare ("packed") == 0;
is_align = inline_option.compare ("align") == 0;
}

else
{
std::string rep = inline_option.substr (0, oparen);
is_pack = rep.compare ("packed") == 0;
is_align = rep.compare ("align") == 0;

size_t cparen = inline_option.find (')', oparen);
if (cparen == std::string::npos)
{
rust_error_at (locus, "malformed attribute");
}

std::string value_str = inline_option.substr (oparen, cparen);
value = strtoul (value_str.c_str () + 1, NULL, 10);
}

if (is_pack)
repr.pack = value;
else if (is_align)
repr.align = value;

// Multiple repr options must be specified with e.g. #[repr(C,
// packed(2))].
break;
}
}

return repr;
}

} // namespace Resolver
} // namespace Rust
3 changes: 3 additions & 0 deletions gcc/rust/typecheck/rust-hir-type-check-base.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ class TypeCheckBase : public HIR::HIRFullVisitorBase
TyTy::BaseType *resolve_literal (const Analysis::NodeMapping &mappings,
HIR::Literal &literal, Location locus);

TyTy::ADTType::ReprOptions parse_repr_options (const AST::AttrVec &attrs,
Location locus);

Analysis::Mappings *mappings;
Resolver *resolver;
TypeCheckContext *context;
Expand Down
16 changes: 14 additions & 2 deletions gcc/rust/typecheck/rust-hir-type-check-stmt.h
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,18 @@ class TypeCheckStmt : public TypeCheckBase
TyTy::VariantDef::VariantType::TUPLE, nullptr,
std::move (fields)));

// Process #[repr(...)] attribute, if any
const AST::AttrVec &attrs = struct_decl.get_outer_attrs ();
TyTy::ADTType::ReprOptions repr
= parse_repr_options (attrs, struct_decl.get_locus ());

TyTy::BaseType *type
= new TyTy::ADTType (struct_decl.get_mappings ().get_hirid (),
mappings->get_next_hir_id (),
struct_decl.get_identifier (), ident,
TyTy::ADTType::ADTKind::TUPLE_STRUCT,
std::move (variants), std::move (substitutions));
std::move (variants), std::move (substitutions),
repr);

context->insert_type (struct_decl.get_mappings (), type);
infered = type;
Expand Down Expand Up @@ -311,12 +317,18 @@ class TypeCheckStmt : public TypeCheckBase
TyTy::VariantDef::VariantType::STRUCT, nullptr,
std::move (fields)));

// Process #[repr(...)] attribute, if any
const AST::AttrVec &attrs = struct_decl.get_outer_attrs ();
TyTy::ADTType::ReprOptions repr
= parse_repr_options (attrs, struct_decl.get_locus ());

TyTy::BaseType *type
= new TyTy::ADTType (struct_decl.get_mappings ().get_hirid (),
mappings->get_next_hir_id (),
struct_decl.get_identifier (), ident,
TyTy::ADTType::ADTKind::STRUCT_STRUCT,
std::move (variants), std::move (substitutions));
std::move (variants), std::move (substitutions),
repr);

context->insert_type (struct_decl.get_mappings (), type);
infered = type;
Expand Down
16 changes: 14 additions & 2 deletions gcc/rust/typecheck/rust-hir-type-check-toplevel.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,18 @@ class TypeCheckTopLevel : public TypeCheckBase
TyTy::VariantDef::VariantType::TUPLE, nullptr,
std::move (fields)));

// Process #[repr(X)] attribute, if any
const AST::AttrVec &attrs = struct_decl.get_outer_attrs ();
TyTy::ADTType::ReprOptions repr
= parse_repr_options (attrs, struct_decl.get_locus ());

TyTy::BaseType *type
= new TyTy::ADTType (struct_decl.get_mappings ().get_hirid (),
mappings->get_next_hir_id (),
struct_decl.get_identifier (), ident,
TyTy::ADTType::ADTKind::TUPLE_STRUCT,
std::move (variants), std::move (substitutions));
std::move (variants), std::move (substitutions),
repr);

context->insert_type (struct_decl.get_mappings (), type);
}
Expand Down Expand Up @@ -196,12 +202,18 @@ class TypeCheckTopLevel : public TypeCheckBase
TyTy::VariantDef::VariantType::STRUCT, nullptr,
std::move (fields)));

// Process #[repr(X)] attribute, if any
const AST::AttrVec &attrs = struct_decl.get_outer_attrs ();
TyTy::ADTType::ReprOptions repr
= parse_repr_options (attrs, struct_decl.get_locus ());

TyTy::BaseType *type
= new TyTy::ADTType (struct_decl.get_mappings ().get_hirid (),
mappings->get_next_hir_id (),
struct_decl.get_identifier (), ident,
TyTy::ADTType::ADTKind::STRUCT_STRUCT,
std::move (variants), std::move (substitutions));
std::move (variants), std::move (substitutions),
repr);

context->insert_type (struct_decl.get_mappings (), type);
}
Expand Down
3 changes: 2 additions & 1 deletion gcc/rust/typecheck/rust-tyty.cc
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,8 @@ ADTType::clone () const

return new ADTType (get_ref (), get_ty_ref (), identifier, ident,
get_adt_kind (), cloned_variants, clone_substs (),
used_arguments, get_combined_refs ());
get_repr_options (), used_arguments,
get_combined_refs ());
}

static bool
Expand Down
28 changes: 28 additions & 0 deletions gcc/rust/typecheck/rust-tyty.h
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,20 @@ class ADTType : public BaseType, public SubstitutionRef
ENUM
};

// Representation options, specified via attributes e.g. #[repr(packed)]
struct ReprOptions
{
// bool is_c;
// bool is_transparent;
//...

// For align and pack: 0 = unspecified. Nonzero = byte alignment.
// It is an error for both to be nonzero, this should be caught when
// parsing the #[repr] attribute.
unsigned char align = 0;
unsigned char pack = 0;
};
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be worth having some kind of error node state here. For example, during the type-checking of this structure, you could represent an error state by a flag or something so that when you are compiling the type we know if it's ok or not so we can error or ignore the options. This will only really matter down the line when we get rid of the saw_errors guards between each compiler pass.


ADTType (HirId ref, std::string identifier, RustIdent ident, ADTKind adt_kind,
std::vector<VariantDef *> variants,
std::vector<SubstitutionParamMapping> subst_refs,
Expand All @@ -1276,7 +1290,20 @@ class ADTType : public BaseType, public SubstitutionRef
identifier (identifier), variants (variants), adt_kind (adt_kind)
{}

ADTType (HirId ref, HirId ty_ref, std::string identifier, RustIdent ident,
ADTKind adt_kind, std::vector<VariantDef *> variants,
std::vector<SubstitutionParamMapping> subst_refs, ReprOptions repr,
SubstitutionArgumentMappings generic_arguments
= SubstitutionArgumentMappings::error (),
std::set<HirId> refs = std::set<HirId> ())
: BaseType (ref, ty_ref, TypeKind::ADT, ident, refs),
SubstitutionRef (std::move (subst_refs), std::move (generic_arguments)),
identifier (identifier), variants (variants), adt_kind (adt_kind),
repr (repr)
{}

ADTKind get_adt_kind () const { return adt_kind; }
ReprOptions get_repr_options () const { return repr; }

bool is_struct_struct () const { return adt_kind == STRUCT_STRUCT; }
bool is_tuple_struct () const { return adt_kind == TUPLE_STRUCT; }
Expand Down Expand Up @@ -1385,6 +1412,7 @@ class ADTType : public BaseType, public SubstitutionRef
std::string identifier;
std::vector<VariantDef *> variants;
ADTType::ADTKind adt_kind;
ReprOptions repr;
};

class FnType : public BaseType, public SubstitutionRef
Expand Down
1 change: 1 addition & 0 deletions gcc/rust/util/rust-attributes.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ static const BuiltinAttrDefinition __definitions[] = {
{"lang", HIR_LOWERING},
{"link_section", CODE_GENERATION},
{"no_mangle", CODE_GENERATION},
{"repr", CODE_GENERATION},
};

BuiltinAttributeMappings *
Expand Down
19 changes: 19 additions & 0 deletions gcc/testsuite/rust/compile/struct_align1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#[repr(align(8))]
struct Foo {
x: i16,
// { dg-warning "field is never read" "" { target *-*-* } .-1 }
y: i8,
// { dg-warning "field is never read" "" { target *-*-* } .-1 }
z: i32,
// { dg-warning "field is never read" "" { target *-*-* } .-1 }
}

#[repr(align(8))]
struct Bar(i8, i32);

fn main () {
let f = Foo { x: 5, y: 2, z: 13 };
// { dg-warning "unused name" "" { target *-*-* } .-1 }
let b = Bar (7, 262);
// { dg-warning "unused name" "" { target *-*-* } .-1 }
}
18 changes: 18 additions & 0 deletions gcc/testsuite/rust/compile/struct_align2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@

fn main () {

#[repr(align(8))]
struct Baz {
x: u16,
y: u32,
};

#[repr(align(4))]
struct Qux (u8, i16);

let b = Baz { x: 5, y: 1984 };
// { dg-warning "unused name" "" { target *-*-* } .-1 }

let c = Qux (1, 2);
// { dg-warning "unused name" "" { target *-*-* } .-1 }
}
19 changes: 19 additions & 0 deletions gcc/testsuite/rust/compile/struct_pack1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#[repr(packed(2))]
struct Foo {
x: i16,
// { dg-warning "field is never read" "" { target *-*-* } .-1 }
y: i8,
// { dg-warning "field is never read" "" { target *-*-* } .-1 }
z: i32,
// { dg-warning "field is never read" "" { target *-*-* } .-1 }
}

#[repr(packed)]
struct Bar(i8, i32);

fn main () {
let f = Foo { x: 5, y: 2, z: 13 };
// { dg-warning "unused name" "" { target *-*-* } .-1 }
let b = Bar (7, 262);
// { dg-warning "unused name" "" { target *-*-* } .-1 }
}
18 changes: 18 additions & 0 deletions gcc/testsuite/rust/compile/struct_pack2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@

fn main () {

#[repr(packed(2))]
struct Baz {
x: u16,
y: u32,
};

#[repr(packed)]
struct Qux (u8, i16);

let b = Baz { x: 5, y: 1984 };
// { dg-warning "unused name" "" { target *-*-* } .-1 }

let c = Qux (1, 2);
// { dg-warning "unused name" "" { target *-*-* } .-1 }
}