| Message ID | 1469674679-8580-7-git-send-email-wei.guo.simon@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi all, This is causing cppcheck warnings (having just landed in next): [arch/powerpc/kernel/ptrace.c:2062]: (error) Uninitialized variable: ckpt_regs [arch/powerpc/kernel/ptrace.c:2130]: (error) Uninitialized variable: ckpt_regs This is from... > -static int gpr32_get(struct task_struct *target, > +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) > + void *kbuf, void __user *ubuf, bool tm_active) > { > 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; > > - if (target->thread.regs == NULL) > - return -EIO; > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > + ckpt_regs = &target->thread.ckpt_regs.gpr[0]; > +#endif > + if (tm_active) { > + regs = ckpt_regs; ... this bit here. If the ifdef doesn't trigger, cppcheck can't find an initialisation for ckpt_regs, so it complains. Techinically it's a false positive as (I assume!) tm_active cannot ever be true in the absense of CONFIG_PPC_TRANSACTIONAL_MEM. Is there a nice simple fix we could deploy to squash this warning, or will we just live with it? > -static int gpr32_set(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) > + const void *kbuf, const void __user *ubuf, bool tm_active) > { > 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; > > - if (target->thread.regs == NULL) > - return -EIO; > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > + ckpt_regs = &target->thread.ckpt_regs.gpr[0]; > +#endif > > - CHECK_FULL_REGS(target->thread.regs); > + if (tm_active) { > + regs = ckpt_regs; FWIW it happens again here. Regards, Daniel Axtens
Daniel Axtens <dja@axtens.net> writes: > [ Unknown signature status ] > Hi all, > > This is causing cppcheck warnings (having just landed in next): > > [arch/powerpc/kernel/ptrace.c:2062]: (error) Uninitialized variable: ckpt_regs > [arch/powerpc/kernel/ptrace.c:2130]: (error) Uninitialized variable: ckpt_regs Sigh. > This is from... >> -static int gpr32_get(struct task_struct *target, >> +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) >> + void *kbuf, void __user *ubuf, bool tm_active) >> { >> 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; >> >> - if (target->thread.regs == NULL) >> - return -EIO; >> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> + ckpt_regs = &target->thread.ckpt_regs.gpr[0]; >> +#endif >> + if (tm_active) { >> + regs = ckpt_regs; > ... this bit here. If the ifdef doesn't trigger, cppcheck can't find an > initialisation for ckpt_regs, so it complains. > > Techinically it's a false positive as (I assume!) tm_active cannot ever > be true in the absense of CONFIG_PPC_TRANSACTIONAL_MEM. That's correct, so the code is safe. See the one call site (which passes an int!): #ifdef CONFIG_PPC_TRANSACTIONAL_MEM static int tm_cgpr32_get(struct task_struct *target, const struct user_regset *regset, unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf) { return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 1); } > Is there a nice simple fix we could deploy to squash this warning, or > will we just live with it? This series has been nothing but pain. Given we're already at v13, and people really want this support to go in, I'm going to leave it in the tree. Once it's in we can refactor the implementation, which is a bit of a mess, and hopefully in the process fix the warnings. cheers
Michael Ellerman <mpe@ellerman.id.au> writes: >> Is there a nice simple fix we could deploy to squash this warning, or >> will we just live with it? > > This series has been nothing but pain. Given we're already at v13, and people > really want this support to go in, I'm going to leave it in the tree. > > Once it's in we can refactor the implementation, which is a bit of a mess, and > hopefully in the process fix the warnings. OK, I'll push this onto my stack to look at again in a couple of months. Regards, Daniel
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index ebf3b0e..2cdb4e7 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -907,24 +907,35 @@ static const struct user_regset_view user_ppc_native_view = { #ifdef CONFIG_PPC64 #include <linux/compat.h> -static int gpr32_get(struct task_struct *target, +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) + void *kbuf, void __user *ubuf, bool tm_active) { 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; - if (target->thread.regs == NULL) - return -EIO; +#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; + 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); @@ -964,20 +975,31 @@ static int gpr32_get(struct task_struct *target, PT_REGS_COUNT * sizeof(reg), -1); } -static int gpr32_set(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) + const void *kbuf, const void __user *ubuf, bool tm_active) { 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; - if (target->thread.regs == NULL) - return -EIO; +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM + ckpt_regs = &target->thread.ckpt_regs.gpr[0]; +#endif - CHECK_FULL_REGS(target->thread.regs); + 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); @@ -1037,6 +1059,22 @@ static int gpr32_set(struct task_struct *target, (PT_TRAP + 1) * sizeof(reg), -1); } +static int gpr32_get(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + void *kbuf, void __user *ubuf) +{ + return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 0); +} + +static int gpr32_set(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, 0); +} + /* * These are the regset flavors matching the CONFIG_PPC32 native set. */