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

[BUG] Not all global static references are __attribute__((visibility(default))) #1651

Closed
wence- opened this issue Aug 20, 2024 · 0 comments · Fixed by #1653
Closed

[BUG] Not all global static references are __attribute__((visibility(default))) #1651

wence- opened this issue Aug 20, 2024 · 0 comments · Fixed by #1653
Assignees
Labels
bug Something isn't working

Comments

@wence-
Copy link
Contributor

wence- commented Aug 20, 2024

Describe the bug

In #826, it was noted that the std::map in per_device_resource.hpp is a global static reference returned from an inline function. This causes issues with the RMM-default visibility(hidden) attribute of functions in the RMM headers, since although when compiling within a single library, the linker will uniquify the references to the function, when RMM is included in two separate libraries, this does not occur.

As a consequence, with visibility(hidden), the static reference will be unique per library, but differ across libraries.

#833 fixed this for the particular case of rmm::mr::detail::get_map, by making that function RMM_EXPORT.

However, there are other static references returned from inline functions in that file that were not exported. These should also be RMM_EXPORT.

In particular rmm::mr::detail::map_lock returns a static reference to a mutex that protects accesses to the per-device-resource map. However, since it is visibility(hidden), we actually don't have a single map lock, and so we are actually not protecting concurrent access to the per-device-resource map from different libraries.

An example, printing the address of the map lock:

index a56a784a..d5afafe4 100644
--- a/include/rmm/mr/device/per_device_resource.hpp
+++ b/include/rmm/mr/device/per_device_resource.hpp
@@ -108,6 +108,7 @@ inline device_memory_resource* initial_resource()
 inline std::mutex& map_lock()
 {
   static std::mutex map_lock;
+  std::cout << "Map lock " << &map_lock << std::endl; 
   return map_lock;
 }

We see, when cudf is compiled and used from cudf python (the python bindings build a separate library that includes RMM):

import cudf
s = cudf.Series([1])

prints

Map lock 0x7355ec744e20
Map lock 0x7355bc40c400
@wence- wence- added bug Something isn't working ? - Needs Triage Need team to review and classify labels Aug 20, 2024
@wence- wence- self-assigned this Aug 20, 2024
@wence- wence- added 2 - In Progress Currently a work in progress and removed ? - Needs Triage Need team to review and classify labels Aug 20, 2024
wence- added a commit to wence-/rmm that referenced this issue Aug 20, 2024
…bility

In rapidsai#833, we gave `rmm::mr::detail::get_map` default visibility.
However, there are a number of other functions that return static
references that should also have this visibility so that the static
reference is unique across multiple DSOs.

- Closes rapidsai#1651
@harrism harrism moved this from Todo to Review in RMM Project Board Aug 20, 2024
@harrism harrism removed the 2 - In Progress Currently a work in progress label Aug 20, 2024
@rapids-bot rapids-bot bot closed this as completed in #1653 Sep 6, 2024
@rapids-bot rapids-bot bot closed this as completed in 9864b51 Sep 6, 2024
@github-project-automation github-project-automation bot moved this from Review to Done in RMM Project Board Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants