[BUG] Global static variable in inlined function.

[trivialfis]

Describe the bug

Hi, I'm trying to get rmm to work in XGBoost across Python and C++ code base. One issue I found is the global variable device_id_to_resource returned by rmm::mr::detail::get_map. The get_map function is inlined, so when XGBoost is compiled with:

    set_target_properties(${target} PROPERTIES
      C_VISIBILITY_PRESET hidden
      CXX_VISIBILITY_PRESET hidden
      CUDA_VISIBILITY_PRESET hidden
    )

different static variables are returned when called in C++ and called in Python. I tried to print out the address of returned map in set_per_device_resource (called in Python reinitialize) and get_per_device_resource(called during construction of device_uvector), and can confirm the returned maps have different addresses. This leads to user configuration not being honored in XGBoost c++ code base. Actually if I use gdb to break the function, the one called in Python is completely ignored.

[jrhemstad]

I looked into this extensively in the past. I even made a repro with my results: https://github.com/jrhemstad/link_test

I'm not familiar with the CXX_VISIBILITY_PRESET options, but I'm guessing that it counteracts the global symbol used for the function local static. This is also why RMM doesn't support Windows is that the function local statics won't be shared across dynamically linked libraries.

I would say RMM is just not compatible with those build settings.

[trivialfis]

Thanks for the reply. The build setting is used to handle c++ symbol conflicts. We ran into some errors that are caused by such conflicts, from both dmlc-core(a submodule used by xgboost and some other libraries) to CUDA libraries (where a corrupted stack gave me an invalid value error). So is there any chance rmm can support such build configuration in the future?

[trivialfis]

I also made a suggestion for cuml to adopt hidden symbols rapidsai/cuml#3859. There's an introduction on why it's useful: https://gcc.gnu.org/wiki/Visibility . (the pdf linked on that page might also be worth a read).

[jrhemstad]

The only way to support that build configuration is to make RMM no longer be a header-only library and distribute a binary. That would be a big change.

[jrhemstad]

I was reading through your link and found this:

With -fvisibility=hidden, you are telling GCC that every declaration not explicitly marked with a visibility attribute has a hidden visibility

So it sounds like we should be able to explicitly annotate the get_map symbol with __attribute__ ((visibility ("default"))) to make it explicitly shared.

[trivialfis]

So it sounds like we should be able to explicitly annotate the get_map symbol with attribute ((visibility ("default"))) to make it explicitly shared.

That sounds like a good idea, I will test it tomorrow to see if it solves the problem. ;-)

[robertmaynard]

we should be able to explicitly annotate the get_map symbol with attribute ((visibility ("default")))

Yes that will work. Explicit visibility attributes take priority over -fvisibility=hidden

If we go down this path for rmm we should consider having some form of ALWAYS_EXPORT, and NEVER_EXPORT macros that make it easier to apply these rules.

[trivialfis]

Just tested the solution by @jrhemstad and it works.

[trivialfis]

Hi, I can open a PR for this simple change if it's decided to be useful.

[jrhemstad]

Yeah, we'd be happy to have a PR for this.