Skip to content

Conversation

ngoldbaum
Copy link
Member

Fixes #28143.

I verified the test triggers TSAN warnings on main and doesn't anymore with this applied.

@ngoldbaum ngoldbaum added the 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703) label Jan 14, 2025
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jan 14, 2025
@ngoldbaum ngoldbaum force-pushed the patch-byteorder-race branch from 88c712a to a539193 Compare January 15, 2025 17:46
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Looks good, but it would be nice to remove the PyArray_DescrNew in the above branch (replace it with an INCREF).
(Can't "suggest" unfortunately)

If this doesn't change metadata (if you don't want check I am happy to just assume/hope!), then we could also just call PyArray_DESCR_REPLACE_CANONICAL unconditionally for all I care (yes, changes meaning slightly more, but OK).

If it does change the metadata, could skip backporting, but I suspect it doesn't and don't think it matters overall.

}
if (in_descr && in_descr->byteorder != NPY_IGNORE) {
in_descr->byteorder = NPY_NATIVE;
PyArray_DESCR_REPLACE_CANONICAL(in_descr);
Copy link
Member

Choose a reason for hiding this comment

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

A bit clearer maybe to just set in_descr = ... (with a new reference) in the first branch. Then we can call PyArray_DESCR_REPLACE_CANONICAL unconditionally.

Copy link
Member Author

@ngoldbaum ngoldbaum Jan 15, 2025

Choose a reason for hiding this comment

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

Unfortunately it looks like there are cases where PyArray_Check(op) is False but in_descr is NULL, so I still need the if (in_descr) check, but I think this is a lot simpler now.

@charris charris merged commit 1e10174 into numpy:main Jan 15, 2025
67 checks passed
charris pushed a commit to charris/numpy that referenced this pull request Jan 16, 2025
* BUG: Avoid data race in PyArray_CheckFromAny_int

* TST: add test

* MAINT: simplify byteswapping code in PyArray_CheckFromAny_int

* MAINT: drop ISBYTESWAPPED check
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jan 16, 2025
ngoldbaum added a commit to ngoldbaum/numpy that referenced this pull request Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Race on descr->byteorder under free threading
3 participants