Author nnorwitz
Recipients alexandre.vassalotti, christian.heimes, donmez, gregory.p.smith, gvanrossum, loewis, nnorwitz
Date 2008-01-28.02:17:33
SpamBayes Score 0.00117364
Marked as misclassified No
Message-id <1201486658.16.0.924505845459.issue1621@psf.upfronthosting.co.za>
In-reply-to
Content
I'm just starting to look at the patch.  The first one changes i to
unsigned in Modules/_csv.c.  Hopefully most of them are like this.  The
code is fine as it is.  There is no reliance on overflow AFAICT.  It's
just that the breaking condition from the loop is not in the for (...).
 I think this change is fine to avoid a warning.  Just pointing out that
in this one case, it's not a real problem.

Change to heapq doesn't seem needed.  I looked at the warning generated
from this and it's if (!n).  This seems to indicate the compiler thinks
that n could be negative.  That should not be possible.  It came from
PyList_GET_SIZE.  We had verified the object was already a list.  So
this value should be between 0 and PY_SSIZE_T_MAX.  We check for 0, so
it might be > 0.  After decrementing n, it should be between 0 and
PY_SSIZE_T_MAX-1.  Of course, the compiler can't know the value can't be
negative (or PY_SSIZE_T_MIN) which would cause an underflow.

Change to hotshot should avoid a cast, so it's slightly better with this
approach.  Although with the change to size_t, the cast in flush_data
can be removed (just after the fwrite).

I don't see the reason to need for the change in sre.c, but I'm pretty
sure there are other overflows.

audioop definitely looks needed.
cPickle looks necessary.
expat/xmlparse.c is interesting--not sure if it's really necessary.  In
practice this probably can't be reached.
gc can't really overflow given that NUM_GENERATIONS is 3 and not likely
to grow much more. :-)

I stopped looking at this point.  It looks like some of these are really
needed.  Others are not possible given other invariants (the compiler
can't know about).  I like the idea of silencing compiler warnings. 
However, I fear this may generate a different problem.  Namely
signed/unsigned mismatches.

What happens if you add this warning:  -Wsign-compare
I think we got rid of most of those before (probably not true as much
for Modules/*.c).  I think this introduces many more.  Would it be
possible to come up with a patch that doesn't introduce more warnings
w/-Wsign-compare?

One potential issue with this patch is that while the additions might
have guaranteed semantics, we might have other problems when doing:

   size_t value = PyXXX_Size();
   if (value < 0) ...

I'm hoping that if we can use both -Wstrict-overflow and
-Wsign-conversion, eliminate all warnings, resulting in better code. 
(You could also try building with g++.  The core used to work without
warnings.  The modules still had a ways to go.)

Also what is the current state?  What has been implemented and what else
needs to be done?  Perhaps we should make other bug report(s) to address
other tangents that were discussed in this thread.
History
Date User Action Args
2008-01-28 02:17:38nnorwitzsetspambayes_score: 0.00117364 -> 0.00117364
recipients: + nnorwitz, gvanrossum, loewis, gregory.p.smith, christian.heimes, alexandre.vassalotti, donmez
2008-01-28 02:17:38nnorwitzsetspambayes_score: 0.00117364 -> 0.00117364
messageid: <1201486658.16.0.924505845459.issue1621@psf.upfronthosting.co.za>
2008-01-28 02:17:36nnorwitzlinkissue1621 messages
2008-01-28 02:17:34nnorwitzcreate