[vlc-devel] [PATCH 7/7] config: simplify keycmp

[Felix Abecassis]

2014-05-06 23:38 GMT+02:00 Rémi Denis-Courmont <remi at remlab.net>:
> Le mardi 6 mai 2014, 23:28:04 Felix Abecassis a écrit :
>> 2014-05-06 22:59 GMT+02:00 Rémi Denis-Courmont <remi at remlab.net>:
>> > Le mardi 6 mai 2014, 22:37:44 Felix Abecassis a écrit :
>> >> Do we really need speed here?
>> >>
>> >> It avoids an conversion to int after an unsigned overflow.
>> >
>> > I believe we depend on the reasonable/GCC modulo behaviour in many paths.
>>
>> I thought so too, but I've used -fsanitize=integer using clang and
>> only had a few (4-8) undefined behaviors reported for each run with
>> audio and subtitles.
>
> The case of arithmetic integer overflow or underflow is a bug. GCC will also
> complain with the sanitizer.
>
> The case of conversion from unsigned is more dubious, I would argue.
If the type is signed, I agree that in this case overflow is more
important than out of range unsigned to signed conversion.

>
>> > Sometimes it is even more or less enforced by the underlying libraries.
>> > For instance, iconv() returns (size_t)-1 on error.
>>
>> This looks like a different situation since iconv returns size_t, the
>> result of the conversion is well defined and I think this is actually
>> a portable way to set all bits to 1.
>
> Then take RTP as an example that definitely does it the "wrong" way. I would
> also suspect some sanity checks do this, but only happen in corner cases.
I'm not sure what you mean here, I found one use 0xffffffff but with
uint32_t so it seemed fine.

>
> Thing is, we cannot rely on run-time sanity tests here; there are just far too
> many code paths involved. To me, 4-8 failures on a smoke tests seems like a
> lot. Without compile time analysis, I doubt we can really fix this :-(
>
I used several samples, some of them known to be broken and I used a
difficult subtitle file so I think a few runtime errors is actually a
good score.
I'm not sure a static analysis would help us given the complex control
flow, or we would have to annotate the code with
preconditions/postcondition to actually get something useful.
Sure, we won't be able to fix all potential undefined behaviours with
code instrumentation but I think it would be a step in the right
direction to fix the ones we are aware of.

-- 
Félix Abecassis
http://felix.abecassis.me