Skip to content

Fix various issues with dwarf emission: #7116

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.

Sign up for GitHub

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

Merged
sklam merged 14 commits into from
Jun 28, 2021
Merged

Fix various issues with dwarf emission: #7116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
51d0563
Change lang in compile_unit
vlad-perevezentsev Jun 1, 2021
5b295cc
Fix various issues with dwarf emission:
stuartarchibald Jun 11, 2021
bf19801
Add some basic testing of debug information.
stuartarchibald Jun 15, 2021
b727829
Add test for arg variable location
stuartarchibald Jun 21, 2021
77f4530
Remove remark about 'jumping' debug info from docs
stuartarchibald Jun 21, 2021
777f495
Merge remote-tracking branch 'upstream/master' into fix/7103
stuartarchibald Jun 21, 2021
f9f3a91
Fix up tests WRT alloca opt patch
stuartarchibald Jun 21, 2021
ed5183e
Numerous improvements/fixes.
stuartarchibald Jun 23, 2021
fc5859b
Fix test case to prevent reordering.
stuartarchibald Jun 23, 2021
78989a0
Merge remote-tracking branch 'upstream/master' into fix/7103
stuartarchibald Jun 25, 2021
b11eb20
Fix kwarg change
stuartarchibald Jun 25, 2021
7bb2960
Do not add vectorization helper passes at OPT=0
stuartarchibald Jun 25, 2021
c0d9ee8
Update comment
stuartarchibald Jun 28, 2021
8329107
Add predicate for debug emission throughout lowering.
stuartarchibald Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions docs/source/user/troubleshoot.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,14 +402,8 @@ debug info is available:

Known issues:

* Stepping depends heavily on optimization level.

* At full optimization (equivalent to O3), most of the variables are
optimized out.
* With no optimization (e.g. ``NUMBA_OPT=0``), source location jumps around
when stepping through the code.
* At O1 optimization (e.g. ``NUMBA_OPT=1``), stepping is stable but some
variables are optimized out.
* Stepping depends heavily on optimization level. At full optimization
(equivalent to O3), most of the variables are optimized out.

* Memory consumption increases significantly with debug info enabled.
The compiler emits extra information (`DWARF <http://www.dwarfstd.org/>`_)
Expand Down
22 changes: 13 additions & 9 deletions numba/core/cgutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from llvmlite import ir

from numba.core import utils, types, config
from numba.core import utils, types, config, debuginfo
import numba.core.datamodel


Expand Down Expand Up @@ -370,14 +370,18 @@ def alloca_once(builder, ty, size=None, name='', zfill=False):
"""
if isinstance(size, int):
size = ir.Constant(intp_t, size)
with builder.goto_entry_block():
ptr = builder.alloca(ty, size=size, name=name)
# Always zero-fill at init-site. This is safe.
builder.store(ptr.type.pointee(None), ptr)
# Also zero-fill at the use-site
if zfill:
builder.store(ptr.type.pointee(None), ptr)
return ptr
# suspend debug metadata emission else it links up python source lines with
# alloca in the entry block as well as their actual location and it makes
# the debug info "jump about".
with debuginfo.suspend_emission(builder):
with builder.goto_entry_block():
ptr = builder.alloca(ty, size=size, name=name)
# Always zero-fill at init-site. This is safe.
builder.store(ptr.type.pointee(None), ptr)
# Also zero-fill at the use-site
if zfill:
builder.store(ptr.type.pointee(None), ptr)
return ptr


def sizeof(builder, ptr_type):
Expand Down
4 changes: 3 additions & 1 deletion numba/core/codegen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,7 +1209,9 @@ def _module_pass_manager(self, **kwargs):
cost = kwargs.pop("cost", None)
with self._pass_manager_builder(**kwargs) as pmb:
pmb.populate(pm)
if cost is not None and cost == "cheap":
# If config.OPT==0 do not include these extra passes to help with
# vectorization.
if cost is not None and cost == "cheap" and config.OPT != 0:
# This knocks loops into rotated form early to reduce the likelihood
# of vectorization failing due to unknown PHI nodes.
pm.add_loop_rotate_pass()
Expand Down
14 changes: 13 additions & 1 deletion numba/core/debuginfo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,22 @@

import abc
import os.path
from contextlib import contextmanager

from llvmlite import ir
from numba.core import cgutils

@contextmanager
def suspend_emission(builder):
"""Suspends the emission of debug_metadata for the duration of the context
managed block."""
ref = builder.debug_metadata
builder.debug_metadata = None
try:
yield
finally:
builder.debug_metadata = ref


class AbstractDIBuilder(metaclass=abc.ABCMeta):
@abc.abstractmethod
Expand Down Expand Up @@ -216,7 +228,7 @@ def _di_file(self):

def _di_compile_unit(self):
return self.module.add_debug_info('DICompileUnit', {
'language': ir.DIToken('DW_LANG_Python'),
'language': ir.DIToken('DW_LANG_C_plus_plus'),
'file': self.difile,
'producer': 'Numba',
'runtimeVersion': 0,
Expand Down
47 changes: 34 additions & 13 deletions numba/core/lowering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def __init__(self, context, library, fndesc, func_ir, metadata=None):
self.call_conv = context.call_conv
self.generator_info = func_ir.generator_info
self.metadata = metadata
self.flags = utils.ConfigStack.top_or_none()

# Initialize LLVM
self.module = self.library.create_ir_module(self.fndesc.unique_name)
Expand Down Expand Up @@ -190,9 +191,12 @@ def lower_normal_function(self, fndesc):
self.extract_function_arguments()
entry_block_tail = self.lower_function_body()

# Close tail of entry block
self.builder.position_at_end(entry_block_tail)
self.builder.branch(self.blkmap[self.firstblk])
# Close tail of entry block, do not emit debug metadata else the
# unconditional jump gets associated with the metadata from the function
# body end.
with debuginfo.suspend_emission(self.builder):
self.builder.position_at_end(entry_block_tail)
self.builder.branch(self.blkmap[self.firstblk])

def lower_function_body(self):
"""
Expand Down Expand Up @@ -276,6 +280,14 @@ def init(self):
# find all singly assigned variables
self._find_singly_assigned_variable()

@property
def _disable_sroa_like_opt(self):
"""Flags that the SROA like optimisation that Numba performs (which
prevent alloca and subsequent load/store for locals) should be disabled.
Currently, this is conditional solely on the presence of a request for
the emission of debug information."""
return False if self.flags is None else self.flags.debuginfo

def _find_singly_assigned_variable(self):
func_ir = self.func_ir
blocks = func_ir.blocks
Expand Down Expand Up @@ -1281,10 +1293,11 @@ def _alloca_var(self, name, fetype):
# quit early
return

llty = self.context.get_value_type(fetype)
# If the name is used in multiple blocks...
if name not in self._singly_assigned_vars:
# If the name is used in multiple blocks or lowering with debuginfo...
if ((name not in self._singly_assigned_vars) or
self._disable_sroa_like_opt):
# If not already defined, allocate it
llty = self.context.get_value_type(fetype)
ptr = self.alloca_lltype(name, llty)
# Remember the pointer
self.varmap[name] = ptr
Expand All @@ -1293,15 +1306,16 @@ def getvar(self, name):
"""
Get a pointer to the given variable's slot.
"""
assert name not in self._blk_local_varmap
assert name not in self._singly_assigned_vars
if not self._disable_sroa_like_opt:
assert name not in self._blk_local_varmap
assert name not in self._singly_assigned_vars
return self.varmap[name]

def loadvar(self, name):
"""
Load the given variable's value.
"""
if name in self._blk_local_varmap:
if name in self._blk_local_varmap and not self._disable_sroa_like_opt:
return self._blk_local_varmap[name]
ptr = self.getvar(name)
return self.builder.load(ptr)
Expand All @@ -1315,7 +1329,8 @@ def storevar(self, value, name):
self._alloca_var(name, fetype)

# Store variable
if name in self._singly_assigned_vars:
if (name in self._singly_assigned_vars and
not self._disable_sroa_like_opt):
self._blk_local_varmap[name] = value
else:
# Clean up existing value stored in the variable
Expand All @@ -1341,15 +1356,16 @@ def delvar(self, name):
fetype = self.typeof(name)

# Out-of-order
if name not in self._blk_local_varmap:
if (name not in self._blk_local_varmap and
not self._disable_sroa_like_opt):
if name in self._singly_assigned_vars:
self._singly_assigned_vars.discard(name)

# Define if not already (may happen if the variable is deleted
# at the beginning of a loop, but only set later in the loop)
self._alloca_var(name, fetype)

if name in self._blk_local_varmap:
if name in self._blk_local_varmap and not self._disable_sroa_like_opt:
llval = self._blk_local_varmap[name]
self.decref(fetype, llval)
else:
Expand All @@ -1369,11 +1385,16 @@ def alloca_lltype(self, name, lltype):
aptr = cgutils.alloca_once(self.builder, lltype,
name=name, zfill=False)
if is_uservar:
# If it's an arg set the location as the function definition line
if name in self.func_ir.arg_names:
loc = self.loc.with_lineno(self.func_ir.loc.line)
else:
loc = self.loc
Comment on lines +1388 to +1392

@gmarkall gmarkall Jun 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I couldn't see anything that tests this. Do you think the following addition is a reasonable test?

diff --git a/numba/tests/test_debuginfo.py b/numba/tests/test_debuginfo.py
index 7a1aae08f..e9d2fc410 100644
--- a/numba/tests/test_debuginfo.py
+++ b/numba/tests/test_debuginfo.py
@@ -182,6 +182,16 @@ class TestDebugInfoEmission(TestCase):
             dilocation_info = metadata_definition_map[k]
             self.assertIn(f'line: {line_no}', dilocation_info)
 
+        # Check that the line number of the argument `a` is the line number
+        # that the function begins on
+        line_re = re.compile(r'.*!DILocalVariable\(name: "a",.*line: ([0-9]*),')
+        for line in metadata:
+            matched = line_re.match(line)
+            if matched:
+                g = matched.groups()
+                self.assertEqual(len(g), 1)
+                self.assertEqual(int(g[0]), pysrc_line_start)
+
 
 if __name__ == '__main__':
     unittest.main()

@stuartarchibald stuartarchibald Jun 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, good point, I'll add something to this effect.

# Emit debug info for user variable
sizeof = self.context.get_abi_sizeof(lltype)
self.debuginfo.mark_variable(self.builder, aptr, name=name,
lltype=lltype, size=sizeof,
line=self.loc.line)
line=loc.line)
return aptr

def incref(self, typ, val):
Expand Down
Loading