Skip to content

Revert change from #532 due to unsafe use of static buffer #1852

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

Closed
wants to merge 1 commit into from
Closed

Revert change from #532 due to unsafe use of static buffer #1852

wants to merge 1 commit into from

Conversation

martin-frbg
Copy link
Collaborator

see #1844

@hjbreg
Copy link

hjbreg commented Nov 4, 2018

This does not work for #1847, results are still incorrect.

@martin-frbg
Copy link
Collaborator Author

#1847 seems unrelated now (possibly mingw/Windows-only and specific to Sandybridge).
And it seems it may be possible to keep the optimization from #532 by declaring either #pragma omp threadprivate(y_dummy) (OpenMP) or static __thread FLOAT y_dummy (pthreads) rather than just static FLOAT y_dummy.

@seberg
Copy link
Contributor

seberg commented Nov 5, 2018

I can confirm that this (at least for pthreads I think), fixes the specific issue from gh-1844/numpy/numpy#11046

A bit curious, is there a way to add tests for such issues?

@martin-frbg
Copy link
Collaborator Author

"this" being the revert as per this PR, or my belated suggestion to make the y_dummy array thread-local ?
I guess we could add a test case for this to the "utest" directory to keep it from reappearing, but (a) it would need to be a self-contained C program (or function) rather than a python/numpy script and (b) it would still be a bit outlandish compared to the other, simple checks. I guess in an ideal world the test would have been someone commenting "stop, you cannot do that" on the PR, but this project happens to have very few contributors compared to the number of (often unwitting indirect) users it managed to acquire over the years.

@seberg
Copy link
Contributor

seberg commented Nov 5, 2018

Oh, sorry with "this" I mean the PR. I have been hesitant to dive in the code.

Many users and few contributors is a general issue and I am sure it is much worse for OpenBLAS compared to numpy. Maybe we will add the "test" to numpy, sometimes downstream is a good "test suit" ;).
I guess ideally the PR changing such things has the tests to make everyone confident it works – even without inspecting the code perfectly. But, testing threading stuff is hard I guess :(.

@martin-frbg
Copy link
Collaborator Author

I think I am going to rework this to keep the problematic optimization, while making sure that it only gets built when conditions allow thread-safe execution. (That is, with the y_dummy array either threadprivate in conjunction with USE_OPENMP or annotated with __thread where the compiler supports it)

@martin-frbg
Copy link
Collaborator Author

Closing as superseded by my PR #1865

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants