[PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[wei.guo.simon]

From: Simon Guo <wei.guo.simon@gmail.com>

The ckpt_regs usage in gpr32_set_common/gpr32_get_common()
will lead to cppcheck error.

[arch/powerpc/kernel/ptrace.c:2062]: (error) Uninitialized variable: ckpt_regs
[arch/powerpc/kernel/ptrace.c:2130]: (error) Uninitialized variable: ckpt_regs

A straightforward fix to clean it.

Reported-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
---
 arch/powerpc/kernel/ptrace.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 4f3c575..ae02377 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2046,21 +2046,23 @@ static const struct user_regset_view user_ppc_native_view = {
 static int gpr32_get_common(struct task_struct *target,
 		     const struct user_regset *regset,
 		     unsigned int pos, unsigned int count,
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 			    void *kbuf, void __user *ubuf, bool tm_active)
+#else
+			    void *kbuf, void __user *ubuf)
+#endif
 {
 	const unsigned long *regs = &target->thread.regs->gpr[0];
-	const unsigned long *ckpt_regs;
 	compat_ulong_t *k = kbuf;
 	compat_ulong_t __user *u = ubuf;
 	compat_ulong_t reg;
 	int i;
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	ckpt_regs = &target->thread.ckpt_regs.gpr[0];
-#endif
 	if (tm_active) {
-		regs = ckpt_regs;
+		regs = &target->thread.ckpt_regs.gpr[0];
 	} else {
+#endif
 		if (target->thread.regs == NULL)
 			return -EIO;
 
@@ -2072,7 +2074,9 @@ static int gpr32_get_common(struct task_struct *target,
 			for (i = 14; i < 32; i++)
 				target->thread.regs->gpr[i] = NV_REG_POISON;
 		}
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	}
+#endif
 
 	pos /= sizeof(reg);
 	count /= sizeof(reg);
@@ -2114,28 +2118,31 @@ static int gpr32_get_common(struct task_struct *target,
 static int gpr32_set_common(struct task_struct *target,
 		     const struct user_regset *regset,
 		     unsigned int pos, unsigned int count,
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 		     const void *kbuf, const void __user *ubuf, bool tm_active)
+#else
+		     const void *kbuf, const void __user *ubuf)
+#endif
 {
 	unsigned long *regs = &target->thread.regs->gpr[0];
-	unsigned long *ckpt_regs;
 	const compat_ulong_t *k = kbuf;
 	const compat_ulong_t __user *u = ubuf;
 	compat_ulong_t reg;
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	ckpt_regs = &target->thread.ckpt_regs.gpr[0];
-#endif
-
 	if (tm_active) {
-		regs = ckpt_regs;
+		regs = &target->thread.ckpt_regs.gpr[0];
 	} else {
+#endif
 		regs = &target->thread.regs->gpr[0];
 
 		if (target->thread.regs == NULL)
 			return -EIO;
 
 		CHECK_FULL_REGS(target->thread.regs);
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	}
+#endif
 
 	pos /= sizeof(reg);
 	count /= sizeof(reg);
@@ -2218,7 +2225,11 @@ static int gpr32_get(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     void *kbuf, void __user *ubuf)
 {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 0);
+#else
+	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf);
+#endif
 }
 
 static int gpr32_set(struct task_struct *target,
@@ -2226,7 +2237,11 @@ static int gpr32_set(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     const void *kbuf, const void __user *ubuf)
 {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, 0);
+#else
+	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf);
+#endif
 }
 
 /*
-- 
1.8.3.1

Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[Daniel Axtens]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

Hi Simon,

> The ckpt_regs usage in gpr32_set_common/gpr32_get_common()
> will lead to cppcheck error.
>
> [arch/powerpc/kernel/ptrace.c:2062]: (error) Uninitialized variable: ckpt_regs
> [arch/powerpc/kernel/ptrace.c:2130]: (error) Uninitialized variable: ckpt_regs
>
> A straightforward fix to clean it.

I'm always happy to see cppcheck warnings fixed :)

>  static int gpr32_get_common(struct task_struct *target,
>  		     const struct user_regset *regset,
>  		     unsigned int pos, unsigned int count,
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  			    void *kbuf, void __user *ubuf, bool tm_active)
> +#else
> +			    void *kbuf, void __user *ubuf)
> +#endif

I wonder if it might be possible to avoid some of the ifdefs and general
churn by making the tm_active argument __maybe_unused rather than
ifdefing around it?

In particular, it would mean the two hunks in the function definitions
and these these two hunks at the call site would be unnecessary:
> @@ -2218,7 +2225,11 @@ static int gpr32_get(struct task_struct *target,
>  		     unsigned int pos, unsigned int count,
>  		     void *kbuf, void __user *ubuf)
>  {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 0);
> +#else
> +	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf);
> +#endif
>  }
>  
>  static int gpr32_set(struct task_struct *target,
> @@ -2226,7 +2237,11 @@ static int gpr32_set(struct task_struct *target,
>  		     unsigned int pos, unsigned int count,
>  		     const void *kbuf, const void __user *ubuf)
>  {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, 0);
> +#else
> +	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf);
> +#endif
>  }

Apart from that, thanks for fixing this up!

Regards,
Daniel

>  
>  /*
> -- 
> 1.8.3.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 859 bytes --]

Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[Simon Guo]

Hi Daniel,
On Wed, Aug 24, 2016 at 12:21:23PM +1000, Daniel Axtens wrote:
> Hi Simon,
> 
> > The ckpt_regs usage in gpr32_set_common/gpr32_get_common()
> > will lead to cppcheck error.
> >
> > [arch/powerpc/kernel/ptrace.c:2062]: (error) Uninitialized variable: ckpt_regs
> > [arch/powerpc/kernel/ptrace.c:2130]: (error) Uninitialized variable: ckpt_regs
> >
> > A straightforward fix to clean it.
> 
> I'm always happy to see cppcheck warnings fixed :)
Thanks for raising this issue :)

> 
> >  static int gpr32_get_common(struct task_struct *target,
> >  		     const struct user_regset *regset,
> >  		     unsigned int pos, unsigned int count,
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >  			    void *kbuf, void __user *ubuf, bool tm_active)
> > +#else
> > +			    void *kbuf, void __user *ubuf)
> > +#endif
> 
> I wonder if it might be possible to avoid some of the ifdefs and general
> churn by making the tm_active argument __maybe_unused rather than
> ifdefing around it?
> 
> In particular, it would mean the two hunks in the function definitions
> and these these two hunks at the call site would be unnecessary:
I think keeping tm_active argument for "ifndef CONFIG_PPC_TRANSACTIONAL_MEM" 
case (with __maybe_unused prefix) will be somehow strange -- Whatever
value is provided in the caller function for tm_active, programmer might be 
puzzled and cost sometime to think about it.  I don't like to use 
"__maybe_unused" to bypass this warning.

Thanks,
- Simon

Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[Daniel Axtens]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

Simon Guo <wei.guo.simon@gmail.com> writes:

> I think keeping tm_active argument for "ifndef CONFIG_PPC_TRANSACTIONAL_MEM" 
> case (with __maybe_unused prefix) will be somehow strange -- Whatever
> value is provided in the caller function for tm_active, programmer might be 
> puzzled and cost sometime to think about it.  I don't like to use 
> "__maybe_unused" to bypass this warning.

Fair enough. I don't have strong feelings either way - we'll see if the
maintainers have any thoughts.

Regards,
Daniel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 859 bytes --]

Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[Michael Ellerman]

Daniel Axtens <dja@axtens.net> writes:

> [ Unknown signature status ]
> Simon Guo <wei.guo.simon@gmail.com> writes:
>
>> I think keeping tm_active argument for "ifndef CONFIG_PPC_TRANSACTIONAL_MEM" 
>> case (with __maybe_unused prefix) will be somehow strange -- Whatever
>> value is provided in the caller function for tm_active, programmer might be 
>> puzzled and cost sometime to think about it.  I don't like to use 
>> "__maybe_unused" to bypass this warning.
>
> Fair enough. I don't have strong feelings either way - we'll see if the
> maintainers have any thoughts.

I do - Sorry Simon but your patch just adds too many #ifdefs.

Any time you have to do something like:

	+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
		}
	+#endif

It should be a sign that something has gone wrong :)

Does Cyril's series to rework the TM structures help at all with this
warning?

cheers

Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[Simon Guo]

On Fri, Sep 09, 2016 at 08:52:52PM +1000, Michael Ellerman wrote:
> I do - Sorry Simon but your patch just adds too many #ifdefs.
> 
> Any time you have to do something like:
> 
> 	+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> 		}
> 	+#endif
> 
> It should be a sign that something has gone wrong :)
> 
> Does Cyril's series to rework the TM structures help at all with this
> warning?
Hi Michael,

What cppchecker complains is only concerned with GPR:
gpr32_get/set_common() which is used by tm_cgpr32_get()/gpr32_get().

Cyril's patch changes TM FPR/VR/VSX state saving location to be consistent with 
GPR's.  It doesn't actually modify TM GPR behavior. 

So Cyril's work doesn't relate with this cppchecker complaining issue.


Thanks for the feedback regarding too many ifdefs.  Is following implemention 
better for this issue?

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index bf91658..cf48e98 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2065,34 +2065,14 @@ static const struct user_regset_view user_ppc_native_view = {
 static int gpr32_get_common(struct task_struct *target,
 		     const struct user_regset *regset,
 		     unsigned int pos, unsigned int count,
-			    void *kbuf, void __user *ubuf, bool tm_active)
+			    void *kbuf, void __user *ubuf,
+			    unsigned long *regs)
 {
-	const unsigned long *regs = &target->thread.regs->gpr[0];
-	const unsigned long *ckpt_regs;
 	compat_ulong_t *k = kbuf;
 	compat_ulong_t __user *u = ubuf;
 	compat_ulong_t reg;
 	int i;
 
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	ckpt_regs = &target->thread.ckpt_regs.gpr[0];
-#endif
-	if (tm_active) {
-		regs = ckpt_regs;
-	} else {
-		if (target->thread.regs == NULL)
-			return -EIO;
-
-		if (!FULL_REGS(target->thread.regs)) {
-			/*
-			 * We have a partial register set.
-			 * Fill 14-31 with bogus values.
-			 */
-			for (i = 14; i < 32; i++)
-				target->thread.regs->gpr[i] = NV_REG_POISON;
-		}
-	}
-
 	pos /= sizeof(reg);
 	count /= sizeof(reg);
 
