Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751143AbdL1PtU (ORCPT <rfc822;w@1wt.eu>);
        Thu, 28 Dec 2017 10:49:20 -0500
Received: from Galois.linutronix.de ([146.0.238.70]:33041 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750800AbdL1PtS (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 28 Dec 2017 10:49:18 -0500
Date: Thu, 28 Dec 2017 16:49:16 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Nick Desaulniers <nick.desaulniers@gmail.com>
cc: Al Viro <viro@zeniv.linux.org.uk>,
        Deepa Dinamani <deepa.kernel@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: precedence bug in MAKE_PROCESS_CPUCLOCK macro?
In-Reply-To: <CAH7mPvgzy+DRUoqNMnLqSLQZaWquodTmUe3=m3ZG=CqpL7cBzw@mail.gmail.com>
Message-ID: <alpine.DEB.2.20.1712281645540.1899@nanos>
References: <CAH7mPvgzy+DRUoqNMnLqSLQZaWquodTmUe3=m3ZG=CqpL7cBzw@mail.gmail.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
X-Linutronix-Spam-Score: -1.0
X-Linutronix-Spam-Level: -
X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required,  ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2049
Lines: 60

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