937012 - Improve vanilla allocation/free checking in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

We have a script, config/find_vanilla_new_calls, which checks for occurrences
of |operator new| and |operator new[]| in SpiderMonkey.  It has two problems:

- It doesn't check for all the other allocation functions (malloc, etc)

- It's broken.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Replace the busted find_vanilla_new_calls script with the much better check_vanilla_allocations.py.

This patch replaces find_vanilla_new_calls with check_vanilla_allocations.py.
The new script checks for all vanilla allocation/free functions, except that by
default it (unfortunately) doesn't check for malloc/calloc/realloc/free because
that can't be done reliably in a standard configuration.  You can use the
--aggressive option to add checking for them.

This script also is more robust against possible future changes in nm's output.
Explanations of this robustness are in the script.

evilpie, how is your Python?

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 830046 [details] [diff] [review]
Replace the busted find_vanilla_new_calls script with the much better check_vanilla_allocations.py.

Review of attachment 830046 [details] [diff] [review]:
-----------------------------------------------------------------

gps, asking you to review the almost-trivial changes to the build system.  Basically, I rewrote an existing script, and it can be called in two ways.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 830046 [details] [diff] [review]
Replace the busted find_vanilla_new_calls script with the much better check_vanilla_allocations.py.

Review of attachment 830046 [details] [diff] [review]:
-----------------------------------------------------------------

Man, I'm not a huge fan of stuffing random scripts in config/, especially scripts that aren't really build system core. Have you considered putting this in /tools instead? If nothing else, we'd have one less file to keep in sync (I can't wait to kill the manual syncing of files to js/src).

r+ covers just the build bits. I would really like to see the script live elsewhere, however.

::: config/check_vanilla_allocations.py
@@ +127,5 @@
> +            fn = m.group(1)
> +            filename = m.group(2)
> +            linenum = m.group(3)
> +            if filename == 'jsutil.cpp':
> +                jsutil_cpp[fn] = True

Use a set?

@@ +142,5 @@
> +        else:
> +            del jsutil_cpp[fn]
> +
> +    # This should never happen, but check just in case.
> +    if len(jsutil_cpp) > 0:

if jsutil_cpp:

@@ +147,5 @@
> +        fail('unexpected allocation fns used in jsutil.cpp: ' +
> +             ', '.join(jsutil_cpp))
> +
> +    if has_failed:
> +        sys.exit(1)

"Everything is a library." I'd love to see this function available as an API. sys.exit(), sys.argv, etc effectively prevent that. But this is your script.

Comment 4

[Tom S [:evilpie]]

I will look it tomorrow! But I can already say that I hugely prefer python over bash.

Comment 5

[Tom S [:evilpie]]

Comment on attachment 830046 [details] [diff] [review]
Replace the busted find_vanilla_new_calls script with the much better check_vanilla_allocations.py.

Review of attachment 830046 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/check_vanilla_allocations.py
@@ +57,5 @@
> +has_failed = False
> +
> +
> +def fail(msg):
> +    print('TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py |', msg)

This should really throw an exception that gets caught and causes an unclean exit.

@@ +62,5 @@
> +    has_failed = True
> +
> +
> +def main():
> +    aggressive = False

I think we can use 2.7:

import argparse

parser = argparse.ArgumentParser()
parser.add_argument('--agressive', action='store_true',
                    help='deny any instance of malloc/free')
parser.add_argument('file', type=str,
                    help='name of the file to check')
args = parser.parse_args()

print args.agressive
print args.file

@@ +82,5 @@
> +    lines = subprocess.check_output(cmd, universal_newlines=True,
> +                                    stderr=subprocess.PIPE).split('\n')
> +
> +    # alloc_fns contains all the vanilla allocation/free functions that we look
> +    # for.  Regexp chars are escaped appropriately.

double space

@@ +121,5 @@
> +    # This tracks which allocation/free functions have been seen in jsutil.cpp.
> +    jsutil_cpp = {}
> +
> +    for line in lines:
> +        m = re.search(alloc_fns_re, line)

if m is None:
 continue

@@ +139,5 @@
> +    for fn in alloc_fns_unescaped:
> +        if fn not in jsutil_cpp:
> +            fail("'" + fn + "' isn't used as expected in jsutil.cpp")
> +        else:
> +            del jsutil_cpp[fn]

Yeah set, so you can avoid this.

Comment 6

[Nicholas Nethercote [inactive]]

> @@ +57,5 @@
> > +has_failed = False
> > +
> > +
> > +def fail(msg):
> > +    print('TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py |', msg)
> 
> This should really throw an exception that gets caught and causes an unclean
> exit.

has_failed is set to True on failure (and I forgot the necessary |global x|, but have it now).  So we exit with an exitcode of 1, which causes the make invocation to fail.  And this way, multiple failure messages can be printed;  if I threw an exception the script would just abort early.  (Or I'd have to catch it, and then that's the same effect as has_failed but less simple.)

I've made the other requested changes, thanks!

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd939ce2bcdc

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd939ce2bcdc