Skip to content

Add supprt for new c23 deallocators #817

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

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

Conversation

claeusdev
Copy link

closes #725

This commit introduces 2 new functions to add support for free_sized and free_aligned_sized deallocators introduced in c23.

Tests ensure that free_sized and free_aligned_sized are skipped if unavailable otherwise we get a FREE record recorded for them.

This commit introduces 2 new functions to add support for
`free_sized` and `free_aligned_sized` deallocators introduced in
c23.

We ensure that `free_sized` and `free_aligned_sized` are skipped if
unavailable otherwise we get a `FREE` record recorded for them.

Signed-off-by: Nana Adjei Manu <n.k.a.manu06@gmail.com>
Copy link
Contributor

@godlygeek godlygeek 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 like a good start, but I see a lot of things here that don't look right.

Comment on lines +34 to +35
FOR_EACH_HOOKED_FUNCTION(free_sized) \
FOR_EACH_HOOKED_FUNCTION(free_aligned_sized)
Copy link
Contributor

Choose a reason for hiding this comment

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

These functions aren't platform specific, this should go into the #define MEMRAY_HOOKED_FUNCTIONS define below, not into MEMRAY_PLATFORM_HOOKED_FUNCTIONS. Also, you missed adding a \ to the previous line... What you've got here shouldn't compile...

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're trying to test a C23 feature, we shouldn't be using a C++ source file. We need a .c file for this test, not a .cpp file

Comment on lines +402 to +407
subprocess.run(
[sys.executable, str(extension_path / "setup.py"), "build_ext", "--inplace"],
check=True,
cwd=extension_path,
capture_output=True,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably also need to check that the compiler is able to build C23 stuff. If we get a compilation error here, we probably need to tell pytest to skip the test instead of failing it.

for record in records
if record.address in mallocs_addr and record.allocator == AllocatorType.FREE
]
assert len(frees) >= 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't we want to check that len(frees) == len(mallocs)? That is, that every allocation was freed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, what we probably want to do is have the functions in the extension module return the addresses that were allocated, so that the tests can ensure that a deallocation was seen for each of those addresses.

assert records

# Check that at least 2 allocations from malloc and aligned_alloc
mallocs = [record for record in records if record.allocator == AllocatorType.MALLOC]
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't find the allocation that was freed with free_aligned_sized - that allocation should have AllocatorType.ALIGNED_ALLOC instead.

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.

Intercept new free_sized and free_aligned_sized functions
2 participants