linux-next: build warning in Linus' tree

linux-next: build warning in Linus' tree

[Stephen Rothwell]

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

Hi all,

In building Linus' tree, today's linux-next build (x86_64 allmodconfig)
produced this warning:

arch/x86/oprofile/op_model_amd.c: In function 'ibs_eilvt_valid':
arch/x86/oprofile/op_model_amd.c:289: warning: 'offset' may be used uninitialized in this function

.. and indeed it may be.

Introduced by commit 27afdf2008da0b8878a73e32e4eb12381b84e224 ("apic,
x86: Use BIOS settings for IBS and MCE threshold interrupt LVT
offsets").  Sorry I missed this earlier.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: linux-next: build warning in Linus' tree

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi all,
> 
> In building Linus' tree, today's linux-next build (x86_64 allmodconfig)
> produced this warning:
> 
> arch/x86/oprofile/op_model_amd.c: In function 'ibs_eilvt_valid':
> arch/x86/oprofile/op_model_amd.c:289: warning: 'offset' may be used uninitialized in this function
> 
> .. and indeed it may be.

Yep, that looks bogus. I've queued up the fix.

Thanks,

	Ingo

[tip:perf/urgent] x86/oprofile: Fix uninitialized variable use in debug printk

[tip-bot for Ingo Molnar]

Commit-ID:  2c78ffeca98fcd5a1dfd4a322438944506ed5e64
Gitweb:     http://git.kernel.org/tip/2c78ffeca98fcd5a1dfd4a322438944506ed5e64
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Mon, 25 Oct 2010 08:41:09 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 25 Oct 2010 08:46:20 +0200

x86/oprofile: Fix uninitialized variable use in debug printk

Stephen Rothwell reported this build warning:

  arch/x86/oprofile/op_model_amd.c: In function 'ibs_eilvt_valid':
  arch/x86/oprofile/op_model_amd.c:289: warning: 'offset' may be used uninitialized in this function

And correctly observed that indeed the variable is used uninitialized in
this function. The result of this bug can be a debug printk with a bogus
value.

Also fix a few more small details that made this function hard to read
and which probably contributed to the bug being introduced to begin with:

 - Use more symmetric error conditions

 - Remove the !0 obfuscation

 - Add newlines to the printk output

 - Remove bogus linebreaks in printk strings and elsewhere

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Robert Richter <robert.richter@amd.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
LKML-Reference: <20101025115736.41d51abe.sfr@canb.auug.org.au>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/oprofile/op_model_amd.c |   26 +++++++++++---------------
 1 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/arch/x86/oprofile/op_model_amd.c b/arch/x86/oprofile/op_model_amd.c
index 42fb46f..68759e7 100644
--- a/arch/x86/oprofile/op_model_amd.c
+++ b/arch/x86/oprofile/op_model_amd.c
@@ -281,29 +281,25 @@ static inline int eilvt_is_available(int offset)
 
 static inline int ibs_eilvt_valid(void)
 {
-	u64 val;
 	int offset;
+	u64 val;
 
 	rdmsrl(MSR_AMD64_IBSCTL, val);
+	offset = val & IBSCTL_LVT_OFFSET_MASK;
+
 	if (!(val & IBSCTL_LVT_OFFSET_VALID)) {
-		pr_err(FW_BUG "cpu %d, invalid IBS "
-		       "interrupt offset %d (MSR%08X=0x%016llx)",
-		       smp_processor_id(), offset,
-		       MSR_AMD64_IBSCTL, val);
+		pr_err(FW_BUG "cpu %d, invalid IBS interrupt offset %d (MSR%08X=0x%016llx)\n",
+		       smp_processor_id(), offset, MSR_AMD64_IBSCTL, val);
 		return 0;
 	}
 
-	offset = val & IBSCTL_LVT_OFFSET_MASK;
-
-	if (eilvt_is_available(offset))
-		return !0;
-
-	pr_err(FW_BUG "cpu %d, IBS interrupt offset %d "
-	       "not available (MSR%08X=0x%016llx)",
-	       smp_processor_id(), offset,
-	       MSR_AMD64_IBSCTL, val);
+	if (!eilvt_is_available(offset)) {
+		pr_err(FW_BUG "cpu %d, IBS interrupt offset %d not available (MSR%08X=0x%016llx)\n",
+		       smp_processor_id(), offset, MSR_AMD64_IBSCTL, val);
+		return 0;
+	}
 
-	return 0;
+	return 1;
 }
 
 static inline int get_ibs_offset(void)

Re: [tip:perf/urgent] x86/oprofile: Fix uninitialized variable use in debug printk

[Robert Richter]

On 25.10.10 02:52:03, tip-bot for Ingo Molnar wrote:
> Commit-ID:  2c78ffeca98fcd5a1dfd4a322438944506ed5e64
> Gitweb:     http://git.kernel.org/tip/2c78ffeca98fcd5a1dfd4a322438944506ed5e64
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Mon, 25 Oct 2010 08:41:09 +0200
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Mon, 25 Oct 2010 08:46:20 +0200
> 
> x86/oprofile: Fix uninitialized variable use in debug printk
> 
> Stephen Rothwell reported this build warning:
> 
>   arch/x86/oprofile/op_model_amd.c: In function 'ibs_eilvt_valid':
>   arch/x86/oprofile/op_model_amd.c:289: warning: 'offset' may be used uninitialized in this function
> 
> And correctly observed that indeed the variable is used uninitialized in
> this function. The result of this bug can be a debug printk with a bogus
> value.
> 
> Also fix a few more small details that made this function hard to read
> and which probably contributed to the bug being introduced to begin with:
> 
>  - Use more symmetric error conditions
> 
>  - Remove the !0 obfuscation
> 
>  - Add newlines to the printk output
> 
>  - Remove bogus linebreaks in printk strings and elsewhere

Ingo, thanks for changing this, it all looks good to me, though we
actually don't have to print the offset as the valid bit is not set.

For some reason I didn't catch the warning:

...
  LD      arch/x86/kernel/acpi/built-in.o
  CC      arch/x86/oprofile/op_model_amd.o
  CC      arch/x86/kernel/cpu/intel_cacheinfo.o
...

(using gcc (Gentoo 4.4.3-r2 p1.2) 4.4.3)

... but it should warn.

-Robert

> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Robert Richter <robert.richter@amd.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> LKML-Reference: <20101025115736.41d51abe.sfr@canb.auug.org.au>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [tip:perf/urgent] x86/oprofile: Fix uninitialized variable use in debug printk

[Ingo Molnar]

* Robert Richter <robert.richter@amd.com> wrote:

> On 25.10.10 02:52:03, tip-bot for Ingo Molnar wrote:
> > Commit-ID:  2c78ffeca98fcd5a1dfd4a322438944506ed5e64
> > Gitweb:     http://git.kernel.org/tip/2c78ffeca98fcd5a1dfd4a322438944506ed5e64
> > Author:     Ingo Molnar <mingo@elte.hu>
> > AuthorDate: Mon, 25 Oct 2010 08:41:09 +0200
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Mon, 25 Oct 2010 08:46:20 +0200
> > 
> > x86/oprofile: Fix uninitialized variable use in debug printk
> > 
> > Stephen Rothwell reported this build warning:
> > 
> >   arch/x86/oprofile/op_model_amd.c: In function 'ibs_eilvt_valid':
> >   arch/x86/oprofile/op_model_amd.c:289: warning: 'offset' may be used uninitialized in this function
> > 
> > And correctly observed that indeed the variable is used uninitialized in
> > this function. The result of this bug can be a debug printk with a bogus
> > value.
> > 
> > Also fix a few more small details that made this function hard to read
> > and which probably contributed to the bug being introduced to begin with:
> > 
> >  - Use more symmetric error conditions
> > 
> >  - Remove the !0 obfuscation
> > 
> >  - Add newlines to the printk output
> > 
> >  - Remove bogus linebreaks in printk strings and elsewhere
> 
> Ingo, thanks for changing this, it all looks good to me, though we actually don't 
> have to print the offset as the valid bit is not set.

Yeah - thought of that, but didnt want to change behavior.

> For some reason I didn't catch the warning:
> 
> ...
>   LD      arch/x86/kernel/acpi/built-in.o
>   CC      arch/x86/oprofile/op_model_amd.o
>   CC      arch/x86/kernel/cpu/intel_cacheinfo.o
> ...
> 
> (using gcc (Gentoo 4.4.3-r2 p1.2) 4.4.3)
> 
> ... but it should warn.

Weird - should be very obvious for a compiler to see that.

	Ingo