Received-SPF: pass (google.com: domain of linux-kernel+bounces-170225-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d93c08c2-6fa8-4c3c-8887-f5ccabd7777a@nvidia.com>
Date: Mon, 6 May 2024 10:21:01 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] selftests/resctrl: fix clang build warnings related to
 abs(), labs() calls
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <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
References: <20240503023209.80787-1-jhubbard@nvidia.com>
 <793bd068-c3b4-6330-41a4-bea597b1d820@linux.intel.com>
 <f908ba74-86c0-409c-854d-9da5f3917b05@nvidia.com>
 <26f3effc-6ea1-4670-a301-76df3a710fa9@intel.com>
 <b88e73ea-d3f6-42d0-b9e0-f97665546178@nvidia.com>
 <d46b81df-e018-a9bd-1550-31c825c487cf@linux.intel.com>
Content-Language: en-US
From: John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <d46b81df-e018-a9bd-1550-31c825c487cf@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?NGFYRWYrM2JmVWtnOVhTTXYzTENQWkNkdDNxVi9nN3NxbU1NQ1JkZS9QSlp4?=
 =?utf-8?B?Z0NpWE1tVGNjYlB1SUVSWHNGQVBjTXc2QTVxbFdSUHF5ckwyY2ZMLzJiZVJK?=
 =?utf-8?B?Z1Nxdm1UeDBsUlhnYjZXVkhkQWZKMnNQN3hmS3o0TCtpSmF0a2VicjhmUHRp?=
 =?utf-8?B?ZFErZnMvUjlPUzA3NHRiY3UrZ0FXODlHM1RCKzBQaDhBc1hqbFJHQ3NNVm53?=
 =?utf-8?B?dFhzZDJYLzFCYVh3a3RXMlJrR2ptaFA4a0hOVHk5anh6QkF6QlQxTHJiR28w?=
 =?utf-8?B?TEJPVWo5MWdXaGc3TExPOEROR2p6TDJiL2l0dldSUzQrSjI3V01meTF5d2hC?=
 =?utf-8?B?dDRVbER2eFdEVGRRbW5uVDFPQndvRGVCYkUxNUNPbENoLzVwUEliMHRxWnkz?=
 =?utf-8?B?UVBSZVNEMHJLRWEvNmxzM0RUUVRIanBpVDNPQ0xQTk82d1owYmlsSzdyM0VO?=
 =?utf-8?B?OVBlWmo3dmEwSWJGVmdhV09WU3dzSVlyaGtxMm9hU05KRVgzUXIzVUNBRUlM?=
 =?utf-8?B?R1JlbDJwUnBpR1k4NDE4WkdEaWFiSzR5NjJha29XY2FWaHl3TDlPckQ0aG5h?=
 =?utf-8?B?d3ZFOEhoM1RrRlR0aWMrMlRveFN4WVRKaG5GTDN3WkYwdG1MMWI4bGdZWThw?=
 =?utf-8?B?T2JIMWJoeXZwdWJDMVBNZUE0bEpvekwweFl6cXRENzVDWlFvTU0rd2ZPejlL?=
 =?utf-8?B?RDlQZjlkbHl1U1JSc3N1TkxSTUlIQjlBcEpTSmFCakQ5SkNOMlVqc2F3aTNk?=
 =?utf-8?B?Z1BZUXFmZzFNbUZXVVBqbmJyaHY2Z08zQTBiNjA1OXlZZHdzTjl0RThmdVBB?=
 =?utf-8?B?bnl0TS9WRFR3OHdLTHZ0eGZ2MGtEbEZhWjQrL2Qrc0h2T29ha2RDZ3ZURTJx?=
 =?utf-8?B?VUdISVVqbHczMWNxekkzQVExVFF6diswdDAwcEM2eHlSVWFPY1l0S3poUGti?=
 =?utf-8?B?enVPc283bDQ0RE9CTTNEaVE5d245c3FxKzFwNXEvYVBkS1UwZG1la1dWb1E5?=
 =?utf-8?B?L2Fnd0JJcEpGWXBKQmhGeFdIaEpzajZEV3M3dktHLzRMS2ZqZ1N1Z2NMaDNp?=
 =?utf-8?B?REFZOWRraDdhTXlzNnNZZVdZUU9pUFhibk5zRnJpTkJFRjlBeXJJMWdFWkVZ?=
 =?utf-8?B?RGprYlRLd0E0NXlpa01sWm5pUHQ4L0dYeDNKVVdlRVJoSU1MRzlNT01kZWhN?=
 =?utf-8?B?dElRWThKTVkwaUdVKzgvSkZWZXIvOEFuSVM0cFBJSmUrcHVzT2xOVnpmaGQ1?=
 =?utf-8?B?emFGeU4za0xEM0EwYU5wL2dtcXE0WmRmSWFYZ0xDQTZDTFU1ZEQwT1dXSWJk?=
 =?utf-8?B?QmZrRWptTXpNYiszaUZ6VlpYMzRHL3VCN012RDVUbXZua2dYWnZwRjRyY2dl?=
 =?utf-8?B?eUtpaTQrb2dVOXpxRjlrSkpSRTNyUHh1bktYTzJvbEFQcUdkalJDOGJQc3RL?=
 =?utf-8?B?Y1hGWWJNbUh5UUFrL29acFc5YUVPVlhjeHo4NkI1K2JDaTZJUkE0TGczazR3?=
 =?utf-8?B?NzIzRm42RkFMRWpHb3BRRjZnUjNWaUpGN3pmMG16SkJtaDVDTXZhVExiQVRC?=
 =?utf-8?B?aGxXOXc1cDhWOWdyTXJjbVBDNFNkNmg1TFdSSFZyQUdqc2dhWkZjZmtTaU5a?=
 =?utf-8?B?WE9WVHVWWXVYaVB5RFBDb0hnbDEzcklEM2E2ZTdzdmxNbEVTVGYwQWwweU1k?=
 =?utf-8?B?bkswTDZjTWNoL3VvVExwN3JwOTNubW5VSFZZOGw0RzJsaEk0OFpJL0tzL3pR?=
 =?utf-8?B?ZHJWeGhzMDVFbktFSzduV3U1elZPWjlnNEQzSFo5R0pHdjRMLzJEOWN0SHMx?=
 =?utf-8?B?Q3U5dVVXOG13RUVvWU5rNi9hTkVoL1RHZ1JQTkFkeHlJWTNLWXRYUWFSSDJh?=
 =?utf-8?B?K2ZTNWJCUHNZUmtmMk90M3ZhRDFleGY0NDl2N1VrNzZ1Q3AxRGtHZnQzZGts?=
 =?utf-8?B?RFFJbXY0UVlKdk9sN3k2enM5WkhZYW8xRi9Ga0NFUHFCeTlmKy9tNDFIVDEw?=
 =?utf-8?B?ak83NjIrd2tGYkpXY2hoRmhNbldBbkVaSVdyRzMrdmRMd3NqZmVHSksxUkxw?=
 =?utf-8?B?NC92UDNGRm81NXVXcm1JM2x1Yys3ejhNdE5rSVJQM0ovbkxHaDJHa0FWQUpu?=
 =?utf-8?B?VEtMNjRyUlNPdnlqRzRTbU1aenFDZ3cwdHNaMU80VHFwenFRRTkwcHVPdUNz?=
 =?utf-8?Q?8gAne6suHeRoSzHqC28+81Y=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 50dd90be-9efe-4d0e-2b99-08dc6df0ec68
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 17:21:10.7156
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dtZbbF4xn4OjEnW60+oIGns+1SjJePC+rIFSYL4nO9O5+R+cbWkC9fZAmFotjynSi6dPO7Ha53n+EGeUjvOHsA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB8184

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