Mac: Fix broken valgrind suppressions.

Mac: Fix broken valgrind suppressions.

They don't work locally for me, and on the bots only Mac_Sandbox_Intentional_Leak3 is getting used, which is overly wide.

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

[Nico, 2010-07-29 16:39:59]

(Ignore the inline diff, there's a new one at codereview)

On Thu, Jul 29, 2010 at 9:37 AM,  <thakis@chromium.org> wrote:
> Reviewers: rsesek, Lei Zhang,
>
> Description:
> Mac: Fix broken valgrind suppressions.
>
> They don't work locally for me, and on the bots only
> Mac_Sandbox_Intentional_Leak3 is getting used, which is overly wide.
>
> Please review this at http://codereview.chromium.org/2808079/show
>
> Affected files:
>  M chrome/common/sandbox_mac_unittest_helper.h
>  M tools/valgrind/memcheck/suppressions_mac.txt
>
>
> Index: chrome/common/sandbox_mac_unittest_helper.h
> diff --git a/chrome/common/sandbox_mac_unittest_helper.h
> b/chrome/common/sandbox_mac_unittest_helper.h
> index
>
3edb7c4a90f4fe6dcfa0ca227969e7adc919652e..574ed9ed6ea1f8549765ac19accc7cd9abb5ad45
> 100644
> --- a/chrome/common/sandbox_mac_unittest_helper.h
> +++ b/chrome/common/sandbox_mac_unittest_helper.h
> @@ -15,7 +15,7 @@ namespace sandboxtest {
>  //
>  // How to write a sandboxed test:
>  // 1. Create a class that inherits from MacSandboxTestCase and overrides
> -// it's functions to run code before or after the sandbox is initialised in
> a
> +// its functions to run code before or after the sandbox is initialised in
> a
>  // subprocess.
>  // 2. Register the class you just created with the
> REGISTER_SANDBOX_TEST_CASE()
>  // macro.
> Index: tools/valgrind/memcheck/suppressions_mac.txt
> diff --git a/tools/valgrind/memcheck/suppressions_mac.txt
> b/tools/valgrind/memcheck/suppressions_mac.txt
> index
>
426b0cb162568c848dd4c8f5b5831f9bbec3884d..de3928a6e1fceecbb9ec2365d4e88d549dd2dd92
> 100644
> --- a/tools/valgrind/memcheck/suppressions_mac.txt
> +++ b/tools/valgrind/memcheck/suppressions_mac.txt
> @@ -212,7 +212,6 @@
>    Mac_Sandbox_Intentional_Leak1
>    Memcheck:Leak
>    fun:_Znw*
> -
>
fun:_ZN11sandboxtest8internal19RegisterSandboxTestIN12_GLOBAL__N_*MacSandboxed*TestCaseEEC2EPKc
>
>
 fun:_ZN11sandboxtest8internal19RegisterSandboxTestIN12_GLOBAL__N_*MacSandboxed*TestCaseEEC1EPKc
>    fun:_Z41__static_initialization_and_destruction_0ii
>    fun:_GLOBAL__I__ZNSt3tr112_GLOBAL__N_16ignoreE
> @@ -220,16 +219,13 @@
>    fun:_ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEj
>    fun:_ZN11ImageLoader15runInitializersERKNS_11LinkContextE
>    fun:_ZN4dyld24initializeMainExecutableEv
> -   fun:_ZN4dyld5_mainEPK11mach_headermiPPKcS5_S5_
> -   fun:_ZN13dyldbootstrap5startEPK11mach_headeriPPKcl
> -   fun:_dyld_start
> +   ...
>    obj:*
>  }
>  {
>    Mac_Sandbox_Intentional_Leak2
>    Memcheck:Leak
>    fun:_Znw*
> -
>
fun:_ZN11sandboxtest8internal19RegisterSandboxTestIN12_GLOBAL__N_119FontLoadingTestCaseEEC2EPKc
>
>
 fun:_ZN11sandboxtest8internal19RegisterSandboxTestIN12_GLOBAL__N_119FontLoadingTestCaseEEC1EPKc
>    fun:_Z41__static_initialization_and_destruction_0ii
>    fun:_GLOBAL__I__ZNSt3tr112_GLOBAL__N_16ignoreE
> @@ -237,9 +233,7 @@
>    fun:_ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEj
>    fun:_ZN11ImageLoader15runInitializersERKNS_11LinkContextE
>    fun:_ZN4dyld24initializeMainExecutableEv
> -   fun:_ZN4dyld5_mainEPK11mach_headermiPPKcS5_S5_
> -   fun:_ZN13dyldbootstrap5startEPK11mach_headeriPPKcl
> -   fun:_dyld_start
> +   ...
>    obj:*
>  }
>  {
> @@ -253,9 +247,7 @@
>    fun:_ZN11ImageLoader23recursiveInitializationERKNS_11LinkContextEj
>    fun:_ZN11ImageLoader15runInitializersERKNS_11LinkContextE
>    fun:_ZN4dyld24initializeMainExecutableEv
> -   fun:_ZN4dyld5_mainEPK11mach_headermiPPKcS5_S5_
> -   fun:_ZN13dyldbootstrap5startEPK11mach_headeriPPKcl
> -   fun:_dyld_start
> +   ...
>    obj:*
>  }
>  {
>
>
>

[Robert Sesek, 2010-07-29 16:42:35]

http://codereview.chromium.org/2808079/diff/3001/4002
File tools/valgrind/memcheck/suppressions_mac.txt (left):

http://codereview.chromium.org/2808079/diff/3001/4002#oldcode251
tools/valgrind/memcheck/suppressions_mac.txt:251:
fun:_Z41__static_initialization_and_destruction_0ii
Is this suppression still needed? Your modification to Intentional_Leak1 isn't
enough to cover this.

[Nico, 2010-07-29 16:48:56]

http://codereview.chromium.org/2808079/diff/3001/4002
File tools/valgrind/memcheck/suppressions_mac.txt (left):

http://codereview.chromium.org/2808079/diff/3001/4002#oldcode251
tools/valgrind/memcheck/suppressions_mac.txt:251:
fun:_Z41__static_initialization_and_destruction_0ii
On 2010/07/29 16:42:35, rsesek wrote:
> Is this suppression still needed? Your modification to Intentional_Leak1 isn't
> enough to cover this.

do we want a suppression for all statically initialized things? the name
suggests that this was for the sandbox test stuff, so if my new suppression
doesn't cover enough, i will add more focused suppressions for other things.

also removed obj:* lik requested by glider

[Robert Sesek, 2010-07-29 16:54:26]

LGTM

http://codereview.chromium.org/2808079/diff/3001/4002
File tools/valgrind/memcheck/suppressions_mac.txt (left):

http://codereview.chromium.org/2808079/diff/3001/4002#oldcode251
tools/valgrind/memcheck/suppressions_mac.txt:251:
fun:_Z41__static_initialization_and_destruction_0ii
On 2010/07/29 16:48:56, Nico wrote:
> On 2010/07/29 16:42:35, rsesek wrote:
> > Is this suppression still needed? Your modification to Intentional_Leak1
isn't
> > enough to cover this.
> 
> do we want a suppression for all statically initialized things? the name
> suggests that this was for the sandbox test stuff, so if my new suppression
> doesn't cover enough, i will add more focused suppressions for other things.
> 
> also removed obj:* lik requested by glider

OK

[Lei Zhang, 2010-07-29 18:47:12]

Hrm. Already committed.

The story goes like this:
a. Chromium Mac was running fine.
b. We had some intentional leaks -> suppressions 1 and 2
c. Chromium Mac no longer links.
d. We had some hacks to make it all work
e. The stack trace for the intentional leaks changed -> suppression 3

but I kept 1 and 2 around for when we upgrade to 10.6, then maybe we won't need
the special hacks and we'd just go back to using suppressions 1 and 2.

[Nico, 2010-07-29 19:32:40]

On Thu, Jul 29, 2010 at 11:47 AM,  <thestig@chromium.org> wrote:
> Hrm. Already committed.
>
> The story goes like this:
> a. Chromium Mac was running fine.
> b. We had some intentional leaks -> suppressions 1 and 2
> c. Chromium Mac no longer links.
> d. We had some hacks to make it all work
> e. The stack trace for the intentional leaks changed -> suppression 3

But suppression 3 suppresses _everything_ allocated by static
initalizers. Isn't that a bit much?

>
> but I kept 1 and 2 around for when we upgrade to 10.6, then maybe we won't
> need
> the special hacks and we'd just go back to using suppressions 1 and 2.
>
> http://codereview.chromium.org/2808079/show
>

[Lei Zhang, 2010-07-29 19:38:40]

On 2010/07/29 19:32:40, Nico wrote:
> On Thu, Jul 29, 2010 at 11:47 AM,  <mailto:thestig@chromium.org> wrote:
> > Hrm. Already committed.
> >
> > The story goes like this:
> > a. Chromium Mac was running fine.
> > b. We had some intentional leaks -> suppressions 1 and 2
> > c. Chromium Mac no longer links.
> > d. We had some hacks to make it all work
> > e. The stack trace for the intentional leaks changed -> suppression 3
> 
> But suppression 3 suppresses _everything_ allocated by static
> initalizers. Isn't that a bit much?

That's how it shows up on the Mac Valgrind bot. If you look at:
http://build.chromium.org/buildbot/memory/builders/Chromium%20Mac%20(valgrind...

you'll see suppression 3 no longer gets hit and we go red instead.

[Nico, 2010-07-29 20:34:41]

On Thu, Jul 29, 2010 at 12:38 PM,  <thestig@chromium.org> wrote:
> On 2010/07/29 19:32:40, Nico wrote:
>>
>> On Thu, Jul 29, 2010 at 11:47 AM,  <mailto:thestig@chromium.org> wrote:
>> > Hrm. Already committed.
>> >
>> > The story goes like this:
>> > a. Chromium Mac was running fine.
>> > b. We had some intentional leaks -> suppressions 1 and 2
>> > c. Chromium Mac no longer links.
>> > d. We had some hacks to make it all work
>> > e. The stack trace for the intentional leaks changed -> suppression 3
>
>> But suppression 3 suppresses _everything_ allocated by static
>> initalizers. Isn't that a bit much?
>
> That's how it shows up on the Mac Valgrind bot. If you look at:
>
http://build.chromium.org/buildbot/memory/builders/Chromium%20Mac%20(valgrind...
>
> you'll see suppression 3 no longer gets hit and we go red instead.

:-(

I'll revert that part of this CL then.

>
> http://codereview.chromium.org/2808079/show
>

[Nico, 2010-07-29 20:47:22]

On Thu, Jul 29, 2010 at 1:34 PM, Nico Weber <thakis@chromium.org> wrote:
> On Thu, Jul 29, 2010 at 12:38 PM,  <thestig@chromium.org> wrote:
>> On 2010/07/29 19:32:40, Nico wrote:
>>>
>>> On Thu, Jul 29, 2010 at 11:47 AM,  <mailto:thestig@chromium.org> wrote:
>>> > Hrm. Already committed.
>>> >
>>> > The story goes like this:
>>> > a. Chromium Mac was running fine.
>>> > b. We had some intentional leaks -> suppressions 1 and 2
>>> > c. Chromium Mac no longer links.
>>> > d. We had some hacks to make it all work
>>> > e. The stack trace for the intentional leaks changed -> suppression 3
>>
>>> But suppression 3 suppresses _everything_ allocated by static
>>> initalizers. Isn't that a bit much?
>>
>> That's how it shows up on the Mac Valgrind bot. If you look at:
>>
http://build.chromium.org/buildbot/memory/builders/Chromium%20Mac%20(valgrind...
>>
>> you'll see suppression 3 no longer gets hit and we go red instead.
>
> :-(
>
> I'll revert that part of this CL then.

http://codereview.chromium.org/3056032

>
>>
>> http://codereview.chromium.org/2808079/show
>>
>