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

threads_win: fix build error with VS2010 #24326

Closed
wants to merge 3 commits into from
Closed

threads_win: fix build error with VS2010 #24326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d75c161
quic_multistream_test: fix undefined symbol snprintf with VS2010
httpstorm May 4, 2024
13c5dcd
threads_win: fix build error with VS2010
httpstorm May 3, 2024
e8580f1
threads_win: fix build error with VS2010 x86
httpstorm May 3, 2024
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
18 changes: 18 additions & 0 deletions crypto/threads_none.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,24 @@ int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock)
return 1;
}

int CRYPTO_atomic_add64(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
*val += op;
*ret = *val;

return 1;
}

int CRYPTO_atomic_and(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
*val &= op;
*ret = *val;

return 1;
}

int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
Expand Down
52 changes: 52 additions & 0 deletions crypto/threads_pthread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,58 @@ int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock)
return 1;
}

int CRYPTO_atomic_add64(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
# if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
if (__atomic_is_lock_free(sizeof(*val), val)) {
*ret = __atomic_add_fetch(val, op, __ATOMIC_ACQ_REL);
return 1;
}
# elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
/* This will work for all future Solaris versions. */
if (ret != NULL) {
*ret = atomic_add_64_nv(val, op);
return 1;
}
# endif
if (lock == NULL || !CRYPTO_THREAD_write_lock(lock))
return 0;
*val += op;
*ret = *val;

if (!CRYPTO_THREAD_unlock(lock))
return 0;

return 1;
}

int CRYPTO_atomic_and(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
# if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
if (__atomic_is_lock_free(sizeof(*val), val)) {
*ret = __atomic_and_fetch(val, op, __ATOMIC_ACQ_REL);
return 1;
}
# elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
/* This will work for all future Solaris versions. */
if (ret != NULL) {
*ret = atomic_and_64_nv(val, op);
return 1;
}
# endif
if (lock == NULL || !CRYPTO_THREAD_write_lock(lock))
return 0;
*val &= op;
*ret = *val;

if (!CRYPTO_THREAD_unlock(lock))
return 0;

return 1;
}

int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
Expand Down
74 changes: 64 additions & 10 deletions crypto/threads_win.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ struct rcu_lock_st {
CRYPTO_CONDVAR *alloc_signal;
CRYPTO_MUTEX *prior_lock;
CRYPTO_CONDVAR *prior_signal;
CRYPTO_RWLOCK *rw_lock;
};

static struct rcu_qp *allocate_new_qp_group(struct rcu_lock_st *lock,
Expand Down Expand Up @@ -132,6 +133,7 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx)
return NULL;

new->ctx = ctx;
new->rw_lock = CRYPTO_THREAD_lock_new();
nhorman marked this conversation as resolved.
Show resolved Hide resolved
new->write_lock = ossl_crypto_mutex_new();
new->alloc_signal = ossl_crypto_condvar_new();
new->prior_signal = ossl_crypto_condvar_new();
Expand All @@ -143,7 +145,9 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx)
|| new->prior_signal == NULL
|| new->write_lock == NULL
|| new->alloc_lock == NULL
|| new->prior_lock == NULL) {
|| new->prior_lock == NULL
|| new->rw_lock == NULL) {
nhorman marked this conversation as resolved.
Show resolved Hide resolved
CRYPTO_THREAD_lock_free(new->rw_lock);
OPENSSL_free(new->qp_group);
ossl_crypto_condvar_free(&new->alloc_signal);
ossl_crypto_condvar_free(&new->prior_signal);
Expand All @@ -159,6 +163,7 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx)

void ossl_rcu_lock_free(CRYPTO_RCU_LOCK *lock)
{
CRYPTO_THREAD_lock_free(lock->rw_lock);
OPENSSL_free(lock->qp_group);
ossl_crypto_condvar_free(&lock->alloc_signal);
ossl_crypto_condvar_free(&lock->prior_signal);
Expand All @@ -168,17 +173,23 @@ void ossl_rcu_lock_free(CRYPTO_RCU_LOCK *lock)
OPENSSL_free(lock);
}

