Skip to content

Check nvJitLink driver major compatibility#2320

Merged
lijinf2 merged 7 commits into
NVIDIA:mainfrom
isVoid:pr1911-rework-tot
Jul 9, 2026
Merged

Check nvJitLink driver major compatibility#2320
lijinf2 merged 7 commits into
NVIDIA:mainfrom
isVoid:pr1911-rework-tot

Conversation

@isVoid

@isVoid isVoid commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR improves linker backend selection and fixes a cuLink storage-lifetime regression.

  • Handle systems where the CUDA driver major version is newer than the available nvJitLink major version. In this case, cuda.core emits a warning and falls back to the cuLink driver APIs instead of selecting an incompatible nvJitLink backend.
  • Keep the option arrays and log buffers passed to cuLinkCreate alive for the full lifetime of the CUlinkState. The state is now destroyed with cuLinkDestroy before its referenced storage is released, preventing use-after-free, heap corruption, and possible segmentation faults.

Testing

  • Added coverage for nvJitLink and driver major-version compatibility.
  • Added cuLink regression coverage for repeated link/close operations and log access after closing the linker.

closes #712
supersedes #1911

@copy-pr-bot

copy-pr-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@isVoid isVoid added the cuda.core Everything related to the cuda.core module label Jul 8, 2026 — with ChatGPT Codex Connector
@isVoid isVoid added this to the cuda.core next milestone Jul 8, 2026 — with ChatGPT Codex Connector
@isVoid isVoid self-assigned this Jul 8, 2026
@isVoid

isVoid commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test

@isVoid isVoid changed the title [codex] Check nvJitLink driver major compatibility Check nvJitLink driver major compatibility Jul 8, 2026
@isVoid isVoid marked this pull request as ready for review July 8, 2026 22:27
@github-actions

This comment has been minimized.

@isVoid isVoid requested review from Copilot, leofang and lijinf2 and removed request for Copilot July 9, 2026 00:05
@isVoid isVoid added the enhancement Any code-related improvements label Jul 9, 2026
Comment thread cuda_core/tests/test_program_cache.py Outdated
self._error_log = self.get_error_log()
# Destroy the CUlinkState before releasing storage referenced by it.
self._culink_handle.reset()
self._drv_jit_keys.swap(empty_keys)

@lijinf2 lijinf2 Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one question (not blocking): is syntax self._drv_jit_keys.swap(vector[cydriver.CUjit_option]()) supported in .pyx? C++ should support temporary object created as a function argument and I get used to it.

@isVoid isVoid Jul 9, 2026

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.

C++ should support temporary object created as a function argument and I get used to it.

Yes C++ support temp vars as function argument, but only if the function declare to support rvalue reference. swap(vector& __x) _GLIBCXX_NOEXCEPT is declared with lvalue reference so you can't pass a temporary object to argument there. godbolt

However, in cython both works. In either way presented cython will lower the arguments into a named local to support various c++ / python interfacing shenanigans.

By cythonizing both and comparing the lowered code, the inlined version generate a slightly more complex C++ with additional assignment like below. So I suggest we keep the current declare-then-use form, not the inline construction form.

  try {
      __pyx_t_5 = std::vector<CUjit_option>();
  } catch (...) {
      __Pyx_CppExn2PyErr();
      goto error;
  }
  __pyx_v_self->_drv_jit_keys.swap(__pyx_t_5);

  try {
      __pyx_t_6 =
          std::vector<__pyx_t_4core_7_linker_void_ptr>();
  } catch (...) {
      __Pyx_CppExn2PyErr();
      goto error;
  }

@lijinf2 lijinf2 Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good findings. Was not aware that swap takes only lvalue as argument, and that cython is doing the same declare-then-use under the hood with rvalue. That makes sense. I learned a lot from the answer!

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.

Couldn't we just shrink the vector sizes to zero and let the elements go out of scope?

@isVoid isVoid Jul 9, 2026

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.

Couldn't we just shrink the vector sizes to zero and let the elements go out of scope?

Both handles to the vectors now lives with the Linker class so they don't go out of scope as long as the Linker instance stays alive.

clear() and resize(0) both destroys the elements in the held vector but does not necessarily deallcoate the backing allocation just in case we need to reuse them. This could increase unwanted host side usage.

@lijinf2 lijinf2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks pretty good. Thank you!

Also got the code reviewed by agent. Did not find any must-fix comments by examining the agent's suggestions.

@lijinf2 lijinf2 enabled auto-merge (squash) July 9, 2026 22:03
@lijinf2 lijinf2 merged commit e83d434 into NVIDIA:main Jul 9, 2026
105 checks passed
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

Copy link
Copy Markdown
Doc Preview CI
Preview removed because the pull request was closed or merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.core Everything related to the cuda.core module enhancement Any code-related improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-work on Linker dispatching logic

3 participants