Messages in this thread | | | Date | Thu, 28 Dec 2017 16:49:16 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: precedence bug in MAKE_PROCESS_CPUCLOCK macro? |
| |
On Sat, 23 Dec 2017, Nick Desaulniers wrote: > I'm seeing the following warning compiling with Clang: > > kernel/time/posix-cpu-timers.c:1397:29: warning: shifting a negative > signed value is undefined > [-Wshift-negative-value] > return posix_cpu_clock_get(THREAD_CLOCK, tp); > ^~~~~~~~~~~~ > kernel/time/posix-cpu-timers.c:1367:22: note: expanded from macro 'THREAD_CLOCK' > #define THREAD_CLOCK MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./include/linux/posix-timers.h:48:2: note: expanded from macro > 'MAKE_THREAD_CPUCLOCK' > MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./include/linux/posix-timers.h:46:23: note: expanded from macro > 'MAKE_PROCESS_CPUCLOCK' > ((~(clockid_t) (pid) << 3) | (clockid_t) (clock)) > ~~~~~~~~~~~~~~~~~~ ^ > > If I understand C's operator precedence rules > (http://en.cppreference.com/w/c/language/operator_precedence) > correctly, then I suspect the problem is in the sub-expression: > > (~(clockid_t) (pid) << 3) > > where pid (an argument to the macro) is first cast to a clockid_t (aka > [signed] int), then negated, then shifted by 3 (oops, undefined > behavior). > > Should the result after negation be cast to an unsigned int, or should > the left shift happen before negation? > > CPUCLOCK_PID and CLOCKID_TO_FD seem to shift then negate, but > FD_TO_CLOCKID seems to have the same issue as MAKE_PROCESS_CPUCLOCK. > > Changing the sub-expression to: > > (~(clockid_t) ((pid) << 3)) > > changes what it evaluates to. Changing it to: > > (~(unsigned) (pid) << 3) > > or > > ((unsigned) ~(clockid_t) (pid) << 3) > > or > > (((unsigned) ~(clockid_t) (pid)) << 3) /* ugly */
All of these are butt ugly. And the same problem exists for FD_TO_CLOCKID.
My preference would be to replace all these crappy macros with simple inline functions.
Thanks,
tglx
| |