gh-124064: Fix some -Wconversion warnings #124174
Conversation
vstinner
commented
Sep 17, 2024
•
vstinner
commented
- Issue: Lots of new compiler warnings #124064
vstinner
requested review from
pablogsal,
lysnikolaou and
Fidget-Spinner
as code owners
| @@ -227,7 +227,7 @@ static inline void _PyGC_CLEAR_FINALIZED(PyObject *op) { | |||
| _PyObject_CLEAR_GC_BITS(op, _PyGC_BITS_FINALIZED); | |||
| #else | |||
| PyGC_Head *gc = _Py_AS_GC(op); | |||
| gc->_gc_prev &= ~_PyGC_PREV_MASK_FINALIZED; | |||
| gc->_gc_prev &= ~(uintptr_t)_PyGC_PREV_MASK_FINALIZED; | |||
There was a problem hiding this comment.
I think it might be better to make the _PyGC_PREV_MASK_FINALIZED literal unsigned.
There was a problem hiding this comment.
I tried #define _PyGC_PREV_MASK_FINALIZED 1U but the code got miscompiled and failed.
There was a problem hiding this comment.
Ugh... you're right. It won't get sign extended in that case.
There was a problem hiding this comment.
I wrote a different fix in the following change: #124177
-#define _PyGC_PREV_MASK_FINALIZED 1
+#define _PyGC_PREV_MASK_FINALIZED ((uintptr_t)1)It seems to work like that. I also prefer to fix the constant declaration rather than having to cast the constant every time.
There was a problem hiding this comment.
FYI, I started a discussion about -Wsign-conversion in the #gsoc-security-project Discord channel. It seems to me that adding these explicit casts in many places to address -Wsign-conversion does not make the code less bug prone and probably does the opposite.
In this case, we now have a tighter coupling between the _PyGC_PREV_MASK_FINALIZED and the type of _gc_prev and _gc_next. That seems worse to me than before where we relied on the implicit sign conversion.
For example, if we hypothetically changed _gc_prev to uint64_t, the code would now be incorrect.
| @@ -98,7 +98,7 @@ typedef union _PyStackRef { | |||
| static inline PyObject * | |||
| PyStackRef_AsPyObjectBorrow(_PyStackRef stackref) | |||
| { | |||
| PyObject *cleared = ((PyObject *)((stackref).bits & (~Py_TAG_BITS))); | |||
| PyObject *cleared = ((PyObject *)((stackref).bits & (~(uintptr_t)Py_TAG_BITS))); | |||
