Skip to content

[DeviceAsan] Serval bug fixes #2293

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 5, 2024
Merged

[DeviceAsan] Serval bug fixes #2293

merged 5 commits into from
Dec 5, 2024

Conversation

@yingcong-wu
Copy link
Contributor

@yingcong-wu yingcong-wu commented Nov 7, 2024 •
edited
Loading

  • Wrong Allocation Info is used in SanitizerInterceptor::releaseMemory(), leading to double release of the AllocInfo and not release the out-of-quarantine allocations.
  • Remove virtual memory mapping entry once we release them
  • We need to insertProgram() before registerProgram() if the API creates new program(in this case, urProgramLink[Exp]), or we cannot find this program and will trigger assertion.
  • API that produce new programs may fail, but in such case insertProgram() will not be called, leading to cannot find program in time of releaseProgram(). We should ignore them in releaseProgram() in such case.

intel/llvm PR: intel/llvm#16257

@yingcong-wu yingcong-wu requested a review from a team as a code owner November 7, 2024 05:39
@github-actions github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Nov 7, 2024
zhaomaosu
zhaomaosu approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@zhaomaosu zhaomaosu left a comment

Choose a reason for hiding this comment

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

lgtm

AllanZyne
AllanZyne reviewed Nov 8, 2024
View reviewed changes
AllanZyne
AllanZyne reviewed Nov 8, 2024
View reviewed changes
@yingcong-wu yingcong-wu marked this pull request as draft November 8, 2024 06:45
@yingcong-wu yingcong-wu marked this pull request as ready for review November 12, 2024 03:47
@yingcong-wu yingcong-wu marked this pull request as draft November 15, 2024 06:45
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Nov 15, 2024

Convert to draft to try to add another fix.

@yingcong-wu yingcong-wu marked this pull request as ready for review November 15, 2024 07:21
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Nov 15, 2024

Hi @zhaomaosu , please review the latest change.

zhaomaosu
zhaomaosu approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@zhaomaosu zhaomaosu 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 November 15, 2024 08:00
@yingcong-wu yingcong-wu marked this pull request as ready for review November 15, 2024 08:14
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Nov 21, 2024

Hi @pbalcer , would you please help review this PR? It has been updated since your last review. Thank you!

@yingcong-wu yingcong-wu marked this pull request as draft November 21, 2024 07:20
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Nov 21, 2024

Hi @pbalcer , we decide to hold on to this PR in order to fasktrack #2232 . I will update the PR after the refactor PR is merged and request you review it again. Sorry if this is bothering you.

@yingcong-wu yingcong-wu marked this pull request as ready for review November 25, 2024 03:24
@yingcong-wu yingcong-wu changed the title [DeviceAsan] Minor bug fixes [DeviceAsan] Serval bug fixes Nov 25, 2024
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Nov 25, 2024

CI failures seem not related to the PR change. Are they known failures? Or can we restart the CI?

@kbenzie kbenzie added the v0.11.x Include in the v0.11.x release label Nov 25, 2024
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Nov 26, 2024 •
edited
Loading

I pushed a fix commit an hour ago, but yet it does not appear on the PR page. I don't know if that would cause problems. I think Github is having some issues here.

Therefore please wait for @zhaomaosu 's approval.

@pbalcer
Copy link
Contributor

pbalcer commented Nov 26, 2024

I think Github is having some issues here.

Yeah, I've been seeing issues with GitHub as well.

@yingcong-wu yingcong-wu marked this pull request as draft November 27, 2024 02:12
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Nov 27, 2024

After team discussion, we decide not to insertProgram when build failure, and just ingore the un-inserted program in the urProgramRelease.
Convert to draft to rework some of the code.

@yingcong-wu yingcong-wu force-pushed the yc-PR/241107-misc-minor-fix branch from 5a10a41 to 093a95d Compare November 27, 2024 02:23
@yingcong-wu yingcong-wu marked this pull request as ready for review November 27, 2024 02:45
zhaomaosu
zhaomaosu approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@zhaomaosu zhaomaosu 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
Copy link
Contributor Author

yingcong-wu commented Dec 4, 2024

Hi @pbalcer , this PR affects lots of tests, could we merge this ASAP? Thanks.

@pbalcer
Copy link
Contributor

pbalcer commented Dec 4, 2024

Hi @pbalcer , this PR affects lots of tests, could we merge this ASAP? Thanks.

please create an intel/llvm PR.

@yingcong-wu yingcong-wu mentioned this pull request Dec 4, 2024
Merged
@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Dec 4, 2024

Hi @pbalcer , this PR affects lots of tests, could we merge this ASAP? Thanks.

please create an intel/llvm PR.

Done.

@pbalcer pbalcer added the ready to merge Added to PR's which are ready to merge label Dec 5, 2024
@martygrant martygrant merged commit 2739808 into oneapi-src:main Dec 5, 2024
73 checks passed
martygrant added a commit to yingcong-wu/llvm that referenced this pull request Dec 5, 2024
@martygrant
[UR] Update tag to 2739808 for oneapi-src/unified-runtime#2293.
62b384c
martygrant added a commit to intel/llvm that referenced this pull request Dec 6, 2024
@yingcong-wu @martygrant
[DeviceAsan] bugfixes for UR (#16257)
6347914 
UR PR: oneapi-src/unified-runtime#2293

---------

Co-authored-by: Martin Morrison-Grant <[email protected]>
@yingcong-wu yingcong-wu deleted the yc-PR/241107-misc-minor-fix branch January 3, 2025 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pbalcer pbalcer pbalcer approved these changes

+2 more reviewers

@AllanZyne AllanZyne AllanZyne approved these changes

@zhaomaosu zhaomaosu zhaomaosu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

loader Loader related feature/bug ready to merge Added to PR's which are ready to merge sanitizer Sanitizer layer issues/changes/specification v0.11.x Include in the v0.11.x release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@yingcong-wu @pbalcer @AllanZyne @zhaomaosu @kbenzie @martygrant