Fix -Wtype-limits warnings by removing redundant assertions

zyantific / zydis

Fast and lightweight x86/x86-64 disassembler and code generation library

https://zydis.re

MIT License

3.4k stars 434 forks source link

Fix -Wtype-limits warnings by removing redundant assertions #428

Open csaavedra opened 1 year ago

csaavedra commented 1 year ago

This was uncovered in the context of WebKit. These assertions are always true as the bitfield variables in question are too small to hold the values that the assertions are comparing against:

ZydisSetAttributes: The arrays are defined to be larger than the bitfield holding cpu_state, fpu_state, and xmm_state, so the comparison is always true.
ZydisSetEffectiveOperandWidth: operand_size_map is a 3-bit bitfield, and the arrays have a size 8, thus the comparison is always true.
ZydisGetOperandDefinitions: operand_reference is a 15-bit bitfield, it can't hold 0xFFFF (which needs 16 bits), so the comparison is always true.

athre0z commented 1 year ago

Thanks for opening this!

These asserts are mostly in place to guard against when the array sizes are changed at a later point, to remind us that the corresponding lookup table needs a fix-up. Arguably we should probably turn these into static asserts though (as mentioned in the WebKit thread).

As for the assert in src/SharedData.c, I'd be OK with getting rid of that one.

athre0z commented 1 year ago

Interestingly enough, I wasn't actually able to reproduce the warning. I tried

CFLAGS="-Werror=type-limits -Wall -Wpedantic" cmake -DZYAN_FORCE_ASSERTS=ON -DCMAKE_BUILD_TYPE=Debug ..

with gcc 11.x, gcc 12.2.0, gcc 13.x, clang 11.x and clang 16.x but none of them bothered with printing a warning or error.

csaavedra commented 1 year ago

Yeah, weirdly I only get this when I enable SCCACHE with WK. GCC alone is not enough. I am not sure what sorcery goes on in the build system when SCCACHE is enabled, but now that we disabled the warnings in WK I didn't run into this again.