Skip to content

Commit

Permalink
Merge Rust-GCC#780
Browse files Browse the repository at this point in the history 
780: No side effects in 'assert' expressions r=philberty a=tschwinge

Usually, if 'assert'ions are disabled, 'assert' expressions are not evaluated,
so in that case won't effect any side effects.

Via spurious ICEs/test suite FAILs, this may be observed in GCC/Rust, for
example, if configuring with '--enable-checking=no' and disabling a "more
forgiving" 'gcc/system.h:gcc_assert' definition, so that '0 && (EXPR)' gets
used:

     /* Use gcc_assert(EXPR) to test invariants.  */
     #if ENABLE_ASSERT_CHECKING
     #define gcc_assert(EXPR)                                               \
        ((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__), 0 : 0))
    -#elif (GCC_VERSION >= 4005)
    +#elif (0) //GCC_VERSION >= 4005)
     #define gcc_assert(EXPR)                                               \
       ((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0 : 0))
     #else
     /* Include EXPR, so that unused variable warnings do not occur.  */
     #define gcc_assert(EXPR) ((void)(0 && (EXPR)))
     #endif

As that one does cause some issues in GCC proper (that I shall fix separately),
may use this change to 'gcc/rust/rust-system.h:rust_assert' instead:

    +#if 0
     #define rust_assert(EXPR) gcc_assert (EXPR)
    +#else
    +#define rust_assert(EXPR) ((void) (0 && (EXPR)))
    +#endif

To fix these, use the same pattern as is already used in a lot of existing
GCC/Rust code:

    bool ok = [expression with side effects];
    rust_assert (ok);

I've only done a quick manual review; maybe there is a tool for doing this?


Co-authored-by: Thomas Schwinge <thomas@codesourcery.com>
  • Loading branch information
@bors @tschwinge
bors[bot] and tschwinge committed Oct 30, 2021
2 parents a9daecd + f569984 commit ca0b06f
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 22 deletions.
7 changes: 4 additions & 3 deletions gcc/rust/backend/rust-compile-context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,12 @@ class Context
for (auto it = builtins.begin (); it != builtins.end (); it++)
{
HirId ref;
rust_assert (
tyctx->lookup_type_by_node_id ((*it)->get_node_id (), &ref));
bool ok = tyctx->lookup_type_by_node_id ((*it)->get_node_id (), &ref);
rust_assert (ok);

TyTy::BaseType *lookup;
rust_assert (tyctx->lookup_type (ref, &lookup));
ok = tyctx->lookup_type (ref, &lookup);
rust_assert (ok);

Btype *compiled = TyTyCompile::compile (backend, lookup);
compiled_type_map.insert (std::pair<HirId, Btype *> (ref, compiled));
Expand Down
24 changes: 14 additions & 10 deletions gcc/rust/backend/rust-compile-implitem.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,10 @@ class CompileInherentImplItem : public HIRCompileBase
Bexpression *value = CompileExpr::Compile (constant.get_expr (), ctx);

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
ok = ctx->get_mappings ()->lookup_canonical_path (
constant.get_mappings ().get_crate_num (),
constant.get_mappings ().get_nodeid (), &canonical_path));
constant.get_mappings ().get_nodeid (), &canonical_path);
rust_assert (ok);

std::string ident = canonical_path->get ();
Bexpression *const_expr = ctx->get_backend ()->named_constant_expression (
Expand Down Expand Up @@ -148,9 +149,10 @@ class CompileInherentImplItem : public HIRCompileBase
flags |= Backend::function_is_visible;

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
bool ok = ctx->get_mappings ()->lookup_canonical_path (
function.get_mappings ().get_crate_num (),
function.get_mappings ().get_nodeid (), &canonical_path));
function.get_mappings ().get_nodeid (), &canonical_path);
rust_assert (ok);

std::string ir_symbol_name
= canonical_path->get () + fntype->subst_as_string ();
Expand Down Expand Up @@ -260,7 +262,7 @@ class CompileInherentImplItem : public HIRCompileBase
}

std::vector<Bvariable *> locals;
bool ok = compile_locals_for_block (*rib, fndecl, locals);
ok = compile_locals_for_block (*rib, fndecl, locals);
rust_assert (ok);

Bblock *enclosing_scope = NULL;
Expand Down Expand Up @@ -358,9 +360,10 @@ class CompileTraitItem : public HIRCompileBase
= CompileExpr::Compile (constant.get_expr ().get (), ctx);

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
bool ok = ctx->get_mappings ()->lookup_canonical_path (
constant.get_mappings ().get_crate_num (),
constant.get_mappings ().get_nodeid (), &canonical_path));
constant.get_mappings ().get_nodeid (), &canonical_path);
rust_assert (ok);

std::string ident = canonical_path->get ();
Bexpression *const_expr = ctx->get_backend ()->named_constant_expression (
Expand Down Expand Up @@ -413,9 +416,10 @@ class CompileTraitItem : public HIRCompileBase
unsigned int flags = 0;

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
bool ok = ctx->get_mappings ()->lookup_canonical_path (
func.get_mappings ().get_crate_num (), func.get_mappings ().get_nodeid (),
&canonical_path));
&canonical_path);
rust_assert (ok);

std::string fn_identifier = canonical_path->get ();
std::string asm_name = ctx->mangle_item (fntype, *canonical_path);
Expand Down Expand Up @@ -522,7 +526,7 @@ class CompileTraitItem : public HIRCompileBase
}

std::vector<Bvariable *> locals;
bool ok = compile_locals_for_block (*rib, fndecl, locals);
ok = compile_locals_for_block (*rib, fndecl, locals);
rust_assert (ok);

Bblock *enclosing_scope = NULL;
Expand Down
17 changes: 10 additions & 7 deletions gcc/rust/backend/rust-compile-item.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,10 @@ class CompileItem : public HIRCompileBase
Bexpression *value = CompileExpr::Compile (var.get_expr (), ctx);

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
ok = ctx->get_mappings ()->lookup_canonical_path (
var.get_mappings ().get_crate_num (), var.get_mappings ().get_nodeid (),
&canonical_path));
&canonical_path);
rust_assert (ok);

std::string name = canonical_path->get ();
std::string asm_name = ctx->mangle_item (resolved_type, *canonical_path);
Expand Down Expand Up @@ -103,9 +104,10 @@ class CompileItem : public HIRCompileBase
Bexpression *value = CompileExpr::Compile (constant.get_expr (), ctx);

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
ok = ctx->get_mappings ()->lookup_canonical_path (
constant.get_mappings ().get_crate_num (),
constant.get_mappings ().get_nodeid (), &canonical_path));
constant.get_mappings ().get_nodeid (), &canonical_path);
rust_assert (ok);

std::string ident = canonical_path->get ();
Bexpression *const_expr
Expand Down Expand Up @@ -186,9 +188,10 @@ class CompileItem : public HIRCompileBase
flags |= Backend::function_is_visible;

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
bool ok = ctx->get_mappings ()->lookup_canonical_path (
function.get_mappings ().get_crate_num (),
function.get_mappings ().get_nodeid (), &canonical_path));
function.get_mappings ().get_nodeid (), &canonical_path);
rust_assert (ok);

std::string ir_symbol_name
= canonical_path->get () + fntype->subst_as_string ();
Expand Down Expand Up @@ -259,7 +262,7 @@ class CompileItem : public HIRCompileBase
}

std::vector<Bvariable *> locals;
bool ok = compile_locals_for_block (*rib, fndecl, locals);
ok = compile_locals_for_block (*rib, fndecl, locals);
rust_assert (ok);

Bblock *enclosing_scope = NULL;
Expand Down
5 changes: 3 additions & 2 deletions gcc/rust/backend/rust-compile-stmt.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,10 @@ class CompileStmt : public HIRCompileBase
Bexpression *value = CompileExpr::Compile (constant.get_expr (), ctx);

const Resolver::CanonicalPath *canonical_path = nullptr;
rust_assert (ctx->get_mappings ()->lookup_canonical_path (
ok = ctx->get_mappings ()->lookup_canonical_path (
constant.get_mappings ().get_crate_num (),
constant.get_mappings ().get_nodeid (), &canonical_path));
constant.get_mappings ().get_nodeid (), &canonical_path);
rust_assert (ok);

std::string ident = canonical_path->get ();
Bexpression *const_expr
Expand Down

0 comments on commit ca0b06f

Please sign in to comment.