Fixes for lineinfo emission by stuartarchibald · Pull Request #7196 · numba/numba

stuartarchibald · 2021-07-09T11:27:02Z

As title.

Includes: 2d63784 from #7177 as 3631bdb

As title.

sklam · 2021-07-13T22:46:36Z

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 · 2021-07-14T10:27:25Z

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

As title.

sklam · 2021-07-14T14:11:33Z

Cool, i'll try it now

sklam · 2021-07-14T15:19:20Z

+        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)


a duplication of

numba/numba/tests/test_parfors.py

Lines 73 to 86 in 0c499bf

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

As title.

sklam · 2021-07-14T21:57:10Z

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


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

numba/numba/tests/test_parfors.py

Lines 56 to 57 in 2eacea6

_exec_cond = os.environ.get('SUBPROC_TEST', None) == '1'

needs_subprocess = unittest.skipUnless(_exec_cond, "needs subprocess harness")

sklam · 2021-07-14T21:59:54Z

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


Suggested change

self.assertTrue('FAIL' not in status.stderr)

self.assertNotIn('FAIL', status.stderr)

sklam · 2021-07-14T22:00:08Z

+        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)


Suggested change

self.assertTrue('ERROR' not in status.stderr)

self.assertNotIn('ERROR', status.stderr)

sklam · 2021-07-14T22:00:33Z

+            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!


Suggested change

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

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

sklam · 2021-07-14T22:03:15Z

+        sig = (types.float64[::1],)
+        full_ir = self._get_llvmir(foo, sig=sig)
+
+        # make sure decref lines end with `meminfo.<number>)`.


Suggested change

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

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

sklam · 2021-07-14T22:14:59Z

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.

As title.

stuartarchibald · 2021-07-15T16:32:07Z

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.

sklam

Thanks for the patch!

…_refcounted Fixes for lineinfo emission

stuartarchibald added 2 commits July 9, 2021 12:21

Fix up bug in lineinfo wrt decref

3631bdb

Handle lineinfo emission correctly across phi node stripping.

f6bef9d

As title.

stuartarchibald requested review from esc and sklam as code owners July 9, 2021 11:27

stuartarchibald added the 2 - In Progress label Jul 9, 2021

stuartarchibald added this to the Numba 0.54 RC2 milestone Jul 9, 2021

Test for decref, undefined and versioned variables.

5c3d7ae

As title.

stuartarchibald force-pushed the fix/debug_lineinfo_refcounted branch from 078c95d to 5c3d7ae Compare July 14, 2021 13:47

stuartarchibald added 3 - Ready for Review Effort - medium Medium size effort needed and removed 2 - In Progress labels Jul 14, 2021

sklam reviewed Jul 14, 2021

View reviewed changes

Refactor unit tests to share common subprocess testing.

2eacea6

As title.

sklam reviewed Jul 14, 2021

View reviewed changes

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

Respond to feedback

8f1c198

As title.

sklam approved these changes Jul 15, 2021

View reviewed changes

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 mentioned this pull request Jul 15, 2021

Numba 0.54.0 rc2 | llvmlite 0.37.0 rc2 | Checklist #7206

Closed

36 tasks done

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

Merge pull request numba#7196 from stuartarchibald/fix/debug_lineinfo…

83f3123

…_refcounted Fixes for lineinfo emission

sklam mentioned this pull request Jul 15, 2021

RC2 patches for release0.54 branch #7211

Merged

Navigation Menu

Repository navigation

Fixes for lineinfo emission - #7196

Fixes for lineinfo emission#7196
sklam merged 5 commits into
numba:masterfrom
stuartarchibald:fix/debug_lineinfo_refcounted

stuartarchibald commented Jul 9, 2021

Uh oh!

sklam commented Jul 13, 2021 •

edited

Loading

Uh oh!

stuartarchibald commented Jul 14, 2021

Uh oh!

sklam commented Jul 14, 2021

Uh oh!

sklam Jul 14, 2021

Uh oh!

sklam Jul 14, 2021

Uh oh!

sklam Jul 14, 2021

Uh oh!

sklam Jul 14, 2021

Uh oh!

sklam Jul 14, 2021

Uh oh!

sklam Jul 14, 2021

Uh oh!

sklam commented Jul 14, 2021

Uh oh!

stuartarchibald commented Jul 15, 2021

Uh oh!

sklam left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	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)

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

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

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

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

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

Conversation

stuartarchibald commented Jul 9, 2021

Uh oh!

sklam commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartarchibald commented Jul 14, 2021

Uh oh!

sklam commented Jul 14, 2021

Uh oh!

sklam Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

sklam Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

sklam Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

sklam Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

sklam Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

sklam Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

sklam commented Jul 14, 2021

Uh oh!

stuartarchibald commented Jul 15, 2021

Uh oh!

sklam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sklam commented Jul 13, 2021 •

edited

Loading