2024-04-10 19:14:58

by Muhammad Usama Anjum

[permalink] [raw]
Subject: Re: [PATCH] selftests: timers: Fix valid-adjtimex signed left-shift undefined behavior

On 4/10/24 1:22 AM, John Stultz wrote:
> So, the struct adjtimex freq field takes a signed value who's
> units are in shifted (<<16) parts-per-million.
>
> Unfortunately for negative adjustments, the straightforward use
> of:
> freq = ppm<<16
> will trip undefined behavior warnings with clang:
>
> valid-adjtimex.c:66:6: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
> -499<<16,
> ~~~~^
> valid-adjtimex.c:67:6: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
> -450<<16,
> ~~~~^
> ...
>
> So fix our use of shifting negative values in the valid-adjtimex
> test case to use multiply by (1<<16) to avoid this.
>
> The patch also aligns the values a bit to make it look nicer.
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Anna-Maria Behnsen <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: Shuah Khan <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Muhammad Usama Anjum <[email protected]>
> Cc: [email protected]
> Reported-by: Lee Jones <[email protected]>
> Reported-by: Muhammad Usama Anjum <[email protected]>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: John Stultz <[email protected]>
Reviewed-by: Muhammad Usama Anjum <[email protected]>

> ---
> .../testing/selftests/timers/valid-adjtimex.c | 69 ++++++++++---------
> 1 file changed, 35 insertions(+), 34 deletions(-)
>
> diff --git a/tools/testing/selftests/timers/valid-adjtimex.c b/tools/testing/selftests/timers/valid-adjtimex.c
> index 48b9a803235a..9606d45767e7 100644
> --- a/tools/testing/selftests/timers/valid-adjtimex.c
> +++ b/tools/testing/selftests/timers/valid-adjtimex.c
> @@ -62,45 +62,46 @@ int clear_time_state(void)
> #define NUM_FREQ_OUTOFRANGE 4
> #define NUM_FREQ_INVALID 2
>
> +#define SHIFTED_PPM (1 << 16)
> long valid_freq[NUM_FREQ_VALID] = {
> - -499<<16,
> - -450<<16,
> - -400<<16,
> - -350<<16,
> - -300<<16,
> - -250<<16,
> - -200<<16,
> - -150<<16,
> - -100<<16,
> - -75<<16,
> - -50<<16,
> - -25<<16,
> - -10<<16,
> - -5<<16,
> - -1<<16,
> + -499 * SHIFTED_PPM,
> + -450 * SHIFTED_PPM,
> + -400 * SHIFTED_PPM,
> + -350 * SHIFTED_PPM,
> + -300 * SHIFTED_PPM,
> + -250 * SHIFTED_PPM,
> + -200 * SHIFTED_PPM,
> + -150 * SHIFTED_PPM,
> + -100 * SHIFTED_PPM,
> + -75 * SHIFTED_PPM,
> + -50 * SHIFTED_PPM,
> + -25 * SHIFTED_PPM,
> + -10 * SHIFTED_PPM,
> + -5 * SHIFTED_PPM,
> + -1 * SHIFTED_PPM,
> -1000,
> - 1<<16,
> - 5<<16,
> - 10<<16,
> - 25<<16,
> - 50<<16,
> - 75<<16,
> - 100<<16,
> - 150<<16,
> - 200<<16,
> - 250<<16,
> - 300<<16,
> - 350<<16,
> - 400<<16,
> - 450<<16,
> - 499<<16,
> + 1 * SHIFTED_PPM,
> + 5 * SHIFTED_PPM,
> + 10 * SHIFTED_PPM,
> + 25 * SHIFTED_PPM,
> + 50 * SHIFTED_PPM,
> + 75 * SHIFTED_PPM,
> + 100 * SHIFTED_PPM,
> + 150 * SHIFTED_PPM,
> + 200 * SHIFTED_PPM,
> + 250 * SHIFTED_PPM,
> + 300 * SHIFTED_PPM,
> + 350 * SHIFTED_PPM,
> + 400 * SHIFTED_PPM,
> + 450 * SHIFTED_PPM,
> + 499 * SHIFTED_PPM,
> };
>
> long outofrange_freq[NUM_FREQ_OUTOFRANGE] = {
> - -1000<<16,
> - -550<<16,
> - 550<<16,
> - 1000<<16,
> + -1000 * SHIFTED_PPM,
> + -550 * SHIFTED_PPM,
> + 550 * SHIFTED_PPM,
> + 1000 * SHIFTED_PPM,
> };
>
> #define LONG_MAX (~0UL>>1)

--
BR,
Muhammad Usama Anjum