| Message ID | 20240409202222.2830476-1-jstultz@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8e222dcf92a812bdd45e33ba029ccdda263d3559 |
| Headers | show |
| Series | selftests: timers: Fix valid-adjtimex signed left-shift undefined behavior | expand |
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 <tglx@linutronix.de> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: Anna-Maria Behnsen <anna-maria@linutronix.de> > Cc: Frederic Weisbecker <frederic@kernel.org> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Nathan Chancellor <nathan@kernel.org> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Lee Jones <joneslee@google.com> > Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: linux-kselftest@vger.kernel.org > Reported-by: Lee Jones <joneslee@google.com> > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Link: https://lore.kernel.org/lkml/0c6d4f0d-2064-4444-986b-1d1ed782135f@collabora.com/ > Signed-off-by: John Stultz <jstultz@google.com> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > .../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)
On 4/9/24 14:22, 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 <tglx@linutronix.de> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: Anna-Maria Behnsen <anna-maria@linutronix.de> > Cc: Frederic Weisbecker <frederic@kernel.org> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Nathan Chancellor <nathan@kernel.org> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Lee Jones <joneslee@google.com> > Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: linux-kselftest@vger.kernel.org > Reported-by: Lee Jones <joneslee@google.com> > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Link: https://lore.kernel.org/lkml/0c6d4f0d-2064-4444-986b-1d1ed782135f@collabora.com/ > Signed-off-by: John Stultz <jstultz@google.com> > --- Applied to linux-kselftest next for Linux6.10-rc1. thanks, -- Shuah
Shuah! On Thu, Apr 11 2024 at 15:01, Shuah Khan wrote: > > Applied to linux-kselftest next for Linux6.10-rc1. I took this already through my tree as I have more timer selftest related stuff pending and coming up soon along with actual kernel changes. Thanks, tglx
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)
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 <tglx@linutronix.de> Cc: Stephen Boyd <sboyd@kernel.org> Cc: Anna-Maria Behnsen <anna-maria@linutronix.de> Cc: Frederic Weisbecker <frederic@kernel.org> Cc: Shuah Khan <shuah@kernel.org> Cc: Nathan Chancellor <nathan@kernel.org> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Lee Jones <joneslee@google.com> Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> Cc: linux-kselftest@vger.kernel.org Reported-by: Lee Jones <joneslee@google.com> Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> Link: https://lore.kernel.org/lkml/0c6d4f0d-2064-4444-986b-1d1ed782135f@collabora.com/ Signed-off-by: John Stultz <jstultz@google.com> --- .../testing/selftests/timers/valid-adjtimex.c | 69 ++++++++++--------- 1 file changed, 35 insertions(+), 34 deletions(-)