Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

csaavedra
Copy link

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.

See also https://bugs.webkit.org/show_bug.cgi?id=252309

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.

See also https://bugs.webkit.org/show_bug.cgi?id=252309
@athre0z
Copy link
Member

athre0z commented May 20, 2023

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 athre0z added C-bug Category: This is a bug (or a fix for a bug, when applied to PRs) P-low Priority: Low labels May 20, 2023
@athre0z
Copy link
Member

athre0z commented May 20, 2023

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug (or a fix for a bug, when applied to PRs) P-low Priority: Low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants