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

Rewrite array's size in struct, and argument of static_assert and __builtin_frame_address #293

Merged
merged 27 commits into from
Jul 17, 2024

Conversation

JonathanFoo0523
Copy link
Collaborator

@JonathanFoo0523 JonathanFoo0523 commented Jul 15, 2024

Enhances the support for rewriting the size expressions of
constant-sized arrays in struct fields. Additionally, rewrites the
arguments' expressions of static_assert() and
__builtin_frame_address(), which need to be compile-time constants,
while still allowing the constants that make up the expression— which
may also be referred to elsewhere— to be freely mutated.

Fixes #286 , Fixes #289 , Fixes #290

@JonathanFoo0523
Copy link
Collaborator Author

Failed on window because:

'__builtin_frame_address': identifier not found

The corresponding function for window is _AddressOfReturnAddress and program made use of directive to work on both system. Eg:

void foo() {
#if __GNUC__ || __has_builtin(__builtin_frame_address)
    return __builtin_frame_address(0);
#elif defined(_MSC_VER)
    return _AddressOfReturnAddress();
#endif
}

But this can't be used as test case, because different platform produce different dredd-mutated program.

@afd Any suggestion to solve this?

@afd
Copy link
Member

afd commented Jul 15, 2024

Please see build.sh, under .github/workflows. You'll see there that there are exclusions that remove certain test cases that are known not to work under Windows. I suggest you make this a Linux-only test case, and add it to that exclusion list.

It would be neat to have this tested with the Windows variant too, but it's not a priority so I wouldn't worry about it.

@JonathanFoo0523 JonathanFoo0523 requested a review from afd July 16, 2024 01:01
Copy link
Member

@afd afd left a comment

Choose a reason for hiding this comment

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

This looks good, but there's one functional change I am slightly unsure about and would like to re-review.

A couple of language nits to be fixed too.

src/libdredd/src/mutate_visitor.cc Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Outdated Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Outdated Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Outdated Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Outdated Show resolved Hide resolved
@JonathanFoo0523 JonathanFoo0523 requested a review from afd July 16, 2024 17:36
Copy link
Member

@afd afd left a comment

Choose a reason for hiding this comment

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

This is looking very good! Various nits, and one slightly more substantial change please.

src/libdredd/src/mutate_ast_consumer.cc Outdated Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Outdated Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Outdated Show resolved Hide resolved
src/libdredd/src/mutate_visitor.cc Show resolved Hide resolved
@afd
Copy link
Member

afd commented Jul 17, 2024

After #300 lands, can you rebase this over main and re-generate expectations for single file test (as they may have changed as a result of #220)?

Then let me know when ready for a re-review.

@afd afd merged commit 5f12ec3 into mc-imperial:main Jul 17, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants