Re: [PATCH v4 01/17] selftests/resctrl: Fix compilation issues for global variables

[Shuah Khan]

On 11/30/20 1:19 PM, Fenghua Yu wrote:
> Reinette reported following compilation issue on Fedora 32, gcc version
> 10.1.1
>
> /usr/bin/ld: cqm_test.o:<src_dir>/cqm_test.c:22: multiple definition of
> `cache_size'; cat_test.o:<src_dir>/cat_test.c:23: first defined here
>
> The same issue is reported for long_mask, cbm_mask, count_of_bits etc
> variables as well. Compiler isn't happy because these variables are
> defined globally in two .c files namely cqm_test.c and cat_test.c and
> the compiler during compilation finds that the variable is already
> defined (multiple definition error).
>
> Taking a closer look at the usage of these variables reveals that these
> variables are used only locally to functions such as cqm_resctrl_val()
> (defined in cqm_test.c) and cat_perf_miss_val() (defined in cat_test.c).
> These variables are not shared between those functions. So, there is no
> need for these variables to be global. Hence, fix this issue by making
> them local variables to the functions where they are used.

Easy fix to this problem would be making these variables static to
these files. I am not seeing any real advantage to changing these
variable to local variables.

diff --git a/tools/testing/selftests/resctrl/cat_test.c 
b/tools/testing/selftests/resctrl/cat_test.c
index 5da43767b973..360456b8a1b6 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -17,10 +17,10 @@
 #define MAX_DIFF_PERCENT       4
 #define MAX_DIFF               1000000

-int count_of_bits;
-char cbm_mask[256];
-unsigned long long_mask;
-unsigned long cache_size;
+static int count_of_bits;
+static cbm_mask[256];
+static unsigned long long_mask;
+static unsigned long cache_size;

Same changes made to tools/testing/selftests/resctrl/cqm_test.c

> To fix issues for other global variables (e.g: bm_pid, ppid, llc_occup_path
> and is_amd) that are used across .c files, declare them as extern.

This change is fine. Make this a separate patch.

This way, we can take it as a fixes to stables.

With the above the test builds fine. There is a bigger problem that
needs fixing: (thix might have been fixed in later patches perhaps):

cqm_test.c: In function ‘check_results’:
cqm_test.c:89:9: warning: ‘fgets’ writing 1024 bytes into a region of 
size 512 overflows the destination [-Wstringop-overflow=]
   89 |  while (fgets(temp, 1024, fp)) {
      |         ^~~~~~~~~~~~~~~~~~~~~
In file included from resctrl.h:5,
                 from cqm_test.c:11:
/usr/include/stdio.h:568:14: note: in a call to function ‘fgets’ 
declared with attribute ‘write_only (1, 2)’
  568 | extern char *fgets (char *__restrict __s, int __n, FILE 
*__restrict __stream)
      |              ^~~~~


thanks,
-- Shuah