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

Inline functions that return static references must have default visibility #1653

Merged
merged 7 commits into from
Sep 6, 2024
Merged

Inline functions that return static references must have default visibility #1653

merged 7 commits into from
Sep 6, 2024

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Aug 20, 2024

Description

In #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.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence-
Inline functions that return static references must have default visi…
d76d30d 
…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
@wence- wence- added bug Something isn't working non-breaking Non-breaking change labels Aug 20, 2024
@wence- wence- requested a review from a team as a code owner August 20, 2024 15:40
@wence- wence- requested review from harrism and jrhemstad August 20, 2024 15:40
@github-actions github-actions bot added the cpp Pertains to C++ code label Aug 20, 2024
@wence-
Copy link
Contributor Author

wence- commented Aug 20, 2024 •
edited
Loading

Technically this is "breaking", since it changes symbol visibility. But I think it's ok to be non-breaking. Set as breaking per discussion below.

@wence-
Copy link
Contributor Author

wence- commented Aug 20, 2024

doc build fails because breathe doesn't know how to deal with the definition RMM_EXPORT inline spdlog::logger& logger. I don't know enough doxygen to figure out how to elide the RMM_EXPORT

@harrism
Copy link
Member

harrism commented Aug 21, 2024

Why shouldn't we mark it as breaking? All that does is put it under the "breaking changes" section of the changelog. I think for future reference it should be labeled "breaking". Do you agree?

@harrism
Copy link
Member

harrism commented Aug 21, 2024

Hmm, looks like breathe doesn't like RMM_EXPORT:

/__w/rmm/rmm/python/rmm/docs/librmm_docs/logging.rst:4: WARNING: Error when parsing function declaration.
If the function has no return type:
  Error in declarator or parameters-and-qualifiers
  Invalid C++ declaration: Expecting "(" in parameters-and-qualifiers. [error at 18]

    inline RMM_EXPORT spdlog::logger & logger ()
    ------------------^
If the function has a return type:
  Error in declarator or parameters-and-qualifiers
  If pointer to member declarator:
    Invalid C++ declaration: Expected '::' in pointer to member (function). [error at 33]
      inline RMM_EXPORT spdlog::logger & logger ()
      ---------------------------------^
  If declarator-id:
    Invalid C++ declaration: Expecting "(" in parameters-and-qualifiers. [error at 33]
      inline RMM_EXPORT spdlog::logger & logger ()
      ---------------------------------^

@wence- wence- added breaking Breaking change and removed non-breaking Non-breaking change labels Aug 21, 2024
@wence-
Copy link
Contributor Author

wence- commented Aug 21, 2024

This will be subsumed by #1654, but it's probably safe to belt-and-braces it if we can figure out that docs issue.

@wence-
Copy link
Contributor Author

wence- commented Aug 21, 2024

Why shouldn't we mark it as breaking? All that does is put it under the "breaking changes" section of the changelog. I think for future reference it should be labeled "breaking". Do you agree?

Yes, done.

@wence- wence- mentioned this pull request Aug 21, 2024
Merged
3 tasks
wence- added 2 commits August 22, 2024 15:00
@wence-
Fix doc build for functions annotated with RMM_EXPORT
7d0669c 
We must tell Doxygen to expand this macro, and particularly expand
them to nothing at all, for breathe to know what to do.
@wence-
Merge branch 'branch-24.10' into wence/fix/1651
0e9e5d4
@wence-
Copy link
Contributor Author

wence- commented Aug 22, 2024

botched the merge, will look tomorrow

@vyasr
Copy link
Contributor

vyasr commented Aug 22, 2024

Can we close this (and #1651) now that #1654 is resolved?

@harrism
Copy link
Member

harrism commented Aug 22, 2024

We want these functions explicitly marked in case the namespace export ever changes. However my per device resource PR also has these changes...

wence- added 2 commits August 23, 2024 11:38
@wence-
Must define RMM_NAMESPACE in non-GNUC case
b21a9e6
@wence-
Cargo-cult a Doxygen setup that correctly expands macros
366d808 
It appears to be impossible to get doxygen to EXPAND_AS_DEFINED a
macro that is in a file that is excluded via EXCLUDE_PATTERNS (even if
it is explicitly listed in INPUT). If the file is only excluded via
EXCLUDE, then it is visible to the preprocessor, but that setting
overrides any INPUT (so explicitly listed files that are EXCLUDEd are
no longer documented).

Consequently, we must unfortunately just repeat the preprocessor
definitions for RMM_NAMESPACE, RMM_EXPORT, and RMM_HIDDEN.
wence-
wence- commented Aug 23, 2024
View reviewed changes
include/rmm/detail/export.hpp
@@ -24,4 +24,5 @@
#else
#define RMM_EXPORT
#define RMM_HIDDEN
#define RMM_NAMESPACE rmm
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary for anyone compiling with a compiler that doesn't advertise __GNUC__.

@wence-
Copy link
Contributor Author

wence- commented Aug 23, 2024

After lots of fighting, I got doxygen working right. Unfortunately we have to reproduce the definitions of the macros in the doxyfile since it turns out that it's impossible to make EXCLUDE_PATTERNS work with wanting to have the doxygen preprocessor actually find the detail files that are included.

@wence- wence- requested a review from harrism August 23, 2024 15:58
harrism
harrism approved these changes Aug 26, 2024
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Approve, with one suggestion.

include/rmm/mr/device/per_device_resource.hpp Outdated Show resolved Hide resolved
@wence-
Copy link
Contributor Author

wence- commented Sep 6, 2024

/merge

@rapids-bot rapids-bot bot merged commit 9864b51 into rapidsai:branch-24.10 Sep 6, 2024
49 checks passed
@wence- wence- deleted the wence/fix/1651 branch September 6, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism approved these changes

@jrhemstad jrhemstad Awaiting requested review from jrhemstad jrhemstad is a code owner automatically assigned from rapidsai/rmm-cpp-codeowners

Assignees
No one assigned
Labels
breaking Breaking change bug Something isn't working cpp Pertains to C++ code
Projects
RMM Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Not all global static references are __attribute__((visibility(default)))
3 participants
@wence- @harrism @vyasr