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

[DeviceASAN] Fix quarantined memory and related info not release when context is released. #2505

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

yingcong-wu
Copy link
Contributor

@yingcong-wu yingcong-wu commented Dec 24, 2024

Changes:

  • Bugfix: Quarantined memory and related info not get released when context is released. This will cause problems for later context when getting a allocation that shared some address range.
  • Bugfix: with ASAN BUFFER, some pointers is saved as char* instead of void*, this would make the logger try to print string instead of address, which leads to SEGV since they are not really strings.
  • Refactor: Change Quarantine to use shared_ptr instead of the iterator to avoid invalid iterator.
  • Refactor: Change the assertion of wrong allocation info to a fatal runtime error as that is truly a vital problem.
  • Refactor: Extract a common function releaseAllocationNoCheck to help better do allocation release.
  • [DeviceMSAN]: sync some of the changes to msan.

Intel/llvm PR: intel/llvm#16467

@github-actions github-actions bot added the loader Loader related feature/bug label Dec 24, 2024
@yingcong-wu yingcong-wu changed the title Yc test/1223 context qanrantine [DeviceASAN] Fix quarantined memory and related info not release when context is released. Dec 24, 2024
@AllanZyne
Copy link
Contributor

Please sync these fix to msan as well.

@yingcong-wu
Copy link
Contributor Author

Please sync these fix to msan as well.

Sync some fixes to msan.

Comment on lines +924 to +929
if (IsFromQuarantined) {
getContext()->logger.info("Quarantine Free: {}",
(void *)AI->AllocBegin);
} else {
getContext()->logger.info("Free: {}", (void *)AI->AllocBegin);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel not comfortable that "IsFromQuarantined" is just for different logger, it'd be better to move logger before calling "releaseAllocationNoCheck".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is just a helper function to reduce duplicated codes. If we split that logic to caller, then we would have to repeat the same code in different places, which is quite again the purpose of the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean you can just move "getContext()->logger" before calling this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean you can just move "getContext()->logger" before calling this function.

Yes, that is what I mean. If we move that logging logic to caller to this help function, then we would have duplicated codes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor

@AllanZyne AllanZyne left a comment

Choose a reason for hiding this comment

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

LGTM

@yingcong-wu yingcong-wu marked this pull request as draft December 26, 2024 07:18
@yingcong-wu yingcong-wu marked this pull request as ready for review December 31, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loader Loader related feature/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants