-
Notifications
You must be signed in to change notification settings - Fork 54
Fix: Pass correct flags to linker when debugging in the presence of LTOIR code #698
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
base: main
Are you sure you want to change the base?
Fix: Pass correct flags to linker when debugging in the presence of LTOIR code #698
Conversation
…TOIR code The linker code was passing in -lto to linker invocations that did not involve LTOIR code. When enabling debugging of a Numba CUDA kernel which calls into LTOIR code, an exception was being raised by nvjitlink. This change corrects that behavior, only passing in -lto for cases where at least one LTOIR code object is in the link list. The lto= parameter to the Linker initialization is still used to control compilation of .cu code with LTO enabled (which will result in the self._has_ltoir flag being set). A testcase for validating this change and catching regressions is included. Closes NVIDIA#696
Greptile OverviewGreptile SummaryThis PR fixes a critical bug where the Changes Overviewdriver.py:
test_linker.py:
Issues FoundCritical: The test code has a Incomplete Fix: As noted in previous review threads, the Confidence Score: 2/5
Important Files ChangedFile Analysis
|
|
/ok to test 94e6745 |
| add_from_numba = cuda.declare_device( | ||
| "add_from_numba", | ||
| "int32(int32, int32)", | ||
| link=["testing/test_device_functions.ltoir"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong (hardcoding the path will not work in all cases) - instead it should be like in other test cases, e.g.:
numba-cuda/numba_cuda/numba/cuda/tests/cudapy/test_compiler.py
Lines 41 to 43 in 6a1db64
| test_device_functions_ltoir = os.path.join( | |
| TEST_BIN_DIR, "test_device_functions.ltoir" | |
| ) |
| self.arch = arch | ||
| if lto is False: | ||
| # WAR for apparent nvjitlink issue | ||
| lto = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this will be a problem with the 12.9 CUDA Python bindings - there was a version that would only accept None and not False, but I don't recall what the version was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I could tell from the CI so far, the removal of the None value did not cause an issue - perhaps it is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the tests that are now failing, it seems that this workaround is probably still needed.
|
/ok to test ec52710 |
|
Since the code changes in the non-test code looked good, I've just pushed the relevant fix for the test code to get this over the line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 3 comments
| add_from_numba = cuda.declare_device( | ||
| "add_from_numba", | ||
| "int32(int32, int32)", | ||
| link=[test_device_functions_ltoir], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is defined outside the if TEST_BIN_DIR: block (lines 114-117) but references test_device_functions_ltoir which is only defined inside that block. When TEST_BIN_DIR is not set, importing this test file will raise NameError: name 'test_device_functions_ltoir' is not defined.
This code should be indented to be inside the if TEST_BIN_DIR: block, or the declarations should be conditional:
| add_from_numba = cuda.declare_device( | |
| "add_from_numba", | |
| "int32(int32, int32)", | |
| link=[test_device_functions_ltoir], | |
| ) | |
| if TEST_BIN_DIR: | |
| test_device_functions_ltoir = os.path.join( | |
| TEST_BIN_DIR, "test_device_functions.ltoir" | |
| ) | |
| add_from_numba = cuda.declare_device( | |
| "add_from_numba", | |
| "int32(int32, int32)", | |
| link=[test_device_functions_ltoir], | |
| ) | |
| def debuggable_kernel(result): | |
| i = cuda.grid(1) | |
| result[i] = add_from_numba(i, i) |
| def test_debug_kernel_with_lto(self): | ||
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test uses debuggable_kernel which references add_from_numba, but that function is only defined when TEST_BIN_DIR is set. The test will fail with NameError when TEST_BIN_DIR is not set.
The test should either:
- Be skipped when
TEST_BIN_DIRis not set, or - Be moved inside a conditional block
| def test_debug_kernel_with_lto(self): | |
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) | |
| @unittest.skipIf(not os.getenv("NUMBA_CUDA_TEST_BIN_DIR"), "TEST_BIN_DIR not set") | |
| def test_debug_kernel_with_lto(self): | |
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) |
Additional Comments (1)
When such files are added, the linker will incorrectly use Consider tracking LTOIR presence for these file types as well. This could be done by:
Example approach: def add_fatbin(self, fatbin, name="<cudapy-fatbin>", has_ltoir=False):
obj = ObjectCode.from_fatbin(fatbin, name=name)
self._object_codes.append(obj)
if has_ltoir:
self._has_ltoir = True
def add_object(self, obj, name="<cudapy-object>", has_ltoir=False):
obj = ObjectCode.from_object(obj, name=name)
self._object_codes.append(obj)
if has_ltoir:
self._has_ltoir = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| def test_debug_kernel_with_lto(self): | ||
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_debug_kernel_with_lto references debuggable_kernel and add_from_numba (lines 127-129 and 120-124), which are only defined when TEST_BIN_DIR is set (line 114). When TEST_BIN_DIR is not set, this test will fail with a NameError.
This test needs a skip decorator like other tests in the codebase that depend on TEST_BIN_DIR:
| def test_debug_kernel_with_lto(self): | |
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) | |
| @unittest.skipIf(not TEST_BIN_DIR, "necessary binaries not generated.") | |
| def test_debug_kernel_with_lto(self): |
See test_nrt.py line 182 and test_linkable_code.py line 40 for examples of the correct pattern.
Additional Comments (1)
While Note that the code in Consider updating these methods to detect and set def add_fatbin(self, fatbin, name="<cudapy-fatbin>"):
obj = ObjectCode.from_fatbin(fatbin, name=name)
self._object_codes.append(obj)
# Check if fatbin contains LTOIR and set flag if neededThis would ensure consistent behavior regardless of which file format contains the LTOIR code. |
|
/ok to test |
@gmarkall, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 78e0634 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| add_from_numba = cuda.declare_device( | ||
| "add_from_numba", | ||
| "int32(int32, int32)", | ||
| link=[test_device_functions_ltoir], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will cause a NameError when TEST_BIN_DIR is not set, because test_device_functions_ltoir is only defined inside the if TEST_BIN_DIR: block (lines 114-117). When the module loads, the cuda.declare_device() call will fail trying to access an undefined variable.
This will prevent the entire test module from importing when NUMBA_CUDA_TEST_BIN_DIR is not set in the environment.
The module-level declarations of add_from_numba and debuggable_kernel should be moved inside the if TEST_BIN_DIR: block, or the test method test_debug_kernel_with_lto should include a skip decorator similar to other tests in test_nvjitlink.py:
| add_from_numba = cuda.declare_device( | |
| "add_from_numba", | |
| "int32(int32, int32)", | |
| link=[test_device_functions_ltoir], | |
| ) | |
| if TEST_BIN_DIR: | |
| test_device_functions_ltoir = os.path.join( | |
| TEST_BIN_DIR, "test_device_functions.ltoir" | |
| ) | |
| add_from_numba = cuda.declare_device( | |
| "add_from_numba", | |
| "int32(int32, int32)", | |
| link=[test_device_functions_ltoir], | |
| ) | |
| def debuggable_kernel(result): | |
| i = cuda.grid(1) | |
| result[i] = add_from_numba(i, i) |
| def test_debug_kernel_with_lto(self): | ||
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_debug_kernel_with_lto will fail when TEST_BIN_DIR environment variable is not set because debuggable_kernel and add_from_numba are only defined when TEST_BIN_DIR is set (as suggested in the comment on lines 120-124).
This test should include a skip decorator similar to tests in test_nvjitlink.py:
| def test_debug_kernel_with_lto(self): | |
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) | |
| @unittest.skipIf( | |
| not TEST_BIN_DIR, "TEST_BIN_DIR not set" | |
| ) | |
| def test_debug_kernel_with_lto(self): |
This reverts commit 78e0634.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| TEST_BIN_DIR = os.getenv("NUMBA_CUDA_TEST_BIN_DIR") | ||
| if TEST_BIN_DIR: | ||
| test_device_functions_ltoir = os.path.join( | ||
| TEST_BIN_DIR, "test_device_functions.ltoir" | ||
| ) | ||
|
|
||
|
|
||
| add_from_numba = cuda.declare_device( | ||
| "add_from_numba", | ||
| "int32(int32, int32)", | ||
| link=[test_device_functions_ltoir], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable test_device_functions_ltoir is only defined when TEST_BIN_DIR is set (line 114-117), but it's used unconditionally on line 123. This will cause a NameError when the test module is imported if TEST_BIN_DIR is not set in the environment, preventing the entire test module from loading.
This should either:
- Move the
add_from_numbaanddebuggable_kerneldefinitions inside the conditional block, or - Initialize
test_device_functions_ltoirto a default value (likeNone) and add appropriate guards
| TEST_BIN_DIR = os.getenv("NUMBA_CUDA_TEST_BIN_DIR") | |
| if TEST_BIN_DIR: | |
| test_device_functions_ltoir = os.path.join( | |
| TEST_BIN_DIR, "test_device_functions.ltoir" | |
| ) | |
| add_from_numba = cuda.declare_device( | |
| "add_from_numba", | |
| "int32(int32, int32)", | |
| link=[test_device_functions_ltoir], | |
| ) | |
| TEST_BIN_DIR = os.getenv("NUMBA_CUDA_TEST_BIN_DIR") | |
| if TEST_BIN_DIR: | |
| test_device_functions_ltoir = os.path.join( | |
| TEST_BIN_DIR, "test_device_functions.ltoir" | |
| ) | |
| add_from_numba = cuda.declare_device( | |
| "add_from_numba", | |
| "int32(int32, int32)", | |
| link=[test_device_functions_ltoir], | |
| ) | |
| def debuggable_kernel(result): | |
| i = cuda.grid(1) | |
| result[i] = add_from_numba(i, i) |
| def test_debug_kernel_with_lto(self): | ||
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test uses debuggable_kernel which references test_device_functions_ltoir that only exists when TEST_BIN_DIR is set. The test should have a skip decorator to avoid failures when the required LTOIR file is not available.
Consider adding a skip decorator similar to other tests in this file:
| def test_debug_kernel_with_lto(self): | |
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) | |
| @unittest.skipUnless(TEST_BIN_DIR, "TEST_BIN_DIR not set") | |
| def test_debug_kernel_with_lto(self): | |
| cuda.jit("void(int32[::1])", debug=True, opt=False)(debuggable_kernel) |
gmarkall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the old WAR is not related to the fails on CUDA <= 12.2 - this seems to be some other issue that needs debugging - accordingly, I've left the change in its original form, and the only change of mine that I kept is the fix so that the test can run.
I would ignore greptile, it is talking without enough idea about the larger context.
The linker code was passing in -lto to linker invocations that did not involve LTOIR code, and not passing it in some cases where LTOIR code was being linked. When enabling debugging of a Numba CUDA kernel which calls into LTOIR code, an exception was being raised by nvjitlink.
This change corrects that behavior, only passing in -lto for cases where at least one LTOIR code object is in the link list. The lto= parameter to the Linker initialization is still used to control compilation of .cu code with LTO enabled (which will result in the self._has_ltoir flag being set).
A testcase for validating this change and catching regressions is included.
Closes #696