static inline struct rcu_qp *get_hold_current_qp(CRYPTO_RCU_LOCK *lock)
static ossl_inline struct rcu_qp *get_hold_current_qp(CRYPTO_RCU_LOCK *lock)
{
uint32_t qp_idx;
uint32_t tmp;
uint64_t tmp64;

/* get the current qp index */
for (;;) {
qp_idx = InterlockedOr(&lock->reader_idx, 0);
InterlockedAdd64(&lock->qp_group[qp_idx].users, VAL_READER);
if (qp_idx == InterlockedOr(&lock->reader_idx, 0))
CRYPTO_atomic_load_int(&lock->reader_idx, (int *)&qp_idx,
lock->rw_lock);
CRYPTO_atomic_add64(&lock->qp_group[qp_idx].users, VAL_READER, &tmp64,
lock->rw_lock);
CRYPTO_atomic_load_int(&lock->reader_idx, (int *)&tmp, lock->rw_lock);
if (qp_idx == tmp)
break;
InterlockedAdd64(&lock->qp_group[qp_idx].users, -VAL_READER);
CRYPTO_atomic_add64(&lock->qp_group[qp_idx].users, -VAL_READER, &tmp64,
lock->rw_lock);
}

return &lock->qp_group[qp_idx];
Expand Down Expand Up @@ -253,7 +264,9 @@ void ossl_rcu_read_unlock(CRYPTO_RCU_LOCK *lock)
if (data->thread_qps[i].lock == lock) {
data->thread_qps[i].depth--;
if (data->thread_qps[i].depth == 0) {
ret = InterlockedAdd64(&data->thread_qps[i].qp->users, -VAL_READER);
CRYPTO_atomic_add64(&data->thread_qps[i].qp->users,
-VAL_READER, (uint64_t *)&ret,
lock->rw_lock);
OPENSSL_assert(ret >= 0);
data->thread_qps[i].qp = NULL;
data->thread_qps[i].lock = NULL;
Expand All @@ -268,6 +281,7 @@ static struct rcu_qp *update_qp(CRYPTO_RCU_LOCK *lock)
uint64_t new_id;
uint32_t current_idx;
uint32_t tmp;
uint64_t tmp64;

ossl_crypto_mutex_lock(lock->alloc_lock);
/*
Expand All @@ -291,8 +305,10 @@ static struct rcu_qp *update_qp(CRYPTO_RCU_LOCK *lock)
lock->id_ctr++;

new_id = VAL_ID(new_id);
InterlockedAnd64(&lock->qp_group[current_idx].users, ID_MASK);
InterlockedAdd64(&lock->qp_group[current_idx].users, new_id);
CRYPTO_atomic_and(&lock->qp_group[current_idx].users, ID_MASK, &tmp64,
lock->rw_lock);
CRYPTO_atomic_add64(&lock->qp_group[current_idx].users, new_id, &tmp64,
lock->rw_lock);

/* update the reader index to be the prior qp */
tmp = lock->current_alloc_idx;
Expand Down Expand Up @@ -327,7 +343,7 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)

/* wait for the reader count to reach zero */
do {
count = InterlockedOr64(&qp->users, 0);
CRYPTO_atomic_load(&qp->users, &count, lock->rw_lock);
} while (READER_COUNT(count) != 0);

/* retire in order */
Expand Down Expand Up @@ -558,6 +574,44 @@ int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock)
return 1;
}

