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

22474: Fixes rare issues that can lead to inconsistent EvaluableNode flags, leading to crashes #337

Merged
merged 2 commits into from
Jan 7, 2025
Merged

22474: Fixes rare issues that can lead to inconsistent EvaluableNode flags, leading to crashes #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
257d385
22474: Fixes rare issues that can lead to inconsistent EvaluableNode …
howsohazard Jan 7, 2025
27d1f13
22474: Removes left-in preprocessor macros
howsohazard Jan 7, 2025
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
8 changes: 4 additions & 4 deletions src/Amalgam/evaluablenode/EvaluableNodeManagement.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ class EvaluableNodeReference
}

//when attached a child node, make sure that this node reflects the same properties
//if first_attachment is true, then it will not call SetNeedCycleCheck(true) unless the attached node has the flag
//if the attached is not unique. Note that this parameter should not be set to true
//if first_attachment_and_not_construction_stack_node is true, then it will not call SetNeedCycleCheck(true)
// unless the attached node also needs cycle check. Note that this parameter should not be set to true
//if the node can be accessed in any other way, such as the construction stack
void UpdatePropertiesBasedOnAttachedNode(EvaluableNodeReference &attached,
bool first_attachment = false)
bool first_attachment_and_not_construction_stack_node = false)
{
if(attached.value.nodeValue.code == nullptr)
return;
Expand All @@ -74,7 +74,7 @@ class EvaluableNodeReference
unique = false;

//first attachment doesn't need to automatically require a cycle check
if(first_attachment)
if(first_attachment_and_not_construction_stack_node)
{
if(attached.value.nodeValue.code->GetNeedCycleCheck())
value.nodeValue.code->SetNeedCycleCheck(true);
Expand Down
6 changes: 2 additions & 4 deletions src/Amalgam/interpreter/Interpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,15 +731,13 @@ EvaluableNodeReference Interpreter::RewriteByFunction(EvaluableNodeReference fun
{
PushNewConstructionContext(nullptr, cur_node, EvaluableNodeImmediateValueWithType(StringInternPool::NOT_A_STRING_ID), nullptr);

bool first_node = true;
for(auto &[e_id, e] : cur_node->GetMappedChildNodesReference())
{
SetTopCurrentIndexInConstructionStack(e_id);
SetTopCurrentValueInConstructionStack(e);
auto new_e = RewriteByFunction(function, e, original_node_to_new_node);

cur_node.UpdatePropertiesBasedOnAttachedNode(new_e, first_node);
first_node = false;
cur_node.UpdatePropertiesBasedOnAttachedNode(new_e);
e = new_e;
}
if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand All @@ -757,7 +755,7 @@ EvaluableNodeReference Interpreter::RewriteByFunction(EvaluableNodeReference fun
SetTopCurrentIndexInConstructionStack(static_cast<double>(i));
SetTopCurrentValueInConstructionStack(ocn[i]);
auto new_e = RewriteByFunction(function, ocn[i], original_node_to_new_node);
cur_node.UpdatePropertiesBasedOnAttachedNode(new_e, i == 0);
cur_node.UpdatePropertiesBasedOnAttachedNode(new_e);
ocn[i] = new_e;
}

Expand Down
4 changes: 3 additions & 1 deletion src/Amalgam/interpreter/InterpreterOpcodesBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,13 +1480,15 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_OPCODE_STACK(EvaluableNode
EvaluableNodeReference stack_top_holder(evaluableNodeManager->AllocNode(ENT_LIST), true);
auto &sth_ocn = stack_top_holder->GetOrderedChildNodesReference();
sth_ocn.reserve(opcodeStackNodes->size());
bool first_node = true;
for(auto iter = begin(*opcodeStackNodes); iter != end(*opcodeStackNodes); ++iter)
{
EvaluableNode *cur_node = *iter;
EvaluableNodeReference new_node(evaluableNodeManager->AllocNode(cur_node->GetType()), true);
new_node->CopyMetadataFrom(cur_node);
sth_ocn.push_back(new_node);
stack_top_holder.UpdatePropertiesBasedOnAttachedNode(new_node);
stack_top_holder.UpdatePropertiesBasedOnAttachedNode(new_node, first_node);
first_node = false;
}
return stack_top_holder;
}
Expand Down
6 changes: 2 additions & 4 deletions src/Amalgam/interpreter/InterpreterOpcodesDataTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_LIST(EvaluableNode *en, bo
auto value = InterpretNode(ocn[i]);
//add it to the list
new_list_ocn[i] = value;
new_list.UpdatePropertiesBasedOnAttachedNode(value, i == 0);
new_list.UpdatePropertiesBasedOnAttachedNode(value);
}

if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand Down Expand Up @@ -133,7 +133,6 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_ASSOC(EvaluableNode *en, b
//construction stack has a reference, so no KeepNodeReference isn't needed for anything referenced
PushNewConstructionContext(en, new_assoc, EvaluableNodeImmediateValueWithType(StringInternPool::NOT_A_STRING_ID), nullptr);

bool first_node = true;
for(auto &[cn_id, cn] : new_mcn)
{
SetTopCurrentIndexInConstructionStack(cn_id);
Expand All @@ -142,8 +141,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_ASSOC(EvaluableNode *en, b
EvaluableNodeReference element_result = InterpretNode(cn);

cn = element_result;
new_assoc.UpdatePropertiesBasedOnAttachedNode(element_result, first_node);
first_node = false;
new_assoc.UpdatePropertiesBasedOnAttachedNode(element_result);
}

if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand Down
2 changes: 1 addition & 1 deletion src/Amalgam/interpreter/InterpreterOpcodesListManipulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_RANGE(EvaluableNode *en, b

EvaluableNodeReference element_result = InterpretNode(function);
result_ocn[i] = element_result;
result.UpdatePropertiesBasedOnAttachedNode(element_result, i == 0);
result.UpdatePropertiesBasedOnAttachedNode(element_result);
}

if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand Down
10 changes: 4 additions & 6 deletions src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_MAP(EvaluableNode *en, boo

EvaluableNodeReference element_result = InterpretNode(function);
result_ocn[i] = element_result;
result.UpdatePropertiesBasedOnAttachedNode(element_result, i == 0);
result.UpdatePropertiesBasedOnAttachedNode(element_result);
}

if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand Down Expand Up @@ -160,7 +160,6 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_MAP(EvaluableNode *en, boo

PushNewConstructionContext(list, result, EvaluableNodeImmediateValueWithType(StringInternPool::NOT_A_STRING_ID), nullptr);

bool first_node = true;
for(auto &[result_id, result_node] : result_mcn)
{
SetTopCurrentIndexInConstructionStack(result_id);
Expand All @@ -174,8 +173,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_MAP(EvaluableNode *en, boo
//in order to keep the node properties to be updated below
EvaluableNodeReference element_result = InterpretNode(function);
result_node = element_result;
result.UpdatePropertiesBasedOnAttachedNode(element_result, first_node);
first_node = false;
result.UpdatePropertiesBasedOnAttachedNode(element_result);
}

if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand Down Expand Up @@ -255,7 +253,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_MAP(EvaluableNode *en, boo

EvaluableNodeReference element_result = InterpretNode(function);
result->GetOrderedChildNodes()[index] = element_result;
result.UpdatePropertiesBasedOnAttachedNode(element_result, index == 0);
result.UpdatePropertiesBasedOnAttachedNode(element_result);
}

if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand Down Expand Up @@ -1639,7 +1637,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_ASSOCIATE(EvaluableNode *e

//handoff the reference from index_value to the assoc
new_assoc->SetMappedChildNodeWithReferenceHandoff(key_sid, value);
new_assoc.UpdatePropertiesBasedOnAttachedNode(value, i == 0);
new_assoc.UpdatePropertiesBasedOnAttachedNode(value);
}

if(PopConstructionContextAndGetExecutionSideEffectFlag())
Expand Down
Loading