-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Add STATIC_AVOID_DESTRUCTION for ObjectLibrary/Registry #9464
Conversation
@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
utilities/object_registry.cc
Outdated
// statically destroyed and long-lived. | ||
STATIC_AVOID_DESTRUCTION(ObjectRegistry, registry)(ObjectLibrary::Default()); | ||
STATIC_AVOID_DESTRUCTION(std::shared_ptr<ObjectRegistry>, instance) | ||
(®istry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see in CircleCI, this does not pass ASAN, because it's not a valid way to construct a destructible shared_ptr.
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work, thanks
@@ -278,6 +278,9 @@ class ObjectRegistry { | |||
static std::shared_ptr<ObjectRegistry> Default(); | |||
explicit ObjectRegistry(const std::shared_ptr<ObjectRegistry>& parent) | |||
: parent_(parent) {} | |||
explicit ObjectRegistry(const std::shared_ptr<ObjectLibrary>& library) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: when the point of the function/ctor is to save a copy of a shared_ptr, I think it's better to pass by shared_ptr value
@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
2 similar comments
@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This change should guarantee that the default ObjectLibrary/Registry are long-lived and not destroyed while the process is running. This will prevent some issues of them being referenced after they were destroyed via the static destruction.