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

Fixes for lineinfo emission #7196

Merged
merged 5 commits into from Jul 15, 2021

Conversation

stuartarchibald
Copy link
Contributor

As title.

Includes: 2d63784 from #7177 as 3631bdb

@sklam
Copy link
Member

sklam commented Jul 13, 2021

I can confirm that the decref is no longer jumping back to the line of object creation. I used the following code to test the decref DWARF info:

@njit(debug=True)
def foo(n):
    #gdb()
    ar = np.arange(n)
    if ar.sum() == 0:    # lifetime of ar ends after this line
        ret = 2
    else:
        ret = 3
    return ret

@stuartarchibald
Copy link
Contributor Author

@sklam thanks for checking. I've patched the disassembly support here: stuartarchibald@ee91dd8 to include DWARF info in the disassembly CFG.

@sklam
Copy link
Member

sklam commented Jul 14, 2021

Cool, i'll try it now

Comment on lines 85 to 98
themod = self.__module__
thecls = type(self).__name__
injected_method = f'{themod}.{thecls}.{test_name}'
cmd = [sys.executable, '-m', 'numba.runtests', injected_method]
env_copy = os.environ.copy()
env_copy['SUBPROC_TEST'] = '1'
env_copy['NUMBA_OPT'] = '0'
status = subprocess.run(cmd, stdout=subprocess.PIPE,
stderr=subprocess.PIPE, timeout=60,
env=env_copy, universal_newlines=True)
self.assertEqual(status.returncode, 0)
self.assertIn('OK', status.stderr)
self.assertTrue('FAIL' not in status.stderr)
self.assertTrue('ERROR' not in status.stderr)
Copy link
Member

Choose a reason for hiding this comment

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

a duplication of

def runner(self):
themod = self.__module__
test_clazz_name = self.id().split('.')[-1].split('_')[-1]
the_test = f'{themod}.{test_clazz_name}'
cmd = [sys.executable, '-m', 'numba.runtests', the_test]
env_copy = os.environ.copy()
env_copy['SUBPROC_TEST'] = '1'
status = subprocess.run(cmd, stdout=subprocess.PIPE,
stderr=subprocess.PIPE, timeout=self._TIMEOUT,
env=env_copy, universal_newlines=True)
self.assertEqual(status.returncode, 0, msg=status.stderr)
self.assertIn('OK', status.stderr)
self.assertTrue('FAIL' not in status.stderr)
self.assertTrue('ERROR' not in status.stderr)

we should dedup here or in a immediate follow up PR

Comment on lines +121 to +122
_exec_cond = os.environ.get('SUBPROC_TEST', None) == '1'
needs_subprocess = unittest.skipUnless(_exec_cond, "needs subprocess harness")
Copy link
Member

Choose a reason for hiding this comment

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

There's still a copy of this in test_parfors.py

_exec_cond = os.environ.get('SUBPROC_TEST', None) == '1'
needs_subprocess = unittest.skipUnless(_exec_cond, "needs subprocess harness")

f'captured stderr: {status.stderr}')
self.assertEqual(status.returncode, 0, streams)
self.assertIn('OK', status.stderr)
self.assertTrue('FAIL' not in status.stderr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertTrue('FAIL' not in status.stderr)
self.assertNotIn('FAIL', status.stderr)

self.assertEqual(status.returncode, 0, streams)
self.assertIn('OK', status.stderr)
self.assertTrue('FAIL' not in status.stderr)
self.assertTrue('ERROR' not in status.stderr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertTrue('ERROR' not in status.stderr)
self.assertNotIn('ERROR', status.stderr)

if line_stripped.startswith('call void @NRT_decref'):
self.assertRegex(line, r'.*meminfo\.[0-9]+\)$')
count += 1
self.assertTrue(count > 0) # make sure there were some decrefs!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertTrue(count > 0) # make sure there were some decrefs!
self.assertGreater(count, 0) # make sure there were some decrefs!

sig = (types.float64[::1],)
full_ir = self._get_llvmir(foo, sig=sig)

# make sure decref lines end with `meminfo.<number>)`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# make sure decref lines end with `meminfo.<number>)`.
# make sure decref lines end with `meminfo.<number>)` without dbg info.

@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 14, 2021
@sklam
Copy link
Member

sklam commented Jul 14, 2021

I wonder if we should leave a note about how the tests are sensitive to SSA and lowering behavior such that they may become outdated when those are changed.

@stuartarchibald
Copy link
Contributor Author

I wonder if we should leave a note about how the tests are sensitive to SSA and lowering behavior such that they may become outdated when those are changed.

Good point, I've added one as part of 8f1c198. All other comments should be addressed in there too. Thanks for the review.

Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 15, 2021
@sklam sklam merged commit 9b0ad07 into numba:master Jul 15, 2021
sklam added a commit to sklam/numba that referenced this pull request Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants