[PATCH] mm:kasan: fix sparse warnings: Should it be static?

[Nihar Chaithanya]

The kernel test robot had found sparse warnings: Should it be static,
for the variables kasan_ptr_result and kasan_int_result. These were
declared globally and three functions in kasan_test_c.c use them currently.
Add them to be declared within these functions and remove the global
versions of these.

Reported-by: kernel test robot <l...@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312261010...@intel.com/
Signed-off-by: Nihar Chaithanya <niharch...@gmail.com>
---
mm/kasan/kasan_test_c.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index a181e4780d9d..d0d3a9eea80b 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -41,13 +41,6 @@ static struct {
bool async_fault;
} test_status;

-/*
- * Some tests use these global variables to store return values from function
- * calls that could otherwise be eliminated by the compiler as dead code.
- */
-void *kasan_ptr_result;
-int kasan_int_result;
-
/* Probe for console output: obtains test_status lines of interest. */
static void probe_console(void *ignore, const char *buf, size_t len)
{
@@ -1488,6 +1481,7 @@ static void kasan_memchr(struct kunit *test)
{
char *ptr;
size_t size = 24;
+ void *kasan_ptr_result;

/*
* str* functions are not instrumented with CONFIG_AMD_MEM_ENCRYPT.
@@ -1514,6 +1508,7 @@ static void kasan_memcmp(struct kunit *test)
char *ptr;
size_t size = 24;
int arr[9];
+ int kasan_int_result;

/*
* str* functions are not instrumented with CONFIG_AMD_MEM_ENCRYPT.
@@ -1539,6 +1534,8 @@ static void kasan_strings(struct kunit *test)
{
char *ptr;
size_t size = 24;
+ void *kasan_ptr_result;
+ int kasan_int_result;

/*
* str* functions are not instrumented with CONFIG_AMD_MEM_ENCRYPT.
@@ -1585,6 +1582,8 @@ static void kasan_bitops_modify(struct kunit *test, int nr, void *addr)

static void kasan_bitops_test_and_modify(struct kunit *test, int nr, void *addr)
{
+ int kasan_int_result;
+
KUNIT_EXPECT_KASAN_FAIL(test, test_and_set_bit(nr, addr));
KUNIT_EXPECT_KASAN_FAIL(test, __test_and_set_bit(nr, addr));
KUNIT_EXPECT_KASAN_FAIL(test, test_and_set_bit_lock(nr, addr));
--
2.34.1

[Dmitry Vyukov]

On Fri, 11 Oct 2024 at 05:40, Nihar Chaithanya
<niharch...@gmail.com> wrote:
>
> The kernel test robot had found sparse warnings: Should it be static,
> for the variables kasan_ptr_result and kasan_int_result. These were
> declared globally and three functions in kasan_test_c.c use them currently.
> Add them to be declared within these functions and remove the global
> versions of these.
>
> Reported-by: kernel test robot <l...@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312261010...@intel.com/
> Signed-off-by: Nihar Chaithanya <niharch...@gmail.com>
> ---
> mm/kasan/kasan_test_c.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index a181e4780d9d..d0d3a9eea80b 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -41,13 +41,6 @@ static struct {
> bool async_fault;
> } test_status;
>
> -/*
> - * Some tests use these global variables to store return values from function
> - * calls that could otherwise be eliminated by the compiler as dead code.

Doesn't this change break what's described in this comment?
Since we are assigning to a local var, I assume the compiler can
remove these assignments.

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20241011033604.266084-1-niharchaithanya%40gmail.com.

[Nihar Chaithanya]

Sorry about that, thank you for the pointing it out, I understand now that
compiler might optimize and remove the assignments in case of local
variables where the global variables would be helpful, and making them as
static would be correct approach.

Add a fix making the global variables as static and doesn't trigger
the sparse warnings:
mm/kasan/kasan_test.c:36:6: warning: symbol 'kasan_ptr_result' was not declared. Should it be static?
mm/kasan/kasan_test.c:37:5: warning: symbol 'kasan_int_result' was not declared. Should it be static?

Reported-by: kernel test robot <l...@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312261010...@intel.com/
Signed-off-by: Nihar Chaithanya <niharch...@gmail.com>
---

v1 -> v2: Used the aproach of making global variables static to resolve the
warnings instead of local declarations.

Link to v1: https://lore.kernel.org/all/20241011033604.2660...@gmail.com/

mm/kasan/kasan_test_c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index a181e4780d9d..4803a2c4d8a1 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -45,8 +45,8 @@ static struct {

* Some tests use these global variables to store return values from function

* calls that could otherwise be eliminated by the compiler as dead code.

*/
-void *kasan_ptr_result;
-int kasan_int_result;

+static void *kasan_ptr_result;
+static int kasan_int_result;

/* Probe for console output: obtains test_status lines of interest. */
static void probe_console(void *ignore, const char *buf, size_t len)

--
2.34.1

[Dmitry Vyukov]

On Fri, 11 Oct 2024 at 12:09, Nihar Chaithanya
<niharch...@gmail.com> wrote:
>
> Sorry about that, thank you for the pointing it out, I understand now that
> compiler might optimize and remove the assignments in case of local
> variables where the global variables would be helpful, and making them as
> static would be correct approach.

It should be easy for the compiler to see all uses for a static var,
and in this case it's only assignments, so it becomes effectively
dead, and the compiler can remove the variable and all assignments.

Fighting the compiler in such cases when we want to preserve
non-observable behavior of the abstract C machine is hard.

"static volatile" may be a solution here. Does it help to remove the warnings?

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20241011095259.17345-1-niharchaithanya%40gmail.com.

[Nihar Chaithanya]

Yes, when making the global variables kasan_ptr_result and
kasan_int_result as static volatile, the warnings are removed and
the variable and assignments are retained, but when just static is
used I understand that it might be optimized.

Add a fix making the global varaibles - static volatile, removing the

warnings:
mm/kasan/kasan_test.c:36:6: warning: symbol 'kasan_ptr_result' was not declared. Should it be static?
mm/kasan/kasan_test.c:37:5: warning: symbol 'kasan_int_result' was not declared. Should it be static?

Reported-by: kernel test robot <l...@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312261010...@intel.com/
Signed-off-by: Nihar Chaithanya <niharch...@gmail.com>
---
v1 -> v2: Used the aproach of making global variables static to resolve the
warnings instead of local declarations.

v2 -> v3: Making the global variables static volatile to resolve the
warnings.

Link to v1: https://lore.kernel.org/all/20241011033604.2660...@gmail.com/

Link to v2: https://lore.kernel.org/all/20241011095259.1734...@gmail.com/

mm/kasan/kasan_test_c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c

index a181e4780d9d..7884b46a1e71 100644

--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -45,8 +45,8 @@ static struct {
* Some tests use these global variables to store return values from function
* calls that could otherwise be eliminated by the compiler as dead code.
*/
-void *kasan_ptr_result;
-int kasan_int_result;

+static volatile void *kasan_ptr_result;
+static volatile int kasan_int_result;

[Dmitry Vyukov]

On Fri, 11 Oct 2024 at 13:46, Nihar Chaithanya
<niharch...@gmail.com> wrote:
>
> Yes, when making the global variables kasan_ptr_result and
> kasan_int_result as static volatile, the warnings are removed and
> the variable and assignments are retained, but when just static is
> used I understand that it might be optimized.
>
> Add a fix making the global varaibles - static volatile, removing the
> warnings:
> mm/kasan/kasan_test.c:36:6: warning: symbol 'kasan_ptr_result' was not declared. Should it be static?
> mm/kasan/kasan_test.c:37:5: warning: symbol 'kasan_int_result' was not declared. Should it be static?
>
> Reported-by: kernel test robot <l...@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312261010...@intel.com/
> Signed-off-by: Nihar Chaithanya <niharch...@gmail.com>

Reviewed-by: Dmitry Vyukov <dvy...@google.com>

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20241011114537.35664-1-niharchaithanya%40gmail.com.

[Andrey Konovalov]

On Fri, Oct 11, 2024 at 1:46 PM Nihar Chaithanya
<niharch...@gmail.com> wrote:
>
> Yes, when making the global variables kasan_ptr_result and
> kasan_int_result as static volatile, the warnings are removed and
> the variable and assignments are retained, but when just static is
> used I understand that it might be optimized.

For future reference: please write commit messages in a way that is
readable standalone. I.e. without obscured references to the
discussions on the previous versions of the patch. It's fine to give
such references in itself, but you need to give enough context in the
commit message to make it understandable without looking up those
discussions.

Reviewed-by: Andrey Konovalov <andre...@gmail.com>

Thank you for fixing this!