Need a flag to control the error exit code in heap checker

[GoogleCodeExporter]

Hi,

We're doing regular heapchecker testing of Chromium, and it's sometimes hard to 
distinguish a failing or crashing test from the leak reports looking at the 
exit code (this is necessary because our tests do have leaks, and some 
post-processing is done on the logs to suppress known leak reports). It would 
be helpful to change the default exit code returned by the heap checker.

Proposed patch:

Index: heap-checker.cc
===================================================================
--- heap-checker.cc (revision 120970)
+++ heap-checker.cc (working copy)
@@ -223,6 +223,10 @@
              " its checks. Report any such issues to the heap-checker"
              " maintainer(s).");

+DEFINE_int32(heap_check_error_exit_code,
+             EnvToInt("HEAP_CHECK_ERROR_EXIT_CODE", 1),
+             "Exit code to return if any leaks were detected.");
+
 //----------------------------------------------------------------------

 DEFINE_string(heap_profile_pprof,
@@ -2076,7 +2080,8 @@
     }
     RAW_LOG(ERROR, "Exiting with error code (instead of crashing) "
                    "because of whole-program memory leaks");
-    _exit(1);    // we don't want to call atexit() routines!
+    // We don't want to call atexit() routines!
+    _exit(FLAGS_heap_check_error_exit_code);
   }
   return true;
 }

Original issue reported on code.google.com by gli...@chromium.org on 16 Feb 2012 at 9:15

[GoogleCodeExporter]

The intended way to handle this is to put a HeapLeakChecker::Disabler object in 
the places with the known leaks, so you don't have to do the post-processing.  
Is that not possible in your case?

Original comment by csilv...@gmail.com on 17 Feb 2012 at 12:33

[GoogleCodeExporter]

It is not.

The suppression file for the leaks reported by the heap checker contains 200+ 
reports so far 
(http://src.chromium.org/viewvc/chrome/trunk/src/tools/heapcheck/suppressions.tx
t?view=markup), and very few of them are known intentional leaks. We're using 
HeapLeakChecker::Disabler for those, but some of the known leaks just weren't 
annotated yet.
In most cases we suppress real bugs that are not fixed, and finding and 
annotating them is somewhat equivalent to fixing.

Another advantage of post-processing the leak reports instead of annotating 
them in the code is that one does not need to change and recompile the code to 
do that. For some third party code it's often the only solution. For example, 
the WebKit revision is bumped several times a week and new leaks are introduced 
really often. The person who watches the buildbots (the memory sheriff) often 
cannot commit code to WebKit. Even if he is able to, his changes don't appear 
on the bot till the next WebKit roll. While the bot is red, the newly 
introduced leaks in Chromium itself won't be noticed and will be harder to 
track down.

Original comment by gli...@chromium.org on 17 Feb 2012 at 8:16

[GoogleCodeExporter]

OK, I'll leave this one to new maintainer then. :-)

I'm confused, though, why you can't look for the error text 'Exiting with error 
code (instead of crashing) ...'.  This happens at the same time the _exit() 
call happens, so they same code that is checking the exit code could be looking 
for that text, no?

Original comment by csilv...@gmail.com on 21 Feb 2012 at 12:22

[GoogleCodeExporter]

Well, our script just invokes Python's subprocess.Popen() and reads the exit 
code.
In fact it's possible to check the log at the same time, although looking at 
the exit code seems a bit more clear to me.
Another problem with logs is that it's currently impossible to redirect the 
output of different processes running under heapchecker to different log files. 
I'm going to file a separate feature request for that.

Original comment by gli...@chromium.org on 21 Feb 2012 at 7:59

[GoogleCodeExporter]

I have accepted the patch and it is now committed to the trunk. I will close 
this issue at the time of the next release.

Original comment by chapp...@gmail.com on 3 Mar 2012 at 7:47

[GoogleCodeExporter]

Great, thank you!

Original comment by gli...@chromium.org on 5 Mar 2012 at 9:17

[GoogleCodeExporter]

Hi again.
Looks like my idea of overriding the default exit code was a bit incorrect.
Namely, regardless of the exit code returned by the heapchecker, it anyway 
overloads the exit code returned by the process itself, which is not very good.
I haven't found out how to make heapchecker return the original exit code yet.

Original comment by gli...@chromium.org on 16 Mar 2012 at 11:42

[GoogleCodeExporter]

Any further update here?

Original comment by chapp...@gmail.com on 3 May 2012 at 3:11

[GoogleCodeExporter]

I will be backing out this patch until an alternative is provided.

Original comment by chapp...@gmail.com on 4 May 2012 at 5:31

[GoogleCodeExporter]

Any update to this patch? I would like to close this off if possible.

Original comment by chapp...@gmail.com on 28 Oct 2012 at 2:54

[GoogleCodeExporter]

Hi,
Looks like the idea was conceptually incorrect, because we can't obtain the 
original exit code in a portable way.
The best thing to do is really to back this out.

Original comment by gli...@chromium.org on 29 Oct 2012 at 9:16

[GoogleCodeExporter]

Ok sounds good. It's backed out.

Original comment by chapp...@gmail.com on 29 Oct 2012 at 12:12

[GoogleCodeExporter]

Original comment by chapp...@gmail.com on 4 Nov 2012 at 5:24

• Changed state: WontFix