* [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common. @ 2016-08-20 12:24 wei.guo.simon 2016-08-24 2:21 ` Daniel Axtens 0 siblings, 1 reply; 7+ messages in thread From: wei.guo.simon @ 2016-08-20 12:24 UTC (permalink / raw) To: linuxppc-dev; +Cc: Michael Ellerman, Anshuman Khandual, Simon Guo 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common. 2016-08-20 12:24 [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common wei.guo.simon @ 2016-08-24 2:21 ` Daniel Axtens 2016-08-24 7:38 ` Simon Guo 0 siblings, 1 reply; 7+ messages in thread From: Daniel Axtens @ 2016-08-24 2:21 UTC (permalink / raw) To: wei.guo.simon, linuxppc-dev; +Cc: Simon Guo, Anshuman Khandual [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common. 2016-08-24 2:21 ` Daniel Axtens @ 2016-08-24 7:38 ` Simon Guo 2016-08-25 1:06 ` Daniel Axtens 0 siblings, 1 reply; 7+ messages in thread From: Simon Guo @ 2016-08-24 7:38 UTC (permalink / raw) To: Daniel Axtens; +Cc: linuxppc-dev, Anshuman Khandual 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common. 2016-08-24 7:38 ` Simon Guo @ 2016-08-25 1:06 ` Daniel Axtens 2016-09-09 10:52 ` Michael Ellerman 0 siblings, 1 reply; 7+ messages in thread From: Daniel Axtens @ 2016-08-25 1:06 UTC (permalink / raw) To: Simon Guo; +Cc: linuxppc-dev, Anshuman Khandual [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common. 2016-08-25 1:06 ` Daniel Axtens @ 2016-09-09 10:52 ` Michael Ellerman 2016-09-11 12:07 ` Simon Guo 0 siblings, 1 reply; 7+ messages in thread From: Michael Ellerman @ 2016-09-09 10:52 UTC (permalink / raw) To: Daniel Axtens, Simon Guo; +Cc: linuxppc-dev, Anshuman Khandual 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common. 2016-09-09 10:52 ` Michael Ellerman @ 2016-09-11 12:07 ` Simon Guo 2016-09-12 1:55 ` Michael Ellerman 0 siblings, 1 reply; 7+ messages in thread From: Simon Guo @ 2016-09-11 12:07 UTC (permalink / raw) To: Michael Ellerman; +Cc: Daniel Axtens, linuxppc-dev, Anshuman Khandual 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common. 2016-09-11 12:07 ` Simon Guo @ 2016-09-12 1:55 ` Michael Ellerman 0 siblings, 0 replies; 7+ messages in thread From: Michael Ellerman @ 2016-09-12 1:55 UTC (permalink / raw) To: Simon Guo; +Cc: Daniel Axtens, linuxppc-dev, Anshuman Khandual 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-12 1:55 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-20 12:24 [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common wei.guo.simon 2016-08-24 2:21 ` Daniel Axtens 2016-08-24 7:38 ` Simon Guo 2016-08-25 1:06 ` Daniel Axtens 2016-09-09 10:52 ` Michael Ellerman 2016-09-11 12:07 ` Simon Guo 2016-09-12 1:55 ` Michael Ellerman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.