From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x34.google.com (mail-oa1-x34.google.com [IPv6:2001:4860:4864:20::34]) by sourceware.org (Postfix) with ESMTPS id DB1303851148 for ; Thu, 20 Oct 2022 13:39:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DB1303851148 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oa1-x34.google.com with SMTP id 586e51a60fabf-136b5dd6655so24598072fac.3 for ; Thu, 20 Oct 2022 06:39:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=6kFw9/Mkp/p4B+gplrW2tJSwUW1OE2WwvXnOz2gUhis=; b=mXp81kTG3Eb50K3uXKr4dfxrJTcTOwnMCUuQA0x65bC/B3xUZ0wKQ1VVkrXXkeADqT o1yxTb4XuVxt50HznTpXngzb39fqfr9GxnoalrOuBxVuy1r8e6fH+KbHMJPef1nWn0uQ QHLLgtHVa3OL8kh+d2qQj728SjgkodLJuCLnHwUj+YqlORom9U/qKT35nrfzPs3++PjP Q7Lp9qNg1MvwcBUBha4PdrFVpgmD5E/0zko8f4g+CtH92TSpMYUtSeDabB5OiIaU1LY+ VlN+57xknk5fL+c9Em6f/d25jPwkGTy3ULc7s3+z/d/t1mQus4/K8UIBCR+5KazTlWAH l+MA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=6kFw9/Mkp/p4B+gplrW2tJSwUW1OE2WwvXnOz2gUhis=; b=ya7jRuWD611gZQNg6ZmTdavX0FresTTbNvGEd+qF0oNxatz8oFTGP5sHX5u9XW7f1+ 824bVykoMB6Gw7vZiOV1PFRgMK4QRpP7C54FwWVmrrOPTu30V3BqJjujwLyBpyP48z+N w4R75esuoGqn7KVbFGuWnP6zODvWJdkq3ElaHvpPFVjzrGRY1bA8VupqP2F53+2vFtgd D1kTsvH+kdN9tUqNBm6Ol8ENhuoO6g3C14LTGO+/6KuJqX69OWm3GqocDsKHO1ml+9sK bNh/Wtnwcp0rcAJ9WcFzzK1a5KIM1GEQRLwsks8xqwXeVaUSsne8jkuU2ydhLCBkB9qV +Y3A== X-Gm-Message-State: ACrzQf2yWhCevuz3ipNHMamVQ0AfjuA+wNLFMzKT89TVPVatGEKK2C7q 1sCN28FVG76AcBPOhGDQ3+eNR+oxyPY1ppbN X-Google-Smtp-Source: AMsMyM5RdbqxL0HgBSnOBxwG9BZ+HmD/WQiScIfIh+kWhzNoevM48G316kc9B2Lq8iHdxVckD9Z5gQ== X-Received: by 2002:a05:6870:9612:b0:136:66cc:6d5a with SMTP id d18-20020a056870961200b0013666cc6d5amr8681878oaq.172.1666273176674; Thu, 20 Oct 2022 06:39:36 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c3:7d19:dd57:1ff9:1f14:a9d5? ([2804:1b3:a7c3:7d19:dd57:1ff9:1f14:a9d5]) by smtp.gmail.com with ESMTPSA id cm9-20020a056870b60900b00132741e966asm8845900oab.51.2022.10.20.06.39.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 20 Oct 2022 06:39:36 -0700 (PDT) Message-ID: Date: Thu, 20 Oct 2022 10:39:33 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Subject: Re: [PATCH v2] linux: Avoid shifting a negative signed on POSIX timer interface Content-Language: en-US To: Arjun Shankar Cc: libc-alpha@sourceware.org References: <20220830120802.1072466-1-adhemerval.zanella@linaro.org> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 17/10/22 19:33, Arjun Shankar wrote: > Hi Adhemerval, > >> The current macros uses pid as signed value, which triggers a compiler >> warning for process and thread timers. Replace MAKE_PROCESS_CPUCLOCK >> with static inline function that expects the pid as unsigned. These >> are similar to what Linux does internally. >> >> Checked on x86_64-linux-gnu. > > I tried building master with -Wshift-negative-value and ran into a > couple of other left-shifts on negative values in > sysdeps/x86/dl-cacheinfo.h: That's a good experiment, I see that clang enables it as default. > >> count_mask = ~(-1 << (count_mask + 1)); > > Perhaps a cast of the -1 to unsigned here would make the intent even > more clear, and get glibc building cleanly with > -Wshift-negative-value. What do you think? I think we can also make the mask unsigned to avoid a potential UB and remove the inline assembly since we have a proper builtin for it: unsigned int count_mask = __builtin_clz (threads_l2); count_mask = ~(-1U << (count_mask + 1)); > > Anyway, this patch itself looks good to me. > > Reviewed-by: Arjun Shankar Thanks. > >> nptl/pthread_getcpuclockid.c | 2 +- >> sysdeps/unix/sysv/linux/clock_getcpuclockid.c | 2 +- >> sysdeps/unix/sysv/linux/clock_nanosleep.c | 2 +- >> .../unix/sysv/linux/kernel-posix-cpu-timers.h | 28 +++++++++++++++---- >> sysdeps/unix/sysv/linux/timer_create.c | 4 +-- >> 5 files changed, 28 insertions(+), 10 deletions(-) >> >> diff --git a/nptl/pthread_getcpuclockid.c b/nptl/pthread_getcpuclockid.c >> index 344bd6560e..b8bf09f550 100644 >> --- a/nptl/pthread_getcpuclockid.c >> +++ b/nptl/pthread_getcpuclockid.c >> @@ -35,7 +35,7 @@ __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid) >> >> /* The clockid_t value is a simple computation from the TID. */ >> >> - const clockid_t tidclock = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED); >> + const clockid_t tidclock = make_thread_cpuclock (pd->tid, CPUCLOCK_SCHED); >> >> *clockid = tidclock; >> return 0; > > OK. Use the new function instead of the old macro. > >> diff --git a/sysdeps/unix/sysv/linux/clock_getcpuclockid.c b/sysdeps/unix/sysv/linux/clock_getcpuclockid.c >> index 5534127ed7..355d3c86af 100644 >> --- a/sysdeps/unix/sysv/linux/clock_getcpuclockid.c >> +++ b/sysdeps/unix/sysv/linux/clock_getcpuclockid.c >> @@ -29,7 +29,7 @@ __clock_getcpuclockid (pid_t pid, clockid_t *clock_id) >> /* The clockid_t value is a simple computation from the PID. >> But we do a clock_getres call to validate it. */ >> >> - const clockid_t pidclock = MAKE_PROCESS_CPUCLOCK (pid, CPUCLOCK_SCHED); >> + const clockid_t pidclock = make_process_cpuclock (pid, CPUCLOCK_SCHED); >> >> #ifndef __NR_clock_getres_time64 >> # define __NR_clock_getres_time64 __NR_clock_getres > > OK. Same. > >> diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c >> index befe6ecb8c..e610fd4e8d 100644 >> --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c >> +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c >> @@ -34,7 +34,7 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, >> if (clock_id == CLOCK_THREAD_CPUTIME_ID) >> return EINVAL; >> if (clock_id == CLOCK_PROCESS_CPUTIME_ID) >> - clock_id = MAKE_PROCESS_CPUCLOCK (0, CPUCLOCK_SCHED); >> + clock_id = PROCESS_CLOCK; >> >> /* If the call is interrupted by a signal handler or encounters an error, >> it returns a positive value similar to errno. */ > > OK. PROCESS_CLOCK expands to the appropriate call to make_process_cpuclock. > >> diff --git a/sysdeps/unix/sysv/linux/kernel-posix-cpu-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-cpu-timers.h >> index 164a90ddeb..bea1e0e62d 100644 >> --- a/sysdeps/unix/sysv/linux/kernel-posix-cpu-timers.h >> +++ b/sysdeps/unix/sysv/linux/kernel-posix-cpu-timers.h >> @@ -1,4 +1,12 @@ >> -/* Parameters for the Linux kernel ABI for CPU clocks. */ >> +/* >> + Parameters for the Linux kernel ABI for CPU clocks, the bit fields within >> + a clockid: >> + >> + - The most significant 29 bits hold either a pid or a file descriptor. >> + - Bit 2 indicates whether a cpu clock refers to a thread or a process. >> + - Bits 1 and 0 give the type: PROF=0, VIRT=1, SCHED=2, or FD=3. >> + - A clockid is invalid if bits 2, 1, and 0 are all set. >> + */ >> >> #define CPUCLOCK_PID(clock) ((pid_t) ~((clock) >> 3)) >> #define CPUCLOCK_PERTHREAD(clock) \ > > OK. Comment lines up with the one in Linux's "include/linux/posix-timers.h". > >> @@ -12,7 +20,17 @@ >> #define CPUCLOCK_SCHED 2 >> #define CPUCLOCK_MAX 3 >> >> -#define MAKE_PROCESS_CPUCLOCK(pid, clock) \ >> - ((~(clockid_t) (pid) << 3) | (clockid_t) (clock)) > > OK. clockid_t is a signed type, thus expanding with pid = 0 leads to a > left-shift on a negative value. > >> -#define MAKE_THREAD_CPUCLOCK(tid, clock) \ >> - MAKE_PROCESS_CPUCLOCK((tid), (clock) | CPUCLOCK_PERTHREAD_MASK) > > OK. Same macro used. Same issue. > >> +static inline clockid_t >> +make_process_cpuclock (unsigned int pid, clockid_t clock) >> +{ >> + return ((~pid) << 3) | clock; >> +} >> + > > OK. Replacement function that uses unsigned. Lines up with the version > in Linux's sources. > >> +static inline clockid_t >> +make_thread_cpuclock (unsigned int tid, clockid_t clock) >> +{ >> + return make_process_cpuclock (tid, clock | CPUCLOCK_PERTHREAD_MASK); >> +} >> + > > OK. Replaces the second macro for tid. > >> +#define PROCESS_CLOCK make_process_cpuclock (0, CPUCLOCK_SCHED) >> +#define THREAD_CLOCK make_thread_cpuclock (0, CPUCLOCK_SCHED) > > OK. Macros for convenience. > >> diff --git a/sysdeps/unix/sysv/linux/timer_create.c b/sysdeps/unix/sysv/linux/timer_create.c >> index a8b2a41d9e..290324a7ea 100644 >> --- a/sysdeps/unix/sysv/linux/timer_create.c >> +++ b/sysdeps/unix/sysv/linux/timer_create.c >> @@ -33,9 +33,9 @@ ___timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid) >> { >> { >> clockid_t syscall_clockid = (clock_id == CLOCK_PROCESS_CPUTIME_ID >> - ? MAKE_PROCESS_CPUCLOCK (0, CPUCLOCK_SCHED) >> + ? PROCESS_CLOCK >> : clock_id == CLOCK_THREAD_CPUTIME_ID >> - ? MAKE_THREAD_CPUCLOCK (0, CPUCLOCK_SCHED) >> + ? THREAD_CLOCK >> : clock_id); >> >> /* If the user wants notification via a thread we need to handle > > OK. Use the new macros. >