Skip to content

Commit

Permalink
Rehaul, Apply code review from Arthur
Browse files Browse the repository at this point in the history
gcc/rust/ChangeLog:

	* backend/rust-compile-asm.cc (CompileAsm::visit):
	Change API, public/private, comments, formatting from code
	review
	(CompileAsm::asm_build_expr): Likewise.
	(CompileAsm::tree_codegen_asm): Likewise.
	* backend/rust-compile-asm.h (class CompileAsm): Likewise.
	* backend/rust-compile-expr.cc (CompileExpr::visit): Likewise.
	* checks/errors/privacy/rust-privacy-reporter.cc (PrivacyReporter::visit): Likewise.
	* expand/rust-macro-builtins-asm.cc (strip_double_quotes): Likewise.
	(parse_options): Likewise.
	(parse_asm_arg): Likewise.
	(expand_inline_asm_strings): Likewise.
	(parse_asm): Likewise.
	* expand/rust-macro-builtins-asm.h (strip_double_quotes): Likewise.
	(expand_inline_asm_strings): Likewise.
	(expand_inline_asm_string): Likewise.
	* hir/tree/rust-hir-expr.h: Likewise.

gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_typecheck.rs: Change comments
  • Loading branch information
badumbatish authored and CohenArthur committed Sep 2, 2024
1 parent 73a1510 commit 3aca66b
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 123 deletions.
8 changes: 1 addition & 7 deletions gcc/rust/backend/rust-compile-asm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,8 @@ namespace Compile {
CompileAsm::CompileAsm (Context *ctx)
: HIRCompileBase (ctx), translated (error_mark_node)
{}
void
CompileAsm::visit (HIR::InlineAsm &expr)
{
ctx->add_statement (asm_build_expr (expr));
}

tree
CompileAsm::asm_build_expr (HIR::InlineAsm &expr)
CompileAsm::tree_codegen_asm (HIR::InlineAsm &expr)
{
auto asm_expr
= asm_build_stmt (expr.get_locus (), {asm_construct_string_tree (expr),
Expand Down
67 changes: 7 additions & 60 deletions gcc/rust/backend/rust-compile-asm.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

// Copyright (C) 2020-2024 Free Software Foundation, Inc.

// This file is part of GCC.
Expand Down Expand Up @@ -26,16 +25,11 @@
namespace Rust {
namespace Compile {

class CompileAsm : private HIRCompileBase, protected HIR::HIRExpressionVisitor
class CompileAsm : private HIRCompileBase
{
private:
tree translated;

public:
// WE WILL OPEN THIS UP WHEN WE WANT TO ADD A DEDICATED PASS OF HIR'S ASM
// translation.
// static tree Compile (HIR::Expr *expr, Context *ctx);

// RELEVANT MEMBER FUNCTIONS

// The limit is 5 because it stands for the 5 things that the C version of
Expand All @@ -46,7 +40,6 @@ class CompileAsm : private HIRCompileBase, protected HIR::HIRExpressionVisitor
// build_asm_expr (location_t loc, tree string, tree outputs, tree inputs,
// tree clobbers, tree labels, bool simple, bool is_inline)
static const int ASM_TREE_ARRAY_LENGTH = 5;
tree asm_build_expr (HIR::InlineAsm &);
tree asm_build_stmt (location_t,
const std::array<tree, ASM_TREE_ARRAY_LENGTH> &);

Expand All @@ -56,60 +49,14 @@ class CompileAsm : private HIRCompileBase, protected HIR::HIRExpressionVisitor
tree asm_construct_clobber_tree (HIR::InlineAsm &);
tree asm_construct_label_tree (HIR::InlineAsm &);

CompileAsm (Context *ctx);

void visit (HIR::InlineAsm &) override;
public:
// WE WILL OPEN THIS UP WHEN WE WANT TO ADD A DEDICATED PASS OF HIR'S ASM
// translation.
// static tree Compile (HIR::Expr *expr, Context *ctx);

// NON RELEVANT MEMBER FUNCTIONS
CompileAsm (Context *ctx);

void visit (HIR::TupleIndexExpr &) override {}
void visit (HIR::TupleExpr &) override {}
void visit (HIR::ReturnExpr &) override {}
void visit (HIR::CallExpr &) override {}
void visit (HIR::MethodCallExpr &) override {}
void visit (HIR::LiteralExpr &) override {}
void visit (HIR::AssignmentExpr &) override {}
void visit (HIR::CompoundAssignmentExpr &) override {}
void visit (HIR::ArrayIndexExpr &) override {}
void visit (HIR::ArrayExpr &) override {}
void visit (HIR::ArithmeticOrLogicalExpr &) override {}
void visit (HIR::ComparisonExpr &) override {}
void visit (HIR::LazyBooleanExpr &) override {}
void visit (HIR::NegationExpr &) override {}
void visit (HIR::TypeCastExpr &) override {}
void visit (HIR::IfExpr &) override {}
void visit (HIR::IfExprConseqElse &) override {}
void visit (HIR::BlockExpr &) override {}
void visit (HIR::UnsafeBlockExpr &) override {}
void visit (HIR::StructExprStruct &struct_) override {}
void visit (HIR::StructExprStructFields &struct_) override {}
void visit (HIR::GroupedExpr &) override {}
void visit (HIR::FieldAccessExpr &) override {}
void visit (HIR::QualifiedPathInExpression &) override {}
void visit (HIR::PathInExpression &) override {}
void visit (HIR::LoopExpr &) override {}
void visit (HIR::WhileLoopExpr &) override {}
void visit (HIR::BreakExpr &) override {}
void visit (HIR::ContinueExpr &) override {}
void visit (HIR::BorrowExpr &) override {}
void visit (HIR::DereferenceExpr &) override {}
void visit (HIR::MatchExpr &) override {}
void visit (HIR::RangeFromToExpr &) override {}
void visit (HIR::RangeFromExpr &) override {}
void visit (HIR::RangeToExpr &) override {}
void visit (HIR::RangeFullExpr &) override {}
void visit (HIR::RangeFromToInclExpr &) override {}
void visit (HIR::ClosureExpr &) override {}
void visit (HIR::ErrorPropagationExpr &) override {}
void visit (HIR::RangeToInclExpr &) override {}
void visit (HIR::WhileLetLoopExpr &) override {}
void visit (HIR::IfLetExpr &) override {}
void visit (HIR::IfLetExprConseqElse &) override {}
void visit (HIR::AwaitExpr &) override {}
void visit (HIR::AsyncBlockExpr &) override {}
void visit (HIR::StructExprFieldIdentifier &) override {}
void visit (HIR::StructExprFieldIdentifierValue &) override {}
void visit (HIR::StructExprFieldIndexValue &) override {}
tree tree_codegen_asm (HIR::InlineAsm &);
};
} // namespace Compile
} // namespace Rust
Expand Down
4 changes: 2 additions & 2 deletions gcc/rust/backend/rust-compile-expr.cc
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,8 @@ CompileExpr::visit (HIR::IfExpr &expr)
void
CompileExpr::visit (HIR::InlineAsm &expr)
{
CompileAsm a (ctx);
a.visit (expr);
CompileAsm asm_codegen (ctx);
ctx->add_statement (asm_codegen.tree_codegen_asm (expr));
// translated = build_asm_expr (0, NULL_TREE, NULL_TREE, NULL_TREE, NULL_TREE,
// NULL_TREE, true, true);
// CompileAsm::asm_build_expr (expr);
Expand Down
4 changes: 1 addition & 3 deletions gcc/rust/checks/errors/privacy/rust-privacy-reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,7 @@ PrivacyReporter::visit (HIR::TypePathSegmentFunction &)

void
PrivacyReporter::visit (HIR::InlineAsm &)
{
return;
}
{}

void
PrivacyReporter::visit (HIR::TypePath &path)
Expand Down
61 changes: 24 additions & 37 deletions gcc/rust/expand/rust-macro-builtins-asm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,20 @@ std::map<AST::InlineAsmOption, std::string> InlineAsmOptionMap{

std::set<std::string> potentially_nonpromoted_keywords
= {"in", "out", "lateout", "inout", "inlateout", "const", "sym", "label"};

std::string
strip_double_quotes (const std::string &str)
{
// Helper function strips the beginning and ending double quotes from a
// string.
std::string result = str;

rust_assert (result.size () >= 3);
result.erase (0, 1);
result.erase (result.size () - 1, 1);
return result;
}

tl::expected<InlineAsmContext, InlineAsmParseError>
parse_clobber_abi (InlineAsmContext inline_asm_ctx)
{
Expand Down Expand Up @@ -561,9 +575,7 @@ parse_options (InlineAsmContext &inline_asm_ctx)

// Parse comma as optional
if (parser.skip_token (COMMA))
{
continue;
}
continue;
else
{
rust_unreachable ();
Expand Down Expand Up @@ -689,9 +701,7 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)
{
auto expected = parse_clobber_abi (inline_asm_ctx);
if (expected.has_value ())
{
continue;
}
continue;
else if (expected.error () == COMMITTED)
return expected;

Expand All @@ -703,9 +713,7 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)
{
auto expected = parse_options (inline_asm_ctx);
if (expected.has_value ())
{
continue;
}
continue;
else if (expected.error () == COMMITTED)
return expected;

Expand All @@ -718,13 +726,9 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)

auto expected = parse_reg_operand (inline_asm_ctx);
if (expected.has_value ())
{
continue;
}
continue;
else if (expected.error () == COMMITTED)
{
return expected;
}
return expected;

// Since parse_reg_operand is the last thing we've considered,
// The non-committed parse error type means that we have exhausted our
Expand All @@ -742,21 +746,8 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)
return tl::expected<InlineAsmContext, InlineAsmParseError> (inline_asm_ctx);
}

std::string
strip_double_quotes (const std::string &str)
{
// Helper function strips the beginning and ending double quotes from a
// string.
std::string result = str;

rust_assert (result.size () >= 3);
result.erase (0, 1);
result.erase (result.size () - 1, 1);
return result;
}

tl::expected<InlineAsmContext, InlineAsmParseError>
expand_inline_asm_strings (InlineAsmContext &inline_asm_ctx)
expand_inline_asm_strings (InlineAsmContext inline_asm_ctx)
{
auto &inline_asm = inline_asm_ctx.inline_asm;

Expand Down Expand Up @@ -846,20 +837,16 @@ parse_asm (location_t invoc_locus, AST::MacroInvocData &invoc,

auto resulting_context = parse_format_strings (inline_asm_ctx)
.and_then (parse_asm_arg)
.and_then (validate);
.and_then (validate)
.and_then (expand_inline_asm_strings);

// TODO: I'm putting the validation here because the rust reference put
// it here Per Arthur's advice we would actually do the validation in a
// different stage. and visit on the InlineAsm AST instead of it's
// context.
auto is_valid = (bool) resulting_context;
if (is_valid)
{
expand_inline_asm_strings (*resulting_context);
}
if (is_valid)
if (resulting_context)
{
auto node = inline_asm_ctx.inline_asm.clone_expr_without_block ();
auto node = (*resulting_context).inline_asm.clone_expr_without_block ();

std::vector<AST::SingleASTNode> single_vec = {};

Expand Down
10 changes: 2 additions & 8 deletions gcc/rust/expand/rust-macro-builtins-asm.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
#include "system.h"
namespace Rust {

std::string
strip_double_quotes (const std::string &str);

enum InlineAsmParseError
{
// Enum for InlineAsmParseError
Expand Down Expand Up @@ -86,12 +83,9 @@ class InlineAsmContext
this->allow_templates = allow_templates;
}
};

tl::expected<InlineAsmContext, InlineAsmParseError>
expand_inline_asm_strings (InlineAsmContext &inline_asm_ctx);

WARN_UNUSED_RESULT
tl::expected<InlineAsmContext, InlineAsmParseError>
expand_inline_asm_string (InlineAsmContext &inline_asm_ctx);
expand_inline_asm_strings (InlineAsmContext inline_asm_ctx);

// Expected calls
WARN_UNUSED_RESULT
Expand Down
4 changes: 2 additions & 2 deletions gcc/rust/hir/tree/rust-hir-expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -4154,8 +4154,8 @@ class InlineAsm : public ExprWithoutBlock

bool is_inline_asm ()
{
// TODO: Check back later to determine how an InlineAsm is inline.
return true;
// INFO: An inline asm is asm!, which is the opposite of a global_asm()
return !this->is_global_asm;
}
InlineAsm (location_t locus, bool is_global_asm,
std::vector<AST::InlineAsmTemplatePiece> template_,
Expand Down
7 changes: 3 additions & 4 deletions gcc/testsuite/rust/compile/inline_asm_typecheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ fn main() {
// This demonstrates that asm!'s is inferred with a unit type is parsed correctly.
let _ = asm!("nop");

// This errors out per rust spec
// The asm! block never returns, and its return type is defined as ! (never).
// Behavior is undefined if execution falls through past the end of the asm code.
// A noreturn asm block behaves just like a function which doesn't return; notably, local variables in scope are not dropped before it is invoked.
// The asm! block never returns, and its return type is defined as ! (never).
// Behavior is undefined if execution falls through past the end of the asm code.
// A noreturn asm block behaves just like a function which doesn't return; notably, local variables in scope are not dropped before it is invoked.
let _ = asm!("nop", options(noreturn));
}
}

0 comments on commit 3aca66b

Please sign in to comment.