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

[Ilpo Järvinen]

On Thu, 2 May 2024, John Hubbard wrote:

> First of all, in order to build with clang at all, one must first apply
> Valentin Obst's build fix for LLVM [1]. Furthermore, for this particular
> resctrl directory, my pending fix [2] must also be applied. Once those
> fixes are in place, then when building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftests
> 
> ..two types of warnings occur:
> 
>     warning: absolute value function 'abs' given an argument of type
>     'long' but has parameter of type 'int' which may cause truncation of
>     value
> 
>     warning: taking the absolute value of unsigned type 'unsigned long'
>     has no effect
> 
> Fix these by:
> 
> a) using labs() in place of abs(), when long integers are involved, and
> 
> b) don't call labs() unnecessarily.
> 
> [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@xxxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/all/20240503021712.78601-1-jhubbard@xxxxxxxxxx/
> 
> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> ---
>  tools/testing/selftests/resctrl/cmt_test.c | 4 ++--
>  tools/testing/selftests/resctrl/mba_test.c | 2 +-
>  tools/testing/selftests/resctrl/mbm_test.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> 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);

This seems fine but...

> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 7946e32e85c8..673b2bb800f7 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -77,7 +77,7 @@ static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
>  
>  		avg_bw_imc = sum_bw_imc / (NUM_OF_RUNS - 1);
>  		avg_bw_resc = sum_bw_resc / (NUM_OF_RUNS - 1);
> -		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);
>  
>  		ksft_print_msg("%s Check MBA diff within %d%% for schemata %u\n",
> 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?


-- 
 i.