Skip to content

Commit 3c58cc9

Browse files
JoeMattclaude
andcommitted
fix: rotate-by-32 UB in *_opcode_rorq + suppress padding noise
Round-2 sanitizer fix for PR #126: UBSAN at src/tom/gpu.c:1684 -- "shift exponent 32 too large for uint32_t". In gpu_opcode_rorq / dsp_opcode_rorq / DSP_rorq the rotation count comes from gpu_convert_zero[] / dsp_convert_zero[], which maps a 0 IMM_1 to 32 (rotate-by-0 means rotate-by-full-word, a no-op). But `RN >> 32` is UB regardless of what the post-shift result is, so the previous-commit fix (only masking the LHS) wasn't enough. Mask r1 to 0x1F before either shift -- maps 32 -> 0, preserving the no-op semantic. Three sites: - src/tom/gpu.c gpu_opcode_rorq - src/jerry/dsp.c dsp_opcode_rorq, DSP_rorq Also add `-clang-analyzer-optin.performance.Padding` to the disabled list in .clang-tidy -- it fires on inherited UAE/Virtual Jaguar struct layouts where reordering for tighter packing risks silent layout breaks in save-state / dlsym paths. Co-Authored-By: Claude Opus 4.7 <[email protected]>
1 parent cbc22bf commit 3c58cc9

3 files changed

Lines changed: 13 additions & 3 deletions

File tree

.clang-tidy

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,5 +66,8 @@ FormatStyle: none
6666
# locals that are read via macros (BCOMPEN, DSTA2, ...) clang-tidy can't see
6767
# the linkage for. Removing these inits would introduce real bugs.
6868
# - DeprecatedOrUnsafeBufferHandling: MSVC-flavored noise about strcpy/sprintf.
69+
# - optin.performance.Padding: upstream UAE/Virtual Jaguar struct layouts
70+
# inherited from 1990s emulator code; reordering for tighter packing risks
71+
# silent layout breaks in save-state / dlsym paths and isn't free perf.
6972
# - optin.portability.UnixAPI: false positive on libretro_core_options.h calloc.
7073
# - valist.Uninitialized: false-positive prone on our log macros.

src/jerry/dsp.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1417,7 +1417,10 @@ INLINE static void dsp_opcode_ror(void)
14171417

14181418
INLINE static void dsp_opcode_rorq(void)
14191419
{
1420-
uint32_t r1 = dsp_convert_zero[IMM_1 & 0x1F];
1420+
/* dsp_convert_zero[0] returns 32 (rotate-by-0 means rotate-by-full-word,
1421+
* a no-op). Masking to 0x1F maps 32 -> 0, preserving that semantic and
1422+
* avoiding `RN >> 32` UB in the rotate idiom below. */
1423+
uint32_t r1 = dsp_convert_zero[IMM_1 & 0x1F] & 0x1F;
14211424
uint32_t r2 = RN;
14221425
uint32_t res = (r2 >> r1) | (r2 << ((-r1) & 31));
14231426
RN = res;
@@ -2233,7 +2236,8 @@ INLINE static void DSP_ror(void)
22332236

22342237
INLINE static void DSP_rorq(void)
22352238
{
2236-
uint32_t r1 = dsp_convert_zero[PIMM1 & 0x1F];
2239+
/* See dsp_opcode_rorq above for why we mask to 0x1F. */
2240+
uint32_t r1 = dsp_convert_zero[PIMM1 & 0x1F] & 0x1F;
22372241
uint32_t r2 = PRN;
22382242
uint32_t res = (r2 >> r1) | (r2 << ((-r1) & 31));
22392243
PRES = res;

src/tom/gpu.c

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1679,7 +1679,10 @@ INLINE static void gpu_opcode_ror(void)
16791679

16801680
INLINE static void gpu_opcode_rorq(void)
16811681
{
1682-
uint32_t r1 = gpu_convert_zero[IMM_1 & 0x1F];
1682+
/* gpu_convert_zero[0] returns 32 (rotate-by-0 means rotate-by-full-word
1683+
* which is a no-op). Masking to 0x1F maps 32 -> 0, preserving that
1684+
* semantic and avoiding `RN >> 32` UB in the rotate idiom below. */
1685+
uint32_t r1 = gpu_convert_zero[IMM_1 & 0x1F] & 0x1F;
16831686
uint32_t r2 = RN;
16841687
uint32_t res = (r2 >> r1) | (r2 << ((-r1) & 31));
16851688
RN = res;

0 commit comments

Comments
 (0)