-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Misaligned address sanitizer errors with csr_matrix_times_vector()
- leading to CRAN package failing additional tests
#1111
Comments
I believe I have found the source of this in the Stan Math library. This function allocates |
Aha! That might explain why I could make the sanitizer error disappear by adding/removing certain rows of data but not others. When the data structure results in I'm guessing any fix will take a little while to make it to CRAN, if it's an issue all the way upstream in Stan Math? I have until 2024-02-08 to resolve the error before I'm threatened with removal of my package, but I'll try to argue the case for retaining my package whilst this is fixed upstream. Thank you so much for looking into this! |
Hopefully the fix is small enough that the RStan team can backport it sooner |
@bgoodri I think it should be safe to cherry-pick the changes from stan-dev/math#3014 back to earlier versions to resolve this for rstan |
I'll try the backport |
Thanks both. CRAN didn't respond to my emails and have now just archived multinma, which is rather frustrating. Any news on if/when the backport is likely to be possible? If not I might submit an update to CRAN that temporarily avoids running the affected tests, it's not ideal but better than remaining archived. Happy to help in any way I can; I could run the sanitizer checks on a release candidate if that would be useful? Thanks again 🙂 |
Avoid tripping additional checks (stan-dev/rstan#1111)
I also got an e-mail from CRAN warning that geostan will be taken down if not fixed by feb 29 2024. It has an installation error and a note about gcc-UBSAN. I haven't had a chance to look closely yet but the installation error is coming after a matrix-vector product, so presumably this is connected to the same issue as @dmphillippo. https://cran.r-project.org/web/checks/check_results_geostan.html
|
@ConnorDonegan your CRAN note calls out |
Okay, and I see the changes here stan-dev/math#3014 have already been merged into the development branch, thanks @WardBrian. I have a couple weeks before geostan gets removed from CRAN, should I expect to see the changes released before then? |
I am going to backport this and the max / min thing and push another
StanHeaders to CRAN.
…On Thu, Feb 15, 2024 at 12:23 PM Brian Ward ***@***.***> wrote:
@ConnorDonegan <https://github.com/ConnorDonegan> your CRAN note calls
out csr_matrix_times_vector in the back trace so I'd say it is almost
certainly the same issue.
—
Reply to this email directly, view it on GitHub
<#1111 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKUQ2BDGMGKBHWBTRVTYTZAATAVCNFSM6AAAAABCL7YR46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBWGY3DMOBZGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes
…On Thu, Feb 15, 2024 at 12:47 PM Connor Donegan ***@***.***> wrote:
Okay, and I see the changes here stan-dev/math#3014
<stan-dev/math#3014> have already been merged
into the development branch, thanks @WardBrian
<https://github.com/WardBrian>. I have a couple weeks before geostan gets
removed from CRAN, should I expect to see the changes released before then?
—
Reply to this email directly, view it on GitHub
<#1111 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKQCLXQHYJIMJR77LP3YTZCZPAVCNFSM6AAAAABCL7YR46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBWG42DIOJZGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I see StanHeaders 2.32.6 is up on CRAN now, I take it this contains the backported fix? |
Yes, and the min/max thing. |
I notice that the This looks like the same issue again:
https://cran.r-project.org/web/checks/check_results_multinma.html |
I am not sure that it is the same error as the alignment thing in the OP (that was fixed). The geostan and multinma packages look like they might just be timing out on the CRAN server builds. |
The reason I thought it may be connected to this issue is that they're both terminating on matrix vector products again multinma: |
The original issue was about a runtime error, this seems to be different |
multinma is now failing again the UBSAN tests on CRAN, with what looks like the same issue: https://www.stats.ox.ac.uk/pub/bdr/memtests/gcc-UBSAN/multinma/tests/testthat.Rout Installing the latest StanHeaders 2.32.7, the small reprex in the first post recreates the errors in rocker/r-devel-san. Downgrading to StanHeaders 2.32.6 doesn't give the UBSAN errors. Is it possible that the latest 2.32.7 StanHeaders missed including this patch? I have been given until 2024-05-22 to have this fixed before multinma is archived again. |
@dmphillippo yes, it appears that SH 2.32.7 is missing the patch to @bgoodri the changes to |
I'll fix it |
Thanks @bgoodri! Any chance this will be patched on CRAN by 22nd? I appreciate it must take a while getting through all the revdep checks etc. |
I see StanHeaders 2.32.8 is on CRAN now, but I'm still seeing the sanitizer errors with this version. I think that the stack_alloc patch may still be missing? The other min/max patch does seem to be in this version, from what I can tell. Sorry! |
@bgoodri just to follow up on this, the multinma and geostan packages are now both archived from CRAN due to the sanitiser warnings. As far as I can tell StanHeaders 2.32.8 is still missing the patch from stan-dev/math#3014 that was added in 2.32.6 to fix this - @WardBrian may be able to confirm? |
2.32.8's source code seems to be missing stan-dev/math#3014, but does include stan-dev/math#3024 |
@dmphillippo StanHeaders 2.32.9 on CRAN now has the padding fix. |
Summary:
Sparse matrix arithmetic using
csr_matrix_times_vector()
seems to trigger sanitizer "misaligned address" errors.Description:
My package multinma that fits models using rstan has been flagged as failing additional tests in gcc-UBSAN. Closer inspection seems to point to
csr_matrix_times_vector()
as the cause.I have also opened a thread on discourse.
Reproducible Steps:
This small model recreates the sanitizer errors:
Commenting out the transformed parameter removes the sanitizer warnings, so this seems to be a problem with
csr_matrix_times_vector()
.Current Output:
Details
RStan Version:
R Version:
Issue recreated using the rocker/r-devel-san docker container, which builds r-devel with compiler sanitizers.
Operating System:
Linux run in rocker/r-devel-san docker container. Issue first appeared on CRAN gcc-UBSAN test environment (also a flavour of linux).
The text was updated successfully, but these errors were encountered: