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.

Sign up for GitHub

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

Fix compiler crash #4505

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Fix compiler crash #4505

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d214c75
Fix issue by using `consume` with an invalid expression
ArthurPV Apr 14, 2024
1eff3bb
Test the changes that resolve the issue #4477
ArthurPV Apr 14, 2024
ee0e5f5
Add a release notes.
ArthurPV Apr 14, 2024
0aab38d
Update .release-notes/4505.md
SeanTAllen Apr 16, 2024
1b87354
Update .release-notes/4505.md
SeanTAllen Apr 16, 2024
7a5df36
Update 4505.md
SeanTAllen Apr 16, 2024
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
10 changes: 10 additions & 0 deletions .release-notes/4505.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
## Fix: Segmentation fault when ponyc compiles this code

Fix an issue caused by passing a wrong expression to `consume`.

With this change, instead of the compiler generating a
segmentation fault, it generates the error message: "You cannot
consume an expression that is not local. Specifically, you can
only consume a local variable (a single lowercase identifier, without
a dot) or a this field (this followed by a dot and a single lowercase
identifier)".
SeanTAllen marked this conversation as resolved.
Show resolved Hide resolved
57 changes: 36 additions & 21 deletions src/libponyc/pass/refer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ static bool is_this_incomplete(pass_opt_t* opt, ast_t* ast)
// is used to get the definition for the type based on the `ast_data` to ensure
// at some point the field is tied to a real type even if we haven't quite fully
// determined the type of each field/subfield reference yet.
static const char* generate_multi_dot_name(ast_t* ast, ast_t** def_found) {
static const char* generate_multi_dot_name(pass_opt_t *opt, ast_t* ast, ast_t** def_found, bool in_consume_ctx, bool *has_consume_error) {
pony_assert(ast_id(ast) == TK_DOT);

ast_t* def = NULL;
Expand Down Expand Up @@ -141,6 +141,20 @@ static const char* generate_multi_dot_name(ast_t* ast, ast_t** def_found) {
{
if (def == NULL)
return stringtab("");
else if (in_consume_ctx) {
pony_assert(has_consume_error);

*has_consume_error = true;

ast_error(opt->check.errors, temp_ast,
"You can't consume an expression that isn't local. More"
" specifically, you can only consume a local variable (a"
" single lowercase identifier, with no dots) or a field"
" of this (this followed by a dot and a single lowercase"
" identifier).");
return stringtab("");
}

pony_assert(0);
}
}
Expand Down Expand Up @@ -181,7 +195,7 @@ static const char* generate_multi_dot_name(ast_t* ast, ast_t** def_found) {
return stringtab_consume(buf, len);
}

static bool is_matching_assign_lhs(ast_t* a, ast_t* b)
static bool is_matching_assign_lhs(pass_opt_t *opt, ast_t* a, ast_t* b)
{
// Has to be the left hand side of an assignment (the first child).
if(a == b)
Expand All @@ -191,8 +205,8 @@ static bool is_matching_assign_lhs(ast_t* a, ast_t* b)
if((ast_id(a) == TK_DOT) && (ast_id(b) == TK_DOT))
{
// get fully qualified string identifier without `this`
const char* a_name = generate_multi_dot_name(a, NULL);
const char* b_name = generate_multi_dot_name(b, NULL);
const char* a_name = generate_multi_dot_name(opt, a, NULL, false, NULL);
const char* b_name = generate_multi_dot_name(opt, b, NULL, false, NULL);

if(a_name == b_name)
return true;
Expand All @@ -201,7 +215,7 @@ static bool is_matching_assign_lhs(ast_t* a, ast_t* b)
return false;
}

static bool is_assigned_to(ast_t* ast, bool check_result_needed)
static bool is_assigned_to(pass_opt_t *opt, ast_t* ast, bool check_result_needed)
{
while(true)
{
Expand All @@ -211,7 +225,7 @@ static bool is_assigned_to(ast_t* ast, bool check_result_needed)
{
case TK_ASSIGN:
{
if(!is_matching_assign_lhs(ast_child(parent), ast))
if(!is_matching_assign_lhs(opt, ast_child(parent), ast))
return false;

if(!check_result_needed)
Expand Down Expand Up @@ -309,15 +323,15 @@ static bool valid_reference(pass_opt_t* opt, ast_t* ast, sym_status_t status)

case SYM_CONSUMED:
case SYM_CONSUMED_SAME_EXPR:
if(is_assigned_to(ast, true))
if(is_assigned_to(opt, ast, true))
return true;

ast_error(opt->check.errors, ast,
"can't use a consumed local or field in an expression");
return false;

case SYM_UNDEFINED:
if(is_assigned_to(ast, true))
if(is_assigned_to(opt, ast, true))
return true;

ast_error(opt->check.errors, ast,
Expand Down Expand Up @@ -638,7 +652,7 @@ static bool refer_multi_dot(pass_opt_t* opt, ast_t* ast)
AST_GET_CHILDREN(ast, left, right);

// get fully qualified string identifier without `this`
const char* name = generate_multi_dot_name(ast, NULL);
const char* name = generate_multi_dot_name(opt, ast, NULL, false, NULL);

// use this string to check status using `valid_reference` function.
sym_status_t status;
Expand Down Expand Up @@ -766,7 +780,7 @@ static lvalue_t assign_multi_dot(pass_opt_t* opt, ast_t* ast, bool need_value)
pony_assert(ast_id(ast) == TK_DOT);

// get fully qualified string identifier without `this`
const char* name = generate_multi_dot_name(ast, NULL);
const char* name = generate_multi_dot_name(opt, ast, NULL, false, NULL);

sym_status_t status;
ast_get(ast, name, &status);
Expand Down Expand Up @@ -1049,7 +1063,7 @@ static bool refer_assign(pass_opt_t* opt, ast_t* ast)
return true;
}

static bool ast_get_child(ast_t* ast, const char* name)
static bool ast_get_child(pass_opt_t *opt, ast_t* ast, const char* name)
{
const char* assign_name = NULL;

Expand All @@ -1064,7 +1078,7 @@ static bool ast_get_child(ast_t* ast, const char* name)
case TK_DOT:
{
// get fully qualified string identifier without `this`
assign_name = generate_multi_dot_name(ast, NULL);
assign_name = generate_multi_dot_name(opt, ast, NULL, false, NULL);
break;
}

Expand All @@ -1079,7 +1093,7 @@ static bool ast_get_child(ast_t* ast, const char* name)

while(child != NULL)
{
if(ast_get_child(child, name))
if(ast_get_child(opt, child, name))
return true;

child = ast_sibling(child);
Expand All @@ -1088,7 +1102,7 @@ static bool ast_get_child(ast_t* ast, const char* name)
return false;
}

static bool check_assigned_same_expression(ast_t* ast, const char* name,
static bool check_assigned_same_expression(pass_opt_t *opt, ast_t* ast, const char* name,
ast_t** ret_assign_ast)
{
ast_t* assign_ast = ast;
Expand All @@ -1101,7 +1115,7 @@ static bool check_assigned_same_expression(ast_t* ast, const char* name,
return false;

ast_t* assign_left = ast_child(assign_ast);
return ast_get_child(assign_left, name);
return ast_get_child(opt, assign_left, name);
}

static void set_flag_recursive(ast_t* outer, uint32_t flag)
Expand Down Expand Up @@ -1137,7 +1151,7 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast)
ast_t* id = ast_child(term);
name = ast_name(id);
ast_t* assign_ast = NULL;
if(check_assigned_same_expression(id, name, &assign_ast))
if(check_assigned_same_expression(opt, id, name, &assign_ast))
{
consumed_same_expr = true;
ast_setflag(assign_ast, AST_FLAG_CNSM_REASGN);
Expand All @@ -1157,6 +1171,7 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast)
AST_GET_CHILDREN(term, left, right);

ast_t* def = NULL;
bool has_consume_error = false;

if(ast_id(left) == TK_THIS)
{
Expand All @@ -1175,14 +1190,14 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast)
{
// get fully qualified string identifier without `this`
// and def of the root object
name = generate_multi_dot_name(term, &def);
name = generate_multi_dot_name(opt, term, &def, true, &has_consume_error);

// defer checking it's not a let or embed if it's not a `this` variable
// because we don't have the type info available. The expr pass will
// catch it in the `expr_consume` function.
}

if(def == NULL)
if(def == NULL && !has_consume_error)
{
ast_error(opt->check.errors, ast,
"cannot consume an unknown field type");
Expand All @@ -1191,7 +1206,7 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast)

ast_t* assign_ast = NULL;

if(!check_assigned_same_expression(ast, name, &assign_ast))
if(!check_assigned_same_expression(opt, ast, name, &assign_ast))
{
ast_error(opt->check.errors, ast,
"consuming a field is only allowed if it is reassigned in the same"
Expand Down Expand Up @@ -1274,7 +1289,7 @@ static bool refer_local(pass_opt_t* opt, ast_t* ast)
if(ast_id(type) == TK_NONE)
{
// No type specified, infer it later
if(!is_dontcare && !is_assigned_to(ast, false))
if(!is_dontcare && !is_assigned_to(opt, ast, false))
{
ast_error(opt->check.errors, ast,
"locals must specify a type or be assigned a value");
Expand All @@ -1284,7 +1299,7 @@ static bool refer_local(pass_opt_t* opt, ast_t* ast)
else if(ast_id(ast) == TK_LET)
{
// Let, check we have a value assigned
if(!is_assigned_to(ast, false))
if(!is_assigned_to(opt, ast, false))
{
ast_error(opt->check.errors, ast,
"can't declare a let local without assigning to it");
Expand Down
33 changes: 33 additions & 0 deletions test/libponyc/refer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
{ const char* errs[] = {err1, err2, err3, NULL}; \
DO(test_expected_errors(src, "refer", errs)); }

#define TEST_ERRORS_4(src, err1, err2, err3, err4) \
{ const char* errs[] = {err1, err2, err3, err4, NULL}; \
DO(test_expected_errors(src, "refer", errs)); }


class ReferTest : public PassTest
{};
Expand Down Expand Up @@ -884,6 +888,35 @@ TEST_F(ReferTest, ConsumeAfterMemberAccessWithConsumeLhs)
"consume must take 'this', a local, or a parameter");
}

TEST_F(ReferTest, ConsumeInvalidExpression)
{
// From issue #4477
const char *src =
"struct FFIBytes\n"
"var ptr: Pointer[U8 val] = Pointer[U8].create()\n"
"var length: USize = 0\n"
"fun iso string(): String val =>\n"
"recover String.from_cpointer(consume FFIBytes.ptr,"
"consume FFIBytes.length) end\n"
"actor Main\n"
"new create(env: Env) =>\n"
"env.out.print(\"nothing to see here\")\n";

TEST_ERRORS_4(src,
"You can't consume an expression that isn't local. More specifically,"
" you can only consume a local variable (a single lowercase"
" identifier, with no dots) or a field of this (this followed by a dot"
" and a single lowercase identifier).",
"consuming a field is only allowed if it is reassigned in the same"
" expression",
"You can't consume an expression that isn't local. More specifically,"
" you can only consume a local variable (a single lowercase"
" identifier, with no dots) or a field of this (this followed by a dot"
" and a single lowercase identifier).",
"consuming a field is only allowed if it is reassigned in the same"
" expression");
}

TEST_F(ReferTest, MemberAccessWithConsumeLhs)
{
const char* src =
Expand Down
Loading