Remove kChildCleanExit since it's not used anymore.

Remove kChildCleanExit since it's not used anymore.

R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260304

[Peter Kasting, 2014-03-28 20:24:24]

LGTM

[earthdok, 2014-04-03 07:40:18]

On 2014/03/28 20:24:24, Peter Kasting wrote:
> LGTM

ASan (and TSan) relies on this flag for leak detection and coverage analysis.
Those are tasks that happen in an atexit() handler, and without
"--child-clean-exit" the subprocess may be killed before they are completed. (We
don't currently set this flag on any buildbots, but ClusterFuzz uses it.)

One simpler option we have here is to do the following under
#ifdef(ADDRESS_SANITIZER): set
ChildProcessLauncher::terminate_child_on_shutdown_ to false and skip the
_exit(0) call in SuicideOnChannelErrorFilter. However, I think that the "kill
after 60 seconds" behavior should be preserved as well, especially given that we
ship an ASan canary. In this light, keeping this flag seems to be the best
option.

[earthdok, 2014-04-03 17:35:58]

On 2014/04/03 07:40:18, earthdok wrote:
> On 2014/03/28 20:24:24, Peter Kasting wrote:
> > LGTM
> 
> ASan (and TSan) relies on this flag for leak detection and coverage analysis.

To clarify, "leak detection and coverage" refers to ASan. Under TSan, we use
this flag to a) catch shutdown races and b) make sure TSan prints suppression
statistics for subprocesses (also done in an atexit() handler). This is
according to glider@ who is responsible for TSan deployment.

[Peter Kasting, 2014-04-03 19:59:48]

On 2014/04/03 07:40:18, earthdok wrote:
> On 2014/03/28 20:24:24, Peter Kasting wrote:
> > LGTM
> 
> ASan (and TSan) relies on this flag for leak detection and coverage analysis.
> Those are tasks that happen in an atexit() handler, and without
> "--child-clean-exit" the subprocess may be killed before they are completed.
(We
> don't currently set this flag on any buildbots, but ClusterFuzz uses it.)
> 
> One simpler option we have here is to do the following under
> #ifdef(ADDRESS_SANITIZER): set
> ChildProcessLauncher::terminate_child_on_shutdown_ to false and skip the
> _exit(0) call in SuicideOnChannelErrorFilter. However, I think that the "kill
> after 60 seconds" behavior should be preserved as well, especially given that
we
> ship an ASan canary. In this light, keeping this flag seems to be the best
> option.

It would be really good to avoid flags for ASan/TSan.  Can you also implement
the 60-second kill behavior under an ifdef?  It seems like anything that's
possible with a flag ought to be possible instead with ifdefs, and those would
make it very clear who actually uses this and why.

[earthdok, 2014-04-03 20:43:19]

On 2014/04/03 19:59:48, Peter Kasting wrote:
> It would be really good to avoid flags for ASan/TSan.  Can you also implement
> the 60-second kill behavior under an ifdef?  It seems like anything that's
> possible with a flag ought to be possible instead with ifdefs, and those would
> make it very clear who actually uses this and why.

+glider

Alex, would you be ok with forcing --child-clean-exit in all TSan builds?

[Alexander Potapenko, 2014-04-04 04:20:17]

Yes, that'll be fine.
On Apr 4, 2014 12:43 AM, <earthdok@chromium.org> wrote:

> On 2014/04/03 19:59:48, Peter Kasting wrote:
>
>> It would be really good to avoid flags for ASan/TSan.  Can you also
>> implement
>> the 60-second kill behavior under an ifdef?  It seems like anything that's
>> possible with a flag ought to be possible instead with ifdefs, and those
>> would
>> make it very clear who actually uses this and why.
>>
>
> +glider
>
> Alex, would you be ok with forcing --child-clean-exit in all TSan builds?
>
> https://codereview.chromium.org/217403002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-04-04 04:23:10]

On 2014/04/03 07:40:18, earthdok wrote:
> On 2014/03/28 20:24:24, Peter Kasting wrote:
> > LGTM
> 
> ASan (and TSan) relies on this flag for leak detection and coverage analysis.
> Those are tasks that happen in an atexit() handler, and without
> "--child-clean-exit" the subprocess may be killed before they are completed.
(We
> don't currently set this flag on any buildbots, but ClusterFuzz uses it.)
> 
> One simpler option we have here is to do the following under
> #ifdef(ADDRESS_SANITIZER): set
> ChildProcessLauncher::terminate_child_on_shutdown_ to false and skip the
> _exit(0) call in SuicideOnChannelErrorFilter. However, I think that the "kill
> after 60 seconds" behavior should be preserved as well, especially given that
we
> ship an ASan canary. In this light, keeping this flag seems to be the best
> option.

can you explain to me how i could have seen that asan uses this? I searched for
"child-clean-exit" in code search but didn't find any hits.

how will someone in the future know not to remove this flag when they don't see
any usage of it in the codebase?

[Peter Kasting, 2014-04-04 04:37:27]

On 2014/04/04 04:23:10, jam wrote:
> can you explain to me how i could have seen that asan uses this? I searched
for
> "child-clean-exit" in code search but didn't find any hits.
> 
> how will someone in the future know not to remove this flag when they don't
see
> any usage of it in the codebase?

That's one of the reasons why I would prefer that this functionality be
reintroduced as "#if defined(ADDRESS_SANITIZER)" or some such instead of as a
flag -- it documents the flag's use.  It also limits its scope, which means for
example that the binaries we ship to stable won't have to have the (tiny amount
of) code for this feature in them, and if some  other team wants to use the
functionality, they'll have to change the code to make that feasible, hopefully
making it clear at the same time.

[Alexander Potapenko, 2014-04-04 08:04:10]

+inferno FYI

[earthdok, 2014-04-04 13:17:03]

On 2014/04/04 04:23:10, jam wrote:
> can you explain to me how i could have seen that asan uses this? I searched
for
> "child-clean-exit" in code search but didn't find any hits.

The features of ASan that require this flag are still undergoing deployment. In
fact I was about to add this flag to ASan bot configs.

I will submit a CL to put this code under an #ifdef.

[jochen (gone - plz use gerrit), 2014-08-01 12:01:34]

On 2014/04/04 at 13:17:03, earthdok wrote:
> On 2014/04/04 04:23:10, jam wrote:
> > can you explain to me how i could have seen that asan uses this? I searched
for
> > "child-clean-exit" in code search but didn't find any hits.
> 
> The features of ASan that require this flag are still undergoing deployment.
In fact I was about to add this flag to ASan bot configs.
> 
> I will submit a CL to put this code under an #ifdef.

what's the latest and greatest to get --child-clean-exit behavior these days?