Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove globals and other cleanup #169

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Remove globals and other cleanup #169

wants to merge 21 commits into from

Conversation

nth10sd
Copy link
Contributor

@nth10sd nth10sd commented Apr 11, 2018

Here's a bunch of cleanups.

  • [funfuzz] pylint "global-statement" issue #125 gets fixed here, removing all globals
  • 2 ride-alongs
    • detect_malloc_errors gets removed with its functions simplified and inlined
    • knownPath (from the days of multiple-year-lasting branches) is also removed

@codecov-io
Copy link

codecov-io commented Apr 11, 2018

Codecov Report

Merging #169 into master will increase coverage by 0.15%.
The diff coverage is 17.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   44.17%   44.32%   +0.15%     
==========================================
  Files          31       30       -1     
  Lines        2685     2653      -32     
==========================================
- Hits         1186     1176      -10     
+ Misses       1499     1477      -22
Impacted Files Coverage Δ
src/funfuzz/util/__init__.py 100% <ø> (ø) ⬆️
src/funfuzz/bot.py 23.35% <ø> (+0.16%) ⬆️
src/funfuzz/js/loop.py 18.11% <0%> (+0.38%) ⬆️
src/funfuzz/js/compare_jit.py 15.72% <25%> (-0.65%) ⬇️
src/funfuzz/js/js_interesting.py 22.22% <33.33%> (-0.23%) ⬇️
src/funfuzz/util/file_manipulation.py 15.9% <9.09%> (-2.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35b5e09...e8bc84d. Read the comment docs.

@nth10sd
Copy link
Contributor Author

nth10sd commented Apr 13, 2018

Sample command:

python -u -m lithium funfuzz.js.compare_jit --flags="--fuzzing-safe --test-wasm-await-tier2 --no-wasm-ion --no-asmjs --no-avx --no-sse4 --no-ion --nursery-strings=on --no-array-proto-values --ion-offthread-compile=off --no-threads --baseline-eager" ./js testcase.js
Traceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/home/winworklin/.local/lib/python2.7/site-packages/lithium/__main__.py", line 13, in <module>
    main()
  File "/home/winworklin/.local/lib/python2.7/site-packages/lithium/reducer.py", line 1414, in main
    exit(Lithium().main())
  File "/home/winworklin/.local/lib/python2.7/site-packages/lithium/reducer.py", line 1193, in main
    return self.run()
  File "/home/winworklin/.local/lib/python2.7/site-packages/lithium/reducer.py", line 1209, in run
    result = self.strategy.main(self.testcase, self.interesting, self.testcaseTempFilename)
  File "/home/winworklin/.local/lib/python2.7/site-packages/lithium/reducer.py", line 329, in main
    if not interesting(testcase, writeIt=False):
  File "/home/winworklin/.local/lib/python2.7/site-packages/lithium/reducer.py", line 1366, in interesting
    inter = self.conditionScript.interesting(self.conditionArgs, tempPrefix)
  File "/home/winworklin/.local/lib/python2.7/site-packages/funfuzz/js/compare_jit.py", line 259, in interesting
    opts.jsengine, opts.flags, opts.infilename, tempPrefix, opts, False, False)[0]
AttributeError: 'list' object has no attribute 'jsengine'

This PR still seems to throw when run with Lithium... :-/

Of course, turns out _args is used by Lithium. What will be a good way of solving this?

@jschwartzentruber
Copy link
Collaborator

This is what we did in grizzly's interesting script:

class Interesting(object):

    def __init__(self, interestingness_script=False):
        if interestingness_script:
            global init, interesting, cleanup  # pylint: disable=global-variable-undefined,invalid-name
            init = self.init
            interesting = self.interesting
            cleanup = self.cleanup

        self.args = None
        ...

    def init(self, args):
        self.args = self.parse_ffp_options(args)
        ...

    def interesting(self, _args, temp_prefix):
        ...

    def cleanup(self, _):
        ...

Interesting(True)

So the interestingness_script argument is used to create an instance of the class that handles the global init, interesting, and cleanup "functions" when lithium is called from the command-line, but the class is also usable with lithium.Lithium(). You do still have globals, but they're hidden...

Copy link
Collaborator

@jschwartzentruber jschwartzentruber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nth10sd added a commit to nth10sd/funfuzz that referenced this pull request Jun 6, 2018
nth10sd added a commit to nth10sd/funfuzz that referenced this pull request Jun 7, 2018
nth10sd added a commit to nth10sd/funfuzz that referenced this pull request Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants