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

gh-120593: Fix const qualifier in pyatomic.h #121052

Closed
wants to merge 1 commit into from
Closed

gh-120593: Fix const qualifier in pyatomic.h #121052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions Include/cpython/pyatomic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ static inline Py_ssize_t
_Py_atomic_load_ssize(const Py_ssize_t *obj);

static inline void *
_Py_atomic_load_ptr(const void *obj);
_Py_atomic_load_ptr(void *obj);


// --- _Py_atomic_load_relaxed -----------------------------------------------
Expand Down Expand Up @@ -358,7 +358,7 @@ static inline Py_ssize_t
_Py_atomic_load_ssize_relaxed(const Py_ssize_t *obj);

static inline void *
_Py_atomic_load_ptr_relaxed(const void *obj);
_Py_atomic_load_ptr_relaxed(void *obj);

static inline unsigned long long
_Py_atomic_load_ullong_relaxed(const unsigned long long *obj);
Expand Down Expand Up @@ -463,7 +463,7 @@ _Py_atomic_store_ullong_relaxed(unsigned long long *obj,

// Loads `*obj` (acquire operation)
static inline void *
_Py_atomic_load_ptr_acquire(const void *obj);
_Py_atomic_load_ptr_acquire(void *obj);

static inline uintptr_t
_Py_atomic_load_uintptr_acquire(const uintptr_t *obj);
Expand Down
10 changes: 5 additions & 5 deletions Include/cpython/pyatomic_gcc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ _Py_atomic_load_ssize(const Py_ssize_t *obj)
{ return __atomic_load_n(obj, __ATOMIC_SEQ_CST); }

static inline void *
_Py_atomic_load_ptr(const void *obj)
_Py_atomic_load_ptr(void *obj)
{ return (void *)__atomic_load_n((void **)obj, __ATOMIC_SEQ_CST); }


Expand Down Expand Up @@ -355,8 +355,8 @@ _Py_atomic_load_ssize_relaxed(const Py_ssize_t *obj)
{ return __atomic_load_n(obj, __ATOMIC_RELAXED); }

static inline void *
_Py_atomic_load_ptr_relaxed(const void *obj)
{ return (void *)__atomic_load_n((const void **)obj, __ATOMIC_RELAXED); }
_Py_atomic_load_ptr_relaxed(void *obj)
{ return (void *)__atomic_load_n((void **)obj, __ATOMIC_RELAXED); }

static inline unsigned long long
_Py_atomic_load_ullong_relaxed(const unsigned long long *obj)
Expand Down Expand Up @@ -489,12 +489,12 @@ _Py_atomic_store_ullong_relaxed(unsigned long long *obj,
// --- _Py_atomic_load_ptr_acquire / _Py_atomic_store_ptr_release ------------

static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
_Py_atomic_load_ptr_acquire(void *obj)
{ return (void *)__atomic_load_n((void **)obj, __ATOMIC_ACQUIRE); }

static inline uintptr_t
_Py_atomic_load_uintptr_acquire(const uintptr_t *obj)
{ return (uintptr_t)__atomic_load_n((uintptr_t *)obj, __ATOMIC_ACQUIRE); }
{ return (uintptr_t)__atomic_load_n(obj, __ATOMIC_ACQUIRE); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change. Shouldn't you rather remove the const from the signature?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change, the compiler emits a warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but isn't it because the object being passed to _Py_atomic_load_uintptr_acquire is a const uintptr_t *?

Now that I look a bit around, all the signatures take a const T * but I'm not sure if it's better to remove the inner cast or to fix the cast itself.

For instance,

static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
{ return (void *)__atomic_load_n((void **)obj, __ATOMIC_ACQUIRE); }

could be fixed as

static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
{ return (void *)__atomic_load_n((void * const*)obj, __ATOMIC_ACQUIRE); }

instead of changing the signature from const void * to void * (also, the cast on the return type should not needed I think). What do you think of that? at least, we would keep the same logic for signatures.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but isn't it because the object being passed to _Py_atomic_load_uintptr_acquire is a const uintptr_t *?

The compiler warns if you cast from "const TYPE" to "TYPE". My change removes the cast. It just works.

instead of changing the signature from const void * to void *

void* is special. I gave up trying to fix the warning, so I changed the parameters type of the 3 "pointer" functions instead.

Did you try your proposed fix? When I tried, it didn't work for me.

Copy link
Contributor

@picnixz picnixz Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well yes, it worked for me actually.. that's why I suggested it. I changed this:

// L491 in pyatomic_gcc.h
static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
{ return (void *)__atomic_load_n((void **)obj, __ATOMIC_ACQUIRE); }

into

static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
{ return __atomic_load_n((void * const*)obj, __ATOMIC_ACQUIRE); }

and I did not have any warning for this specific function (there were more but not for this one) (I also tried with a cast on the return and the warning still did not appear).

My GCC version is 7.5.0 by the way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I tried const void * const *, I don't recall.

Anyway, I wrote a new PR instead: PR gh-121055.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I tried const void * const *,

It's highly possible because the first fix I suggested on the issue was actually with a return type of const void * and not void * (hence the const void * const *). So it's partly my fault!

Copy link
Member Author

@vstinner vstinner Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your nice advice, it worked well at the end 😉


static inline void
_Py_atomic_store_ptr_release(void *obj, void *value)
Expand Down
10 changes: 5 additions & 5 deletions Include/cpython/pyatomic_msc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,12 +595,12 @@ _Py_atomic_load_int64(const int64_t *obj)
}

static inline void*
_Py_atomic_load_ptr(const void *obj)
_Py_atomic_load_ptr(void *obj)
{
#if SIZEOF_VOID_P == 8
return (void*)_Py_atomic_load_uint64((const uint64_t *)obj);
return (void*)_Py_atomic_load_uint64((uint64_t *)obj);
#else
return (void*)_Py_atomic_load_uint32((const uint32_t *)obj);
return (void*)_Py_atomic_load_uint32((uint32_t *)obj);
#endif
}

Expand Down Expand Up @@ -707,7 +707,7 @@ _Py_atomic_load_ssize_relaxed(const Py_ssize_t *obj)
}

static inline void*
_Py_atomic_load_ptr_relaxed(const void *obj)
_Py_atomic_load_ptr_relaxed(void *obj)
{
return *(void * volatile *)obj;
}
Expand Down Expand Up @@ -903,7 +903,7 @@ _Py_atomic_store_ullong_relaxed(unsigned long long *obj,
// --- _Py_atomic_load_ptr_acquire / _Py_atomic_store_ptr_release ------------

static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
_Py_atomic_load_ptr_acquire(void *obj)
{
#if defined(_M_X64) || defined(_M_IX86)
return *(void * volatile *)obj;
Expand Down
12 changes: 6 additions & 6 deletions Include/cpython/pyatomic_std.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,10 @@ _Py_atomic_load_ssize(const Py_ssize_t *obj)
}

static inline void*
_Py_atomic_load_ptr(const void *obj)
_Py_atomic_load_ptr(void *obj)
{
_Py_USING_STD;
return atomic_load((const _Atomic(void*)*)obj);
return atomic_load((_Atomic(void*)*)obj);
}


Expand Down Expand Up @@ -612,10 +612,10 @@ _Py_atomic_load_ssize_relaxed(const Py_ssize_t *obj)
}

static inline void*
_Py_atomic_load_ptr_relaxed(const void *obj)
_Py_atomic_load_ptr_relaxed(void *obj)
{
_Py_USING_STD;
return atomic_load_explicit((const _Atomic(void*)*)obj,
return atomic_load_explicit((_Atomic(void*)*)obj,
memory_order_relaxed);
}

Expand Down Expand Up @@ -856,10 +856,10 @@ _Py_atomic_store_ullong_relaxed(unsigned long long *obj,
// --- _Py_atomic_load_ptr_acquire / _Py_atomic_store_ptr_release ------------

static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
_Py_atomic_load_ptr_acquire(void *obj)
{
_Py_USING_STD;
return atomic_load_explicit((const _Atomic(void*)*)obj,
return atomic_load_explicit((_Atomic(void*)*)obj,
memory_order_acquire);
}

Expand Down
Loading