@@ -2133,29 +2113,13 @@ static int gpr32_get_common(struct task_struct *target,
 static int gpr32_set_common(struct task_struct *target,
 		     const struct user_regset *regset,
 		     unsigned int pos, unsigned int count,
-		     const void *kbuf, const void __user *ubuf, bool tm_active)
+		     const void *kbuf, const void __user *ubuf,
+		     unsigned long *regs)
 {
-	unsigned long *regs = &target->thread.regs->gpr[0];
-	unsigned long *ckpt_regs;
 	const compat_ulong_t *k = kbuf;
 	const compat_ulong_t __user *u = ubuf;
 	compat_ulong_t reg;
 
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	ckpt_regs = &target->thread.ckpt_regs.gpr[0];
-#endif
-
-	if (tm_active) {
-		regs = ckpt_regs;
-	} else {
-		regs = &target->thread.regs->gpr[0];
-
-		if (target->thread.regs == NULL)
-			return -EIO;
-
-		CHECK_FULL_REGS(target->thread.regs);
-	}
-
 	pos /= sizeof(reg);
 	count /= sizeof(reg);
 
@@ -2220,7 +2184,7 @@ static int tm_cgpr32_get(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     void *kbuf, void __user *ubuf)
 {
-	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 1);
+	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, &target->thread.ckpt_regs.gpr[0]);
 }
 
 static int tm_cgpr32_set(struct task_struct *target,
@@ -2228,7 +2192,7 @@ static int tm_cgpr32_set(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     const void *kbuf, const void __user *ubuf)
 {
-	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, 1);
+	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, &target->thread.ckpt_regs.gpr[0]);
 }
 #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
 
@@ -2237,7 +2201,18 @@ static int gpr32_get(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     void *kbuf, void __user *ubuf)
 {
-	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 0);
+	if (target->thread.regs == NULL)
+		return -EIO;
+
+	if (!FULL_REGS(target->thread.regs)) {
+		/*
+		 * We have a partial register set.
+		 * Fill 14-31 with bogus values.
+		 */
+		for (i = 14; i < 32; i++)
+			target->thread.regs->gpr[i] = NV_REG_POISON;
+	}
+	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, &target->thread.regs->gpr[0]);
 }
 
 static int gpr32_set(struct task_struct *target,
@@ -2245,7 +2220,11 @@ static int gpr32_set(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     const void *kbuf, const void __user *ubuf)
 {
-	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, 0);
+	if (target->thread.regs == NULL)
+		return -EIO;
+
+	CHECK_FULL_REGS(target->thread.regs);
+	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, &target->thread.regs->gpr[0]);
 }
 
 /*


Thanks,
- Simon

Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.

[Michael Ellerman]

Simon Guo <wei.guo.simon@gmail.com> writes:
> Thanks for the feedback regarding too many ifdefs.  Is following implemention 
> better for this issue?

Yes that looks much better. Can you send a proper patch with a change
log and so on, thanks.

cheers