Skip to content

starter text for declspec description #10770

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

Closed
naughtont3 wants to merge 5 commits into from
Closed

starter text for declspec description #10770

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e59ac19
starter text for declspec description
naughtont3 Sep 6, 2022
fcd79bc
limit example to just ompi/opal/oshmem projects
naughtont3 Sep 6, 2022
ebb496c
Apply suggestions from code review
naughtont3 Sep 6, 2022
fba4495
fix format and a typo
naughtont3 Sep 6, 2022
dda0abb
more feedback and remove module_declspec
naughtont3 Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/developers/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ probably don't need to read this section.
building-open-mpi
terminology
source-code-tree-layout
source-code-declspec
frameworks
gnu-autotools
sphinx
Expand Down
30 changes: 30 additions & 0 deletions docs/developers/source-code-declspec.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
Source code: Symbol visibility
==============================

The ``*_DECLSPEC`` macros provide a method to annotate symbols to indicate
their intended visibility when compiling dynamically shared object files
(e.g., `libmpi.so`).
Copy link
Member

@jsquyres jsquyres Sep 7, 2022

Choose a reason for hiding this comment

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

Suggested change
(e.g., `libmpi.so`).
(e.g., ``libmpi.so``).


The macros are defined on a per project basis:

* Open MPI: ``OMPI_DECLSPEC``, ``OMPI_MODULE_DECLSPEC``
* Open PAL: ``OPAL_DECLSPEC``, ``OPAL_MODULE_DECLSPEC``
* OpenSHMEM: ``OSHMEM_DECLSPEC``, ``OSHMEM_MODULE_DECLSPEC``

The macros expand to the appropriate compiler and platform flags for marking
whether a symbol should be explicitly made public in the target project's
library namespace. This is related to the Windows `__declspec <https://docs.microsoft.com/en-us/cpp/cpp/declspec?view=msvc-170>`_.

The ``*_DECLSPEC`` attribute is used to declare that the developer wants a
symbol to be usable outside of that library/DSO. For example,
Copy link
Member

@jsquyres jsquyres Sep 7, 2022

Choose a reason for hiding this comment

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

Suggested change
The ``*_DECLSPEC`` attribute is used to declare that the developer wants a
symbol to be usable outside of that library/DSO. For example,
The ``*_DECLSPEC`` attributes are used to declare that a
symbol is to be visible outside of that library/DSO's scope. For example,

``OMPI_DECLSPEC`` is used to control what symbols are visible in the
``libmpi.so`` scope.

.. note:: This is entirely related to dynamic library compilation and does not
apply to static compilation.

There are two variants of the macro:

* ``*_DECLSPEC``: controls visibility on individual functions/data structures
* ``*_MODULE_DECLSPEC``: controls visibility of MCA module (component) structure
Copy link
Member

@jsquyres jsquyres Sep 7, 2022

Choose a reason for hiding this comment

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

Looking at the code today, I wonder if the MODULE_DECLSPEC is left over from when we supported Windows. Today, DECLSPEC and MODULE_DECLSPEC resolve to the same back-end compiler attribute. I have a super-dim recollection that they might have been different on Windows, and we therefore needed 2 different macros.

But since we've long-since stopped supporting Windows, should we ditch the MODULE_DECLSPEC macro?

Copy link
Member

@jjhursey jjhursey Sep 7, 2022

Choose a reason for hiding this comment

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

👍 for removing MODULE_DECLSPEC if it is redundant.

Copy link
Member

@jsquyres jsquyres Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just checked Open MPI v1.0.0, and I confirm: in Windows, there was a difference between exporting the component symbol and exporting other symbols.

Given that we don't support native Windows builds any more, I think we should remove the distinction.

@naughtont3 I can make a PR for the code change if you want to update the text.

Copy link
Member

@jsquyres jsquyres Sep 7, 2022

Choose a reason for hiding this comment

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

PR to remove MODULE_DECLSPEC: #10774


Copy link
Member

@jsquyres jsquyres Sep 7, 2022

Choose a reason for hiding this comment

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

Nit: extra blank line at the end of the file.