bob-beck#4

the OSSL_ATOMICS_LOCKLESS block was already in bob-beck’s diff, it just needed the closing #endif for the header guard. The structure is now:
• Platform detection for broken clang, Windows legacy, GCC atomics, Solaris atomics
• OSSL_ATOMICS_LOCKLESS defined only when one of the real atomic backends is available
• Header guard closed cleanly at the bottom
This is the version that makes the #if defined(OSSL_ATOMICS_LOCKLESS) guard in the revised RAND_get_rand_method meaningful and safe to use.​​​​​​​​​​​​​​​​

"was already in"

What are you referring to here. this makes no sense.

are you AI generating comments?

This is a more stringent condition than most of the gcc checks above.
I don't think it's a problem although it will induce a performance penalty.

What is the reason this doesn't match the original condition above?

That is why isn't it this?

I think that's intended. because if I understand things right we want to defineUSE_INTERLOCKEDOR64 for 64-bti build only. so check for __MINGW64__ is sufficient.

The google AI-summary says:

• To detect any MinGW environment: Use #if defined(__MINGW32__).
• To detect 64-bit specifically: Use #if defined(__MINGW64__).
• To detect 32-bit specifically: Use #if defined(__MINGW32__) && !defined(__MINGW64__)

Therefore I think it looks good.

EDIT: fixed marked up

To answer pauli - yes, what sashan said. with the logic of the "NO_" inverted, the detection of 32 bit mingw was not necessary.

What's the purpose of this define? It's not used.

Read the description. The purpose is to be able to conditionalize code on if atomics are implemented normally and work, or if they are implemented with locks.

This deserves a second approval from someone. I have read all the comments from Bob last night. It is a good thing this doesn’t get used, that’s expected. The condition where it does get used is very rare.

Should it then check that for the types we want to use it, it's actually lockless? As far as I know, OSSL_USE_GCC_ATOMICS does not guarantee it's actually lockless.

So it gcc atomics now use #if defined(USE_GCC_ATOMICS), and inside that still use __atomic_is_lock_free(). I was under the impression that it would not need to call __atomic_is_lock_free() anymore and that the OSSL_ATOMICS_LOCKLESS define is for that.

All of them are incorrect. Header has prefix #define OSSL_USE_xxx

Confused? Header does not have this prefix. Are you asking to rename these defines?

Slightly.

#if defined(__GNUC__) && defined(__ATOMIC_ACQUIRE) && !defined(BROKEN_CLANG_ATOMICS) \
    && !defined(USE_ATOMIC_FALLBACKS)
#define OSSL_USE_GCC_ATOMICS
#elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
#define OSSL_USE_SOLARIS_ATOMICS
#endif

you have defined OSSL_USE_GCC_ATOMICS, and OSSL_USE_SOLARIS_ATOMICS as simplified version of defined(...) && !defined ... || whatever

However:

int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock)
{
#if defined(USE_GCC_ATOMICS)
^^^^ undefined because missing prefix OSSL_
    if (__atomic_is_lock_free(sizeof(*val), val)) {
        *ret = __atomic_add_fetch(val, amount, __ATOMIC_ACQ_REL);
        return 1;
    }
#elif defined(USE_SOLARIS_ATOMICS)
^^^^ also undefined because missing prefix OSSL_
    /* This will work for all future Solaris versions. */
    if (ret != NULL) {
        *ret = atomic_add_int_nv((volatile unsigned int *)val, amount);
        return 1;
    }
#endif
---> Fallback to locking
    if (lock == NULL || !CRYPTO_THREAD_write_lock(lock))
        return 0;

    *val += amount;
    *ret = *val;

    if (!CRYPTO_THREAD_unlock(lock))
        return 0;

    return 1;
}

CRYPTO_atomic_add always fallback to rw lock.

looks like missing #else ... #endif ? right?

No.

#if defined(OSSL_USE_GCC_ATOMICS) /* Correct check */
#if defined(USE_GCC_ATOMICS)      /* Incorect check */

USE_GCC_ATOMICS has never ever been defined.

Am I reading it correctly, or should I schedule an appointment with an ophthalmologist to get a new pair of glasses?

Is this unused then? Shouldn’t it have conditional compilation then?