8000 MAINT: use more conservative integer types for umath linalg by argriffing · Pull Request #5899 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: use more conservative integer types for umath linalg #5899

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

Jump to bottom
Merged
merged 2 commits into from
May 21, 2015
Merged

MAINT: use more conservative integer types for umath linalg #5899

merged 2 commits into from
May 21, 2015

Conversation

argriffing
Copy link
Contributor

I'm not sure this is useful or even works.

@argriffing argriffing mentioned this pull request May 21, 2015
Closed
jaimefrio
jaimefrio reviewed May 21, 2015
View reviewed changes
numpy/linalg/umath_linalg.c.src

if (!compute_urows_vtcolumns(jobz, m, n, &u_row_count, &vt_column_count))
goto error;

u_size = ((size_t)u_row_count)*m*sizeof(@ftyp@);
vt_size = n*((size_t)vt_column_count)*sizeof(@ftyp@);
u_size = ((size_t)u_row_count) * safe_m * sizeof(@ftyp@);
Copy link
Member

Choose a reason for hiding this comment

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

Why casting here? Wouldn't safe_u_row_count and safe_vt_col_count be more consistent choices?

@jaimefrio
Copy link
Member

Mostly LGTM. It certainly cannot hurt...

@argriffing
Copy link
Contributor Author

The latest commit has addressed some comments about notation consistency.

There is still room for improvement, but this could be out of scope for this PR. For example, there are some function calls like underlying_lapack_function(..., (low_precision_int_type) high_precision_workspace_size, ...); which could be checked for overflow. Also I think the malloc failure branch is weird -- it sets the output values to zero with no python warnings or exceptions as far as I can tell. These questions could be related to #3217.

@jaimefrio
Copy link
Member

Is this ready to merge then? Or do we want to figure out what exactly is going on with the segfault in #5898 before putting it in?

@argriffing
Copy link
Contributor Author

@jaimefrio I'm pretty sure the reason for the segfault in #5898 is that

  1. After working around some overflow problems, the numpy allocation code no longer attempts to allocate a negative number of things, so it no longer prematurely returns from the function call. Therefore it actually reaches the point of calling the underlying LAPACK dgesv function.
  2. Some implementations of the underlying LAPACK function (e.g. OpenBLAS, apparently) can deal with input matrices of size NxN, even with an interface that uses int32 for N and even if N*N would overflow 32 bits. For example OpenBLAS uses blasint for its interface (e.g. for passing N) which sounds like it could be 32-bit, whereas the function that does the actual indexing uses BLASLONG which sounds less likely to overflow.
    https://github.com/xianyi/OpenBLAS/blob/develop/interface/lapack/gesv.c
    https://github.com/xianyi/OpenBLAS/blob/develop/lapack/getrf/getrf_single.c
    But the numpy lapack-lite implementation seems to internally use int32 indices into the flat NxN memory which overflows and segfaults.
    https://github.com/numpy/numpy/blob/maintenance/1.9.x/numpy/linalg/lapack_lite/f2c.h#L10
    https://raw.githubusercontent.com/numpy/numpy/maintenance/1.9.x/numpy/linalg/lapack_lite/dlapack_lite.c

My understanding from #5898 (comment) is that the blas-lite and lapack-lite included with numpy are already known to cause segfaults for these larger matrices (e.g. using dot) because of overflow issues like this.

I'm not 100% sure that this is what's going on with the segfault, but this is my best guess. Maybe wait for comments or a review from @pv or others before merging?

@pv
Copy link
Member
pv commented May 21, 2015

LGTM (assuming no typos)

jaimefrio added a commit that referenced this pull request May 21, 2015
@jaimefrio
Merge pull request #5899 from argriffing/improve-umath-linalg
9dba7a4 
MAINT: use more conservative integer types for umath linalg
@jaimefrio jaimefrio merged commit 9dba7a4 into numpy:master May 21, 2015
@jaimefrio
Copy link
Member

In it goes then. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@argriffing @jaimefrio @pv @alexbrc
0