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

[Reinette Chatre]

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