int CRYPTO_atomic_add64(uint64_t *val, uint64_t op, uint64_t *ret,
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to run make update to get this in the exported ordinals list, its why CI is failing here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I was wondering what this error means.
That ancient piece of history Windows 2003 is using nmake to compile, so I ran the command on macOS instead. I see the two new functions were added to util/libcrypto.num. I hope that's good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite. The CI test for symbol presence is failing, need to look at why.

also, if you're going to add new exported library symbols, the docs check is going to fail if you don't document them in the appropriate pod file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doc/man3/CRYPTO_THREAD_run_once.pod contains all related function, so I added the description there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Building on Windows works only with nmake. I do have GNU Make 4.4.1, but both fail to perform the update target. So my options are macOS 14.4.1 Intel, and Ubuntu 22.04.4 LTS.

nmake update
Microsoft (R) Program Maintenance Utility Version 10.00.40219.01
Copyright (C) Microsoft Corporation.  All rights reserved.

NMAKE : fatal error U1073: don't know how to make 'update'
Stop.

make update
makefile:78: *** missing separator.  Stop.

I also noticed that apart from threads_win.c, the atomic functions are also defined in threads_pthread.c and threads_none.c. Do I need to implement them there as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe I need to add them to ./test/threadstest.c?

Copy link
Contributor

Choose a reason for hiding this comment

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

you do need to add the definitions to threads_pthread.c and threads_none.c, as only one of the three (win/pthreads/none) gets compiled for the build and you need a definition for the exported symbols

As for why make update isn't working. I expect its got something to do with the version of nmake on vs2010. Its worked for me on VS2022 community edition. note VS2010 isn't an officially supported platform

I'd focus on getting the implementations defined on other platforms first, then worry about being able to make the update script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added definitions to threads_pthread.c and threads_none.c. Added tests to threadstest.c.

CRYPTO_RWLOCK *lock)
{
#if (defined(NO_INTERLOCKEDOR64))
if (lock == NULL || !CRYPTO_THREAD_write_lock(lock))
return 0;
*val += op;
*ret = *val;

if (!CRYPTO_THREAD_unlock(lock))
return 0;

return 1;
#else
*ret = (uint64_t)InterlockedAdd64((LONG64 volatile *)val, (LONG64)op);
return 1;
#endif
}

int CRYPTO_atomic_and(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
#if (defined(NO_INTERLOCKEDOR64))
if (lock == NULL || !CRYPTO_THREAD_write_lock(lock))
return 0;
*val &= op;
*ret = *val;

if (!CRYPTO_THREAD_unlock(lock))
return 0;

return 1;
#else
*ret = (uint64_t)InterlockedAnd64((LONG64 volatile *)val, (LONG64)op) & op;
return 1;
#endif
}

int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock)
{
Expand Down
27 changes: 25 additions & 2 deletions doc/man3/CRYPTO_THREAD_run_once.pod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
CRYPTO_THREAD_run_once,
CRYPTO_THREAD_lock_new, CRYPTO_THREAD_read_lock, CRYPTO_THREAD_write_lock,
CRYPTO_THREAD_unlock, CRYPTO_THREAD_lock_free,
CRYPTO_atomic_add, CRYPTO_atomic_or, CRYPTO_atomic_load, CRYPTO_atomic_store,
CRYPTO_atomic_load_int,
CRYPTO_atomic_add, CRYPTO_atomic_add64, CRYPTO_atomic_and, CRYPTO_atomic_or,
CRYPTO_atomic_load, CRYPTO_atomic_store, CRYPTO_atomic_load_int,
OSSL_set_max_threads, OSSL_get_max_threads,
OSSL_get_thread_support_flags, OSSL_THREAD_SUPPORT_FLAG_THREAD_POOL,
OSSL_THREAD_SUPPORT_FLAG_DEFAULT_SPAWN - OpenSSL thread support
Expand All @@ -25,6 +25,10 @@ OSSL_THREAD_SUPPORT_FLAG_DEFAULT_SPAWN - OpenSSL thread support
void CRYPTO_THREAD_lock_free(CRYPTO_RWLOCK *lock);

int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_add64(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_and(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock);
Expand Down Expand Up @@ -94,6 +98,25 @@ supported and I<lock> is NULL, then the function will fail.

=item *

CRYPTO_atomic_add64() atomically adds I<op> to I<*val> and returns the
result of the operation in I<*ret>. I<lock> will be locked, unless atomic
operations are supported on the specific platform. Because of this, if a
variable is modified by CRYPTO_atomic_add64() then CRYPTO_atomic_add64() must
be the only way that the variable is modified. If atomic operations are not
supported and I<lock> is NULL, then the function will fail.

=item *

CRYPTO_atomic_and() performs an atomic bitwise and of I<op> and I<*val> and stores
the result back in I<*val>. It also returns the result of the operation in
I<*ret>. I<lock> will be locked, unless atomic operations are supported on the
specific platform. Because of this, if a variable is modified by
CRYPTO_atomic_and() or read by CRYPTO_atomic_load() then CRYPTO_atomic_and() must
be the only way that the variable is modified. If atomic operations are not
supported and I<lock> is NULL, then the function will fail.

=item *

CRYPTO_atomic_or() performs an atomic bitwise or of I<op> and I<*val> and stores
the result back in I<*val>. It also returns the result of the operation in
I<*ret>. I<lock> will be locked, unless atomic operations are supported on the
Expand Down
4 changes: 4 additions & 0 deletions include/openssl/crypto.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ int CRYPTO_THREAD_unlock(CRYPTO_RWLOCK *lock);
void CRYPTO_THREAD_lock_free(CRYPTO_RWLOCK *lock);

int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_add64(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_and(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret,
CRYPTO_RWLOCK *lock);
int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock);
Expand Down
8 changes: 4 additions & 4 deletions test/quic_multistream_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ static int helper_init(struct helper *h, const char *script_name,
goto err;

/* Set title for qlog purposes. */
snprintf(title, sizeof(title), "quic_multistream_test: %s", script_name);
BIO_snprintf(title, sizeof(title), "quic_multistream_test: %s", script_name);
if (!TEST_true(ossl_quic_set_diag_title(h->c_ctx, title)))
goto err;

Expand Down Expand Up @@ -5827,7 +5827,7 @@ static int test_script(int idx)
}
#endif

snprintf(script_name, sizeof(script_name), "script %d", script_idx + 1);
BIO_snprintf(script_name, sizeof(script_name), "script %d", script_idx + 1);

TEST_info("Running script %d (order=%d, blocking=%d)", script_idx + 1,
free_order, blocking);
Expand Down Expand Up @@ -5912,8 +5912,8 @@ static ossl_unused int test_dyn_frame_types(int idx)
s[i].arg2 = forbidden_frame_types[idx].expected_err;
}

snprintf(script_name, sizeof(script_name),
"dyn script %d", idx);
BIO_snprintf(script_name, sizeof(script_name),
"dyn script %d", idx);

return run_script(dyn_frame_types_script, script_name, 0, 0);
}
Expand Down
46 changes: 46 additions & 0 deletions test/threadstest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,52 @@ static int test_atomic(void)
|| !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64))
goto err;

