Skip to content

do not use global ctor/dtor for test params #1055

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

Merged

Conversation

bratpiorka
Copy link
Contributor

@bratpiorka bratpiorka commented Jan 23, 2025

Do not use global constructor/destructors for test params.

Description

Currently, all provider and pool tests use parameters created during the instantiation phase, with automatic destructors called upon program exit. The problem occurs when the creation of some parameters requires our global base allocator (in the case for the Disjoint Pool in the C version - not merged yet). In this scenario, the global base allocator's free() function could be called after the deletion of the entire global base allocator, because we cannot guarantee the correct order of destructors. Additionally, in the failing scenario, the base allocator's debug checks will report memory leaks.

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly

@bratpiorka bratpiorka force-pushed the rrudnick_test_params_refactor branch 2 times, most recently from 9d3dc55 to c0861fc Compare January 23, 2025 10:12
@vinser52
Copy link
Contributor

Currently, all provider and pool tests use parameters created during the instantiation phase, with automatic destructors called upon program exit. The problem occurs when the creation of some parameters requires our global base allocator (in the case for the Disjoint Pool in the C version - not merged yet). In this scenario, the global base allocator's free() function could be called after the deletion of the entire global base allocator, because we cannot guarantee the correct order of destructors. Additionally, in the failing scenario, the base allocator's debug checks will report memory leaks.

Does it mean that in real use cases, the same problem exists? I mean if some shared library (e.g. UR adaptor) will create disjoint pool params in the library ctor than we have the same issue as described?

@bratpiorka bratpiorka force-pushed the rrudnick_test_params_refactor branch 3 times, most recently from e7b2a1b to ff008cf Compare January 23, 2025 13:04
@bratpiorka
Copy link
Contributor Author

Does it mean that in real use cases, the same problem exists? I mean if some shared library (e.g. UR adaptor) will create disjoint pool params in the library ctor than we have the same issue as described?

library ctor should be fine. The real problem is with global ctors/dtors like here: https://github.com/oneapi-src/unified-memory-framework/pull/1055/files#diff-0884623826c0b9a057c7517f627f3f2f96062c7b14ff570599a115e46a2dd526L246

What I see on Windows is that the base alloc is destroyed before the global object destructors are called and because of this we see mem leaks in base alloc

@bratpiorka bratpiorka force-pushed the rrudnick_test_params_refactor branch 3 times, most recently from 8726d2d to cbe36ee Compare January 24, 2025 08:10
@bratpiorka bratpiorka requested review from ldorau and PatKamin January 24, 2025 08:47
@bratpiorka bratpiorka marked this pull request as ready for review January 24, 2025 08:47
@bratpiorka bratpiorka requested a review from a team as a code owner January 24, 2025 08:47
Copy link
Contributor

@PatKamin PatKamin left a comment

Choose a reason for hiding this comment

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

Looks good

@bratpiorka bratpiorka force-pushed the rrudnick_test_params_refactor branch from cbe36ee to abf957f Compare January 24, 2025 14:24
@bratpiorka bratpiorka merged commit 42adf1e into oneapi-src:main Jan 24, 2025
78 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
Development

Successfully merging this pull request may close these issues.

4 participants