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

MAINT: spatial: Explicitly initialize LAPACK output parameters. #9054

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

tpudlik
Copy link
Contributor

@tpudlik tpudlik commented Jul 18, 2018

Prior to this PR, the info and rcond parameters were not initialized in the .pyx file (and so not in the generated C code). The SciPy code relied on the invoked LAPACK routines to initialize these parameters.

However, it is not guaranteed that the LAPACK routines will perform the initialization. For example, dgecon may return before initializing rcond; the Clang MemorySanitizer flags the subsequent use of this variable (line 1124) as use-of-uninitialized-value. Fortunately, in this case there is no bug in behavior, because the LAPACK routine only fails to initialize rcond if it sets info to a nonzero value. The end result is that _get_barycentric_transforms is guaranteed to return a matrix of NaNs in that case, even though the truth value of the if-statement condition is undefined. Still, comparisons involving undefined values make the code needlessly fragile.

@tylerjereddy tylerjereddy added scipy.spatial maintenance Items related to regular maintenance tasks labels Jul 18, 2018
Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Looks pretty subtle; the CI test failures don't appear to be directly related, but I'll have to take a closer look.

If there's no actual bug I suppose writing a unit test to prevent "regression" would be tricky.

@pv is the author of the module & can perhaps comment

@ilayn
Copy link
Member

ilayn commented Jul 18, 2018

Note that the first of the mentioned info checks belongs to dgetrf and not to dgecon. If that doesn't go through, it either means something went wrong or the matrix is singular so in any case NaN's are produced. This is rather not just fortunate but deliberate. And the if branch is well defined.

That's how most of LAPACK routines behave depending on their quick return behavior and I don't think you can initialize every case.

@pv pv merged commit 3565cec into scipy:master Aug 24, 2018
@pv
Copy link
Member

pv commented Aug 24, 2018

thanks, lgtm, although indeed the code should be ok as it is even though a bit wonky

@pv pv added this to the 1.2.0 milestone Aug 24, 2018
ahoenselaar added a commit to ahoenselaar/jax that referenced this pull request Feb 2, 2021
…itives in memory sanitizers.

Related prior art in SciPy: scipy/scipy#9054
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.spatial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants