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

Fix checkStack() to perform the necessary checks. #194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sashanicolas
Copy link
Contributor

Fix test_stack_* expected stacks.

Fix test_stack_* expected stacks.
Copy link
Contributor

@hainest hainest left a comment

Choose a reason for hiding this comment

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

Overall, this is a substantial improvement on the old checks. My concern is that when we enable optimizations in the mutatee that inlining will destroy our assumed stack order. Will the subsequence approach still work for that in a meaningful way?

src/dyninst/dyninst_comp.C Show resolved Hide resolved
logerror("**Failed** test %d (%s)\n", test_num, test_name);
logerror(" Possibly duplicate (case of wrong recursion) in collected stack. ");
failed = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be good to also add a check that the PC and FP addresses in the computed stack are in strictly decreasing order? That could be done using std::is_sorted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PC does not follow strictly decreasing order, as we don't know where the functions will be placed in memory, neither the base address of different libraries. So kill() could have a higher address compared to any test_stack_mutatee() or test_stack_func*(), while it is usually the last frame in these tests. But the FP for sure.

@sashanicolas sashanicolas self-assigned this Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants