linux-kernel - Re: [PATCH v2] selftests/resctrl: fix clang build warnings related to abs(), labs() calls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d67f4f57-4e9a-4715-b6dd-7b83a240b7dd@intel.com>
Date: Tue, 7 May 2024 18:21:36 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: John Hubbard <jhubbard@...dia.com>, Shuah Khan <shuah@...nel.org>
CC: Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers
	<ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>, Justin Stitt
	<justinstitt@...gle.com>, Fenghua Yu <fenghua.yu@...el.com>, Valentin Obst
	<kernel@...entinobst.de>, <linux-kselftest@...r.kernel.org>, LKML
	<linux-kernel@...r.kernel.org>, <llvm@...ts.linux.dev>
Subject: Re: [PATCH v2] selftests/resctrl: fix clang build warnings related to
 abs(), labs() calls

Hi John,

On 5/7/2024 6:16 PM, John Hubbard wrote:
> On 5/7/24 3:30 PM, Reinette Chatre wrote:
> ...
>>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>> index a81f91222a89..af33abd1cca7 100644
>>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>>> @@ -29,22 +29,22 @@ static int cmt_setup(const struct resctrl_test *test,
>>>       return 0;
>>>   }
>>>   -static int show_results_info(unsigned long sum_llc_val, int no_of_bits,
>>> -                 unsigned long cache_span, unsigned long max_diff,
>>> -                 unsigned long max_diff_percent, unsigned long num_of_runs,
>>> +static int show_results_info(long sum_llc_val, int no_of_bits,
>>> +                 long cache_span, long max_diff,
>>> +                 long max_diff_percent, long num_of_runs,
>>>                    bool platform)
>>>   {
>>> -    unsigned long avg_llc_val = 0;
>>> +    long avg_llc_val = 0;
>>>       float diff_percent;
>>>       long avg_diff = 0;
>>>       int ret;
>>>         avg_llc_val = sum_llc_val / num_of_runs;
>>> -    avg_diff = (long)abs(cache_span - avg_llc_val);
>>> +    avg_diff = labs(cache_span - avg_llc_val);
>>>       diff_percent = ((float)cache_span - avg_llc_val) / cache_span * 100;
>>>         ret = platform && abs((int)diff_percent) > max_diff_percent &&
>>> -          abs(avg_diff) > max_diff;
>>> +          labs(avg_diff) > max_diff;
>>>         ksft_print_msg("%s Check cache miss rate within %lu%%\n",
>>>                  ret ? "Fail:" : "Pass:", max_diff_percent);
>>
>> The changes in this hunk are unexpected. The changes to this area made by previous
>> version was ok, no? It really seems like this just does a brute force of everything
> 
> Well, not entirely. That first version was when I still believed clang's
> claim that abs()/labs() was a no-op. I've since been corrected! :)
> 
>> to long (while taking labs() twice) unnecessarily.
> 
> Which part exactly is unnecessary? Are you looking at the function args?
> Or something else? I've stared at it too much and am not spotting the
> issue yet.
> 

The following (what was in v1) looks good to me. What am I missing?

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index a81f91222a89..05a241519ae8 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -40,11 +40,11 @@ static int show_results_info(unsigned long sum_llc_val, int no_of_bits,
 	int ret;
 
 	avg_llc_val = sum_llc_val / num_of_runs;
-	avg_diff = (long)abs(cache_span - avg_llc_val);
+	avg_diff = (long)(cache_span - avg_llc_val);
 	diff_percent = ((float)cache_span - avg_llc_val) / cache_span * 100;
 
 	ret = platform && abs((int)diff_percent) > max_diff_percent &&
-	      abs(avg_diff) > max_diff;
+	      labs(avg_diff) > max_diff;
 
 	ksft_print_msg("%s Check cache miss rate within %lu%%\n",
 		       ret ? "Fail:" : "Pass:", max_diff_percent);

Reinette