Skip to content

fix(none.h): remove implementation-defined signed shifts in clip helpers (#211)#321

Open
deekshaNVIDIA wants to merge 1 commit into
ARM-software:mainfrom
deekshaNVIDIA:fix/issue-211-signed-shift-ub
Open

fix(none.h): remove implementation-defined signed shifts in clip helpers (#211)#321
deekshaNVIDIA wants to merge 1 commit into
ARM-software:mainfrom
deekshaNVIDIA:fix/issue-211-signed-shift-ub

Conversation

@deekshaNVIDIA

Copy link
Copy Markdown

Fixes #211

Problem

The four software-fallback clip helpers in Include/dsp/none.h
(clip_q63_to_q31, clip_q63_to_q15, clip_q31_to_q7, clip_q31_to_q15)
detect saturation by right-shifting a signed, possibly negative value, e.g.:

return ((q31_t) (x >> 32) != ((q31_t) x >> 31)) ?
  ((0x7FFFFFFF ^ ((q31_t) (x >> 63)))) : (q31_t) x;

Per C99 §6.5.7/5, right-shifting a negative signed integer yields an
implementation-defined result. These are exactly the lines flagged in #211
and by static analysers (e.g. MISRA).

Fix

Replace the shift-based detection with equivalent, fully-defined range checks
using the existing Q31_MAX/Q31_MIN/Q15_MAX/Q15_MIN/Q7_MAX/Q7_MIN
macros from arm_math_types.h. No signed right shifts remain; the only shift
left is an unsigned one in clip_q63_to_q15.

Behaviour is preserved exactly (this is a portability/UB cleanup, not a
functional change). Two quirks of the originals are deliberately kept verbatim
so nothing changes for existing callers:

  • clip_q31_to_q7 saturates outside the signed 24-bit range (±2^23), not
    the Q7 range — preserved via the 0x007FFFFF / 0x00800000 thresholds.
  • clip_q63_to_q15 returns bits [30:15] of the in-range value — preserved via
    (q15_t)((uint64_t)x >> 15), whose low 16 bits are identical to the old
    signed shift.

(If either quirk is actually an unintended bug, I'm happy to address it in a
separate PR — I kept this one strictly behaviour-preserving.)

Verification

  • Bit-exact equivalence, real compiler (clang 22, C11): the verbatim
    originals vs. the rewrites over

    • all 2^32 int32 inputs for clip_q31_to_q7 and clip_q31_to_q15, and
    • 2×10^8 structured/random 64-bit inputs for clip_q63_to_q31 and
      clip_q63_to_q15.

    Result: 0 mismatches.

  • The edited none.h compiles cleanly (no-DSP host config,
    -Wall -Wextra -Wconversion -Wsign-conversion -Wshadow, no warnings).

  • Only clip_q63_to_q31 and clip_q31_to_q15 are called inside the library
    today (e.g. arm_recip_q31, arm_div_int64_to_int32, and several Q31 Source
    files); both are among the exhaustively/structurally verified functions.

Happy to add a dedicated unit test for these helpers if you'd like.

The clip_q63_to_q31/clip_q63_to_q15/clip_q31_to_q7/clip_q31_to_q15
software fallbacks detected saturation by right-shifting a signed,
possibly-negative value. Per C99 6.5.7/5 the result of right-shifting a
negative signed integer is implementation-defined, which static analysers
(e.g. MISRA) flag.

Replace the shift-based logic with equivalent, fully-defined range checks.
Behaviour is preserved bit-for-bit: verified by an exhaustive sweep over all
2^32 int32 inputs (q31->q7/q15) and 2x10^8 structured/random 64-bit inputs
(q63->q31/q15), 0 mismatches. The clip_q31_to_q7 threshold (signed 24-bit
range, not the Q7 range) and the clip_q63_to_q15 bit selection are kept
exactly as before.

Fixes ARM-software#211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix undefined behavior in shifting

1 participant