Suppress valgrind cond/value4 error in PowerMenuButton

Suppress valgrind cond/value4 error in PowerMenuButton

TBR=satorux@chromium.org
BUG=101867
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107530

[jam, 2011-10-27 04:28:30]

why suppress this instead of reverting
http://codereview.chromium.org/8347016 ?

On Wed, Oct 26, 2011 at 9:18 PM, <oshima@chromium.org> wrote:

> Reviewers: satorux1,
>
> Description:
> Suppress valgrind cond/value4 error in PowerMenuButton
>
> TBR=satorux@chromium.org
> BUG=101867
> TEST=none
>
>
> Please review this at
http://codereview.chromium.**org/8400018/<http://codereview.chromium.org/8400...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M tools/valgrind/memcheck/**suppressions.txt
>
>
> Index: tools/valgrind/memcheck/**suppressions.txt
> diff --git a/tools/valgrind/memcheck/**suppressions.txt
> b/tools/valgrind/memcheck/**suppressions.txt
> index afd3ba6f52bb459a03d575f5a5a016**1f53a59db8..**
> ebea6a6b8dab7b5460c855096fbf85**3926b9f296 100644
> --- a/tools/valgrind/memcheck/**suppressions.txt
> +++ b/tools/valgrind/memcheck/**suppressions.txt
> @@ -5060,6 +5060,25 @@
>    fun:_**ZN7testing8internal11CmpHelper**GEIddEENS_**
> 15AssertionResultEPKcS4_RKT_**RKT0_
>    fun:_ZN3gfx31JPEGCodec_**EncodeDecodeRGBA_**Test8TestBodyEv
>  }
> +
> +{
> +   bug_101867_a
> +   Memcheck:Cond
> +   ...
> +   fun:_**ZN8chromeos15PowerMenuButton22**UpdateIconAndLabelInfoEv
> +   fun:_**ZN8chromeos15PowerMenuButtonC1**EPNS_14StatusAreaHostE
> +   fun:_**ZN8chromeos14StatusAreaView4In**itEv
> +   fun:_**ZN8chromeos11BrowserView4InitE**v
> +}
> +{
> +   bug_101867_b
> +   Memcheck:Value4
> +   fun:_**ZN8chromeos15PowerMenuButton22**UpdateIconAndLabelInfoEv
> +   fun:_**ZN8chromeos15PowerMenuButtonC1**EPNS_14StatusAreaHostE
> +   fun:_**ZN8chromeos14StatusAreaView4In**itEv
> +   fun:_**ZN8chromeos11BrowserView4InitE**v
> +}
> +
>  #-----------------------------**------------------------------**
> ------------
>  # 4. These only occur on our Google workstations
>  {
>
>
>

[jam, 2011-10-27 04:35:13]

to expand, this seems like a pretty serious bug, so why not just revert
instead of adding a suppression?

UninitCondition
Conditional jump or move depends on uninitialised value(s)


On Wed, Oct 26, 2011 at 9:28 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> why suppress this instead of reverting
> http://codereview.chromium.org/8347016 ?
>
>
> On Wed, Oct 26, 2011 at 9:18 PM, <oshima@chromium.org> wrote:
>
>> Reviewers: satorux1,
>>
>> Description:
>> Suppress valgrind cond/value4 error in PowerMenuButton
>>
>> TBR=satorux@chromium.org
>> BUG=101867
>> TEST=none
>>
>>
>> Please review this at
http://codereview.chromium.**org/8400018/<http://codereview.chromium.org/8400...
>>
>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>>
>> Affected files:
>>  M tools/valgrind/memcheck/**suppressions.txt
>>
>>
>> Index: tools/valgrind/memcheck/**suppressions.txt
>> diff --git a/tools/valgrind/memcheck/**suppressions.txt
>> b/tools/valgrind/memcheck/**suppressions.txt
>> index afd3ba6f52bb459a03d575f5a5a016**1f53a59db8..**
>> ebea6a6b8dab7b5460c855096fbf85**3926b9f296 100644
>> --- a/tools/valgrind/memcheck/**suppressions.txt
>> +++ b/tools/valgrind/memcheck/**suppressions.txt
>> @@ -5060,6 +5060,25 @@
>>    fun:_**ZN7testing8internal11CmpHelper**GEIddEENS_**
>> 15AssertionResultEPKcS4_RKT_**RKT0_
>>    fun:_ZN3gfx31JPEGCodec_**EncodeDecodeRGBA_**Test8TestBodyEv
>>  }
>> +
>> +{
>> +   bug_101867_a
>> +   Memcheck:Cond
>> +   ...
>> +   fun:_**ZN8chromeos15PowerMenuButton22**UpdateIconAndLabelInfoEv
>> +   fun:_**ZN8chromeos15PowerMenuButtonC1**EPNS_14StatusAreaHostE
>> +   fun:_**ZN8chromeos14StatusAreaView4In**itEv
>> +   fun:_**ZN8chromeos11BrowserView4InitE**v
>> +}
>> +{
>> +   bug_101867_b
>> +   Memcheck:Value4
>> +   fun:_**ZN8chromeos15PowerMenuButton22**UpdateIconAndLabelInfoEv
>> +   fun:_**ZN8chromeos15PowerMenuButtonC1**EPNS_14StatusAreaHostE
>> +   fun:_**ZN8chromeos14StatusAreaView4In**itEv
>> +   fun:_**ZN8chromeos11BrowserView4InitE**v
>> +}
>> +
>>  #-----------------------------**------------------------------**
>> ------------
>>  # 4. These only occur on our Google workstations
>>  {
>>
>>
>>
>

[satorux1, 2011-10-27 05:25:46]

The fix would be to initialize power_status_ in the constructor of
PowerMenuButton. I think sque@ should be able to fix this tomorrow.

[jam, 2011-10-27 05:38:24]

yep I saw that, i was just curious why add a suppression instead of
reverting though.

On Wed, Oct 26, 2011 at 10:25 PM, <satorux@chromium.org> wrote:

> The fix would be to initialize power_status_ in the constructor of
> PowerMenuButton. I think sque@ should be able to fix this tomorrow.
>
>
http://codereview.chromium.**org/8400018/<http://codereview.chromium.org/8400...
>

[oshima, 2011-10-27 05:53:24]

We (memory sheriff) usually don't revert CL for valgrind error (unless it's
causing crash/flaky-ness).
We do, however, set P1 for cond/value error, which I didn't and is my fault.

On 2011/10/27 05:38:24, John Abd-El-Malek wrote:
> yep I saw that, i was just curious why add a suppression instead of
> reverting though.
> 
> On Wed, Oct 26, 2011 at 10:25 PM, <mailto:satorux@chromium.org> wrote:
> 
> > The fix would be to initialize power_status_ in the constructor of
> > PowerMenuButton. I think sque@ should be able to fix this tomorrow.
> >
> >
>
http://codereview.chromium.**org/8400018/%3Chttp://codereview.chromium.org/84...>
> >

[satorux1, 2011-10-27 06:03:38]

sque created a fix, and will submit soon:


http://codereview.chromium.org/8402020/

On Wed, Oct 26, 2011 at 10:53 PM, <oshima@chromium.org> wrote:

> We (memory sheriff) usually don't revert CL for valgrind error (unless  
> it's
> causing crash/flaky-ness).
> We do, however, set P1 for cond/value error, which I didn't and is my
> fault.


> On 2011/10/27 05:38:24, John Abd-El-Malek wrote:

>> yep I saw that, i was just curious why add a suppression instead of
>> reverting though.


>   On Wed, Oct 26, 2011 at 10:25 PM, <mailto:satorux@chromium.org> wrote:


>   > The fix would be to initialize power_status_ in the constructor of
>> > PowerMenuButton. I think sque@ should be able to fix this tomorrow.
>> >
>> >


> http://codereview.chromium.****org/8400018/%3Chttp://coderevi**
> ew.chromium.org/8400018/ <http://codereview.chromium.org/8400018/>>

>> >




>
http://codereview.chromium.**org/8400018/<http://codereview.chromium.org/8400...

[jam, 2011-10-27 06:10:49]

i see, ok thanks for the explanation. in some changes which cause
corruption, i've been asked to revert in the past (which i agree makes
sense).

On Wed, Oct 26, 2011 at 10:53 PM, <oshima@chromium.org> wrote:

> We (memory sheriff) usually don't revert CL for valgrind error (unless it's
> causing crash/flaky-ness).
> We do, however, set P1 for cond/value error, which I didn't and is my
> fault.
>
>
> On 2011/10/27 05:38:24, John Abd-El-Malek wrote:
>
>> yep I saw that, i was just curious why add a suppression instead of
>> reverting though.
>>
>
>  On Wed, Oct 26, 2011 at 10:25 PM, <mailto:satorux@chromium.org> wrote:
>>
>
>  > The fix would be to initialize power_status_ in the constructor of
>> > PowerMenuButton. I think sque@ should be able to fix this tomorrow.
>> >
>> >
>>
>
> http://codereview.chromium.****org/8400018/%3Chttp://coderevi**
> ew.chromium.org/8400018/ <http://codereview.chromium.org/8400018/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8400018/<http://codereview.chromium.org/8400...
>