From: John Hubbard <jhubbard@nvidia.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>,
Shuah Khan <shuah@kernel.org>,
Nathan Chancellor <nathan@kernel.org>,
Nick Desaulniers <ndesaulniers@google.com>,
Bill Wendling <morbo@google.com>,
Justin Stitt <justinstitt@google.com>,
Fenghua Yu <fenghua.yu@intel.com>,
Valentin Obst <kernel@valentinobst.de>,
linux-kselftest@vger.kernel.org,
LKML <linux-kernel@vger.kernel.org>,
llvm@lists.linux.dev
Subject: Re: [PATCH] selftests/resctrl: fix clang build warnings related to abs(), labs() calls
Date: Mon, 6 May 2024 10:21:01 -0700 [thread overview]
Message-ID: <d93c08c2-6fa8-4c3c-8887-f5ccabd7777a@nvidia.com> (raw)
In-Reply-To: <d46b81df-e018-a9bd-1550-31c825c487cf@linux.intel.com>
On 5/6/24 2:07 AM, Ilpo Järvinen wrote:
> On Fri, 3 May 2024, John Hubbard wrote:
>
>> On 5/3/24 11:37 AM, Reinette Chatre wrote:
>>> On 5/3/2024 9:52 AM, John Hubbard wrote:
>>>> On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
>>>>> On Thu, 2 May 2024, John Hubbard wrote:
>>>> ...
>>>>>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> b/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> index d67ffa3ec63a..c873793d016d 100644
>>>>>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long
>>>>>> *bw_resc, size_t span)
>>>>>> avg_bw_imc = sum_bw_imc / 4;
>>>>>> avg_bw_resc = sum_bw_resc / 4;
>>>>>> - avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>>> + avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>>> avg_diff_per = (int)(avg_diff * 100);
>>>>>> ret = avg_diff_per > MAX_DIFF_PERCENT;
>>>>>
>>>>> But how are these two cases same after your change when you ended up
>>>>> removing taking the absolute value entirely?
>>>>
>>>> All of the arguments are unsigned integers, so all arithmetic results
>>>> are interpreted as unsigned, so taking the absolute value of that is
>>>> always a no-op.
>
> (I see there's a better patch posted already but since there are a few
> incorrect claims in this discussion, I'll do for the record type of
> reply.)
>
> This discussion now went to a tangent about the warning. My main point is
> that logic is not correct after removing labs().
>
> I also disagree with the claim that using labs() on unsigned value is
> no-op because labs() takes long so unsigned is just forced into signed
> when calling which is why the warning triggers but it's very misleading
> warning (see below).
>
Yes you are correct.
>>> It does not seem as though clang can see when values have been casted.
>>> I tried to do so explicitly with a:
>>> avg_diff = labs((long)avg_bw_resc - avg_bw_imc) / (float)avg_bw_imc;
>>
>> The subtraction result will get promoted to an unsigned long, before being
>> passed into labs(3).
>>
>>> But that still triggers:
>>> warning: taking the absolute value of unsigned type 'unsigned long' has no
>>> effect [-Wabsolute-value]
>>
>> As expected, yes.
>
> That error message isn't factually correct:
>
> unsigned long a = LONG_MAX;
> long b;
>
> a += 2;
> b = (long)a;
> printf("%llu %lli %lli\n", a, b, labs(a));
>
> Prints (at least when built with gcc):
>
> 9223372036854775809 -9223372036854775807 9223372036854775807
>
> labs(LONG_MAX + 1) won't work though since it's not positively presentable
> with long and the value is left untouched.
>
Thanks for setting the detailed record straight! :)
thanks,
--
John Hubbard
NVIDIA
next prev parent reply other threads:[~2024-05-06 17:21 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-03 2:32 [PATCH] selftests/resctrl: fix clang build warnings related to abs(), labs() calls John Hubbard
2024-05-03 8:00 ` Ilpo Järvinen
2024-05-03 16:52 ` John Hubbard
2024-05-03 18:37 ` Reinette Chatre
2024-05-03 19:12 ` John Hubbard
2024-05-03 20:46 ` Reinette Chatre
2024-05-03 22:00 ` John Hubbard
2024-05-06 9:07 ` Ilpo Järvinen
2024-05-06 17:21 ` John Hubbard [this message]
-- strict thread matches above, loose matches on Subject: below --
2024-05-08 19:02 John Hubbard
2024-05-08 19:39 ` John Hubbard
2024-05-10 9:06 ` Ilpo Järvinen
2024-05-10 18:01 ` John Hubbard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d93c08c2-6fa8-4c3c-8887-f5ccabd7777a@nvidia.com \
--to=jhubbard@nvidia.com \
--cc=fenghua.yu@intel.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=justinstitt@google.com \
--cc=kernel@valentinobst.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=morbo@google.com \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=reinette.chatre@intel.com \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).