ret64 = 0;

if (CRYPTO_atomic_and(&val64, 5, &ret64, NULL)) {
/* This succeeds therefore we're on a platform with lockless atomics */
if (!TEST_uint_eq((unsigned int)val64, 1)
|| !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64))
goto err;
} else {
/* This failed therefore we're on a platform without lockless atomics */
if (!TEST_uint_eq((unsigned int)val64, 3)
|| !TEST_int_eq((unsigned int)ret64, 0))
goto err;
}
val64 = 3;
ret64 = 0;

if (!TEST_true(CRYPTO_atomic_and(&val64, 5, &ret64, lock)))
goto err;

if (!TEST_uint_eq((unsigned int)val64, 1)
|| !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64))
goto err;

ret64 = 0;

if (CRYPTO_atomic_add64(&val64, 2, &ret64, NULL)) {
/* This succeeds therefore we're on a platform with lockless atomics */
if (!TEST_uint_eq((unsigned int)val64, 3)
|| !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64))
goto err;
} else {
/* This failed therefore we're on a platform without lockless atomics */
if (!TEST_uint_eq((unsigned int)val64, 1)
|| !TEST_int_eq((unsigned int)ret64, 0))
goto err;
}
val64 = 1;
ret64 = 0;

if (!TEST_true(CRYPTO_atomic_add64(&val64, 2, &ret64, lock)))
goto err;

if (!TEST_uint_eq((unsigned int)val64, 3)
|| !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64))
goto err;

testresult = 1;
err:
CRYPTO_THREAD_lock_free(lock);
Expand Down
2 changes: 2 additions & 0 deletions util/libcrypto.num
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5646,3 +5646,5 @@ OSSL_IETF_ATTR_SYNTAX_print ? 3_4_0 EXIST::FUNCTION:
X509_ACERT_add_attr_nconf ? 3_4_0 EXIST::FUNCTION:
OSSL_LIB_CTX_get_conf_diagnostics ? 3_4_0 EXIST::FUNCTION:
OSSL_LIB_CTX_set_conf_diagnostics ? 3_4_0 EXIST::FUNCTION:
CRYPTO_atomic_add64 ? 3_4_0 EXIST::FUNCTION:
CRYPTO_atomic_and ? 3_4_0 EXIST::FUNCTION:
Loading