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

Define and use the CAMLthread_local macro for TLS variables #12811

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Dec 8, 2023

The current way of declaring Thread-Local Storage for variables, __thread, isn't available under MSVC.

To declare a variable in Thread-Local Storage (TLS):

  • GNU C compatible compilers have the __thread storage class keyword;
  • MSVC has the __declspec(thread) extended storage-class modifier;
  • C11 has the _Thread_local storage-class specifier, and the thread_local macro defined in the (optionally provided) <threads.h> header;
  • C23 has the thread_local storage-class specifier as a keyword, and removes _Thread_local;
  • C++11 has the thread_local storage-class specifier as a keyword;
  • In C11/C17, if implementations don't provide <threads.h>, they must define __STDC_NO_THREADS__. Neither MinGW nor MSVC provide the header or define the macro (non-compliant compilers…).

Adding #define __thread __declspec(thread) for MSVC wouldn't be standard-compliant, as identifiers beginning with an underscore are reserved.

Although it would be nice to use thread_local everywhere, if we use it in a public header we risk it may conflict with a previous declaration from a user. That's unlikely, because users not having a sane definition of thread_local as a macro would expose themselves to the same compatibility problems, and their code will start breaking in C23 when it becomes a keyword.
If we had private headers we could also have used thread_local internally, and have a compatibility definition for our public headers.

We take the disruptive approach of using standard keywords instead of compilers extensions, and the conservative approach of hiding them below the new CAMLthread_local macro.

In the caml/misc.h header, we assume that the file is being compiled at least in C11 or C++11. We already use the _Atomic keyword in public headers, introduced in C11. C++11 is also required for atomics.

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

LGTM. It makes a lot of sense to hide the actual mechanism behind a CAMLthread_local macro, and the proposed definition of the macro looks good to me. MSVC may need a special case later, but that's business as usual.

Changes Outdated Show resolved Hide resolved
shym added a commit to shym/ocaml that referenced this pull request Dec 15, 2023
shym added a commit to shym/ocaml that referenced this pull request Dec 15, 2023
shym added a commit to shym/ocaml that referenced this pull request Dec 18, 2023
shym added a commit to shym/ocaml that referenced this pull request Dec 18, 2023
@gasche gasche merged commit a4e1d3d into ocaml:trunk Dec 18, 2023
9 checks passed
@MisterDA MisterDA deleted the CAMLthread_local branch December 18, 2023 15:59
@dra27 dra27 mentioned this pull request Feb 1, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants