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.

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

Introduce basic leak checking into the validation layer #318

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Introduce basic leak checking into the validation layer #318

merged 4 commits into from
Mar 21, 2023

Conversation

kswiecicki
Copy link
Contributor

@kswiecicki kswiecicki commented Mar 3, 2023 •
edited
Loading

Leak checking is performed by tracking the *Create/*Retain/*Release calls.
It can be controlled through UR_ENABLE_LEAK_CHECKING environmental variable.

Related issue: #144.
Depends on: #154.

@kswiecicki kswiecicki force-pushed the leak-checking branch 2 times, most recently from 9d585bf to 8247f24 Compare March 3, 2023 16:47
igchor
igchor reviewed Mar 3, 2023
View reviewed changes
@pbalcer
Copy link
Contributor

pbalcer commented Mar 6, 2023

#289

Two thoughts. Just printing pointers to leaked objects is not super useful, consider printing backtraces (see man 3 backtrace). I also would prefer for most of the logic to live in the non-generated files.

@kswiecicki kswiecicki force-pushed the leak-checking branch 3 times, most recently from b0b8668 to 7cad7bc Compare March 9, 2023 12:16
@kswiecicki
Copy link
Contributor Author

#289

Two thoughts. Just printing pointers to leaked objects is not super useful, consider printing backtraces (see man 3 backtrace). I also would prefer for most of the logic to live in the non-generated files.

I've added backtrace logging for the references retained after urTearDown().
Also, I've moved most of the leak-checking logic to a separate file.

@kswiecicki kswiecicki force-pushed the leak-checking branch 2 times, most recently from 6cdc250 to 9f89071 Compare March 9, 2023 15:17
igchor
igchor reviewed Mar 9, 2023
View reviewed changes
PatKamin
PatKamin reviewed Mar 10, 2023
View reviewed changes
@kswiecicki kswiecicki force-pushed the leak-checking branch 2 times, most recently from 7645970 to 98bcbf7 Compare March 14, 2023 16:13
@kswiecicki
Copy link
Contributor Author

kswiecicki commented Mar 14, 2023 •
edited
Loading

@pbalcer what do you think about linking the loader with dbghelp Windows lib?
Should I introduce a compile time variable like UR_ENABLE_VALIDATION_BACKTRACE and link the necessary sources and dbghelp lib only when it is set?

@kswiecicki kswiecicki force-pushed the leak-checking branch 2 times, most recently from dc1e1e2 to 8ebd7a8 Compare March 14, 2023 16:24
pbalcer
pbalcer reviewed Mar 15, 2023
View reviewed changes
source/loader/layers/validation/ur_leak_check.hpp Outdated
it->second.refCount++;
return std::make_pair(it->first, it->second.refCount);
} else {
counts.emplace(ptr, createRefInfo(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should differentiate between create/retain/release. It will allow you to find issues when retain is called on an object that wasn't created properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The origin of those calls should be visible in backtrace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but the way you have it, doing:

  retain(0xCAFE);
  retain(0xCAFE);
  release(0xCAFE);
  release(0xCAFE);

Would be a perfectly valid sequence of calls. That's why I think special-casing create here makes sense.

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've modified the leak-checking mechanism to differentiate between *Create/*Retain/*Release calls.

@pbalcer
Copy link
Contributor

pbalcer commented Mar 15, 2023

@pbalcer what do you think about linking the loader with dbghelp Windows lib? Should I introduce a compile time variable like UR_ENABLE_VALIDATION_BACKTRACE and link the necessary sources and dbghelp lib only when it is set?

According to docs, "DbgHelp.dll ships with all versions of Windows", so I don't see a problem with always linking it in.

@kswiecicki kswiecicki force-pushed the leak-checking branch 2 times, most recently from 547c707 to 6d74a62 Compare March 17, 2023 13:10
pbalcer
pbalcer reviewed Mar 20, 2023
View reviewed changes
source/loader/layers/validation/ur_leak_check.hpp Outdated
it->second.refCount++;
return std::make_pair(it->first, it->second.refCount);
} else {
counts.emplace(ptr, createRefInfo(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but the way you have it, doing:

  retain(0xCAFE);
  retain(0xCAFE);
  release(0xCAFE);
  release(0xCAFE);

Would be a perfectly valid sequence of calls. That's why I think special-casing create here makes sense.

@kswiecicki kswiecicki mentioned this pull request Mar 21, 2023
Open
PatKamin
PatKamin reviewed Mar 21, 2023
View reviewed changes
scripts/templates/valddi.cpp.mako Outdated
@@ -16,18 +17,24 @@ from templates import helper as th
* @file ${name}.cpp
*
*/
#include "${x}_validation_layer.hpp"
#include "ur_leak_check.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using the ${x} for a varying prefix?

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'd forgotten about it. Should be fine now.

@pbalcer pbalcer merged commit edbaaad into oneapi-src:main Mar 21, 2023
@pbalcer pbalcer mentioned this pull request May 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PatKamin PatKamin PatKamin left review comments

@pbalcer pbalcer pbalcer approved these changes

@igchor igchor igchor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kswiecicki @pbalcer @igchor @PatKamin