stb_vorbis: 1404:75: warning: pointer comparison always evaluates to false [-Wtautological-compare]

[manxorist]

stb/stb_vorbis.c

Line 1404 in 5c20573

if (f->stream_start + loc >= f->stream_end || f->stream_start + loc < f->stream_start) {

stb_vorbis.c:1404:75: warning: pointer comparison always evaluates to false [-Wtautological-compare]

This is with Emscripten 4.0.3 (approximately Clang ~19).

As far as I understand it, both overflow check conditions are even Undefined Behaviour (1. pointer addition beyond 1-past-end of allocated buffer (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf 6.5.7 Additive operators p9), and comparison between pointers that are outside of the allocated object (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf 6.5.9 Relational operators p6), 2. pointer addition overflow (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf 6.5.7 Additive operators p9)).

Possible fixes:

1. if (loc >= (size_t)(f->stream_end - f->stream_start)) {, however stb_vorbis currently does neither include <stddef.h> nor currently use size_t at all. For my use cases that would not matter, but it might break other users. The result of pointer subtraction would be a signed type and without the cast to size_t this would result in a signed/unsigned comparison warning. size_t (or an unsigned variant of ptrdiff_t) cannot be named implicitly.
2. if (loc >= f->stream_len) {. As far as I can see, stream_len is set together with stream_start and stream_end in stb_vorbis_open_memory().

I can provide a patch for either solution 1 or 2, whatever is preferred.

There may be more sloppy/wrong pointer additions and comparisons in stb_vorbis. I did not review the whole code for that.

[nothings]

Solution 2 is preferred.

Please do not call these pieces of code "sloppy/wrong". The "Undefined Behavior" witch hunt is a ruinous effort by C compilers to destroy perfectly useful idioms that were just fine 30 years ago.

[manxorist]

Solution 2 is preferred.

Pull request submitted.

Please do not call these pieces of code "sloppy/wrong". The "Undefined Behavior" witch hunt is a ruinous effort by C compilers to destroy perfectly useful idioms that were just fine 30 years ago.

Well, they are wrong according to every C standard. The exact same wording was already there in C89 (>30 years ago).
It may be the case that compilers 30 year ago did not exploit these guarantees given even by the standard back then, however that would be really very compiler-specific. IMHO these idioms resulted in less readable/intuitive code than a standard-compliant implementation (just look at this example).

But then again, that is also just my opinion. I am happy to agree to disagree on this one. We probably will not convince each other :)

In any case, this issue should get fixed regardless of our opinions because Clang mis-compiles it in relation to developer intent (as shown by the warning it emits).