Improve vanilla allocation/free checking in SpiderMonkey

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 830046 [details] [diff] [review]
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?
Attachment #830046 - Flags: review?(evilpies)
(Assignee)

Comment 2

5 years ago
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.
Attachment #830046 - Flags: review?(gps)

Comment 3

5 years ago
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.
Attachment #830046 - Flags: review?(gps) → review+
I will look it tomorrow! But I can already say that I hugely prefer python over bash.
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.
Attachment #830046 - Flags: review?(evilpies) → review+
(Assignee)

Comment 6

5 years ago
> @@ +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!
https://hg.mozilla.org/mozilla-central/rev/fd939ce2bcdc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
See Also: → bug 1205081
You need to log in before you can comment on or make changes to this bug.