Skip to content

BUG: fix alloc tls destruction on gcc#30673

Merged
seberg merged 5 commits into
numpy:mainfrom
kumaraditya303:fix-alloc
Jan 20, 2026
Merged

BUG: fix alloc tls destruction on gcc#30673
seberg merged 5 commits into
numpy:mainfrom
kumaraditya303:fix-alloc

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Jan 19, 2026

Copy link
Copy Markdown
Contributor

Addresses the issue discussed at #30499 (comment)

@seberg

seberg commented Jan 19, 2026

Copy link
Copy Markdown
Member

Thanks for finding the solution. I am wondering if we should add a short comment about it, but not super important.
Ping @ngoldbaum if just because it might be interesting to him.

I do wonder if the armhf failure may be real though :(.

@kumaraditya303

Copy link
Copy Markdown
Contributor Author

Thanks for finding the solution. I am wondering if we should add a short comment about it, but not super important.

I added a comment with link to the gcc issue.

@ngoldbaum

Copy link
Copy Markdown
Member

It looks like the armhf tests are using gcc 13.3, in case it helps. The tests crash on test_arrayprint which has a multithreaded test, so I bet this is related.

@seberg

seberg commented Jan 19, 2026

Copy link
Copy Markdown
Member

Hmmm, could it be that we have to explicitly initialize available to zero now!? (just wondering what the heck could in theory fail...)

@kumaraditya303

Copy link
Copy Markdown
Contributor Author

It failed again with a crash. Not sure how to debug it on such a platform.

@kumaraditya303

Copy link
Copy Markdown
Contributor Author

I am guessing it might be because sizeof(tls_cache) = 66560UL which might be too large for a single allocation or something on armhf

Comment thread numpy/_core/src/multiarray/alloc.cpp Outdated
@seberg

seberg commented Jan 19, 2026

Copy link
Copy Markdown
Member

I am guessing it might be because sizeof(tls_cache) = 66560UL which might be too large for a single allocation or something on armhf

Ohhh, that would make sense, but in that case we would have to split it up? Debugging it certainly won't be fun :(.

@ngoldbaum

Copy link
Copy Markdown
Member

Not sure how to debug it on such a platform.

I've used the tmate action to debug stuff like this on a github runner: https://github.com/mxschmitt/action-tmate

Otherwise I think you have an ARM mac, right? You should be able to run Linux in a VM or use orbstack or some other docker environment to run ARM Linux docker containers locally on your Mac to get the armhf container the CI job uses.

@kumaraditya303

Copy link
Copy Markdown
Contributor Author

It passed with splitting up the cache so it was indeed the size issue.

Comment thread numpy/_core/src/multiarray/alloc.cpp

@ngoldbaum ngoldbaum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. I'd appreciate it if @seberg can confirm this fixes the leaks he saw in valgrind.

@seberg

seberg commented Jan 19, 2026

Copy link
Copy Markdown
Member

I'll test it once more in a bit (it worked with the earlier version, so I think it should be OK).

This large allocation erroring does make me wonder whether it would be cleaner to allocate this so that thread-local is just the pointer to the cache object?!

@seberg seberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, I can confirm that valgrind is happy with this (and so is GCC 13.3.0).

(If nobody has a thought/opinion if an allocation is better, I'll probably just put it in soon. -- I am happy to follow up myself if we should allocate.)

@charris

charris commented Jan 20, 2026

Copy link
Copy Markdown
Member

Backport?

@ngoldbaum

Copy link
Copy Markdown
Member

No, #30499 wasn't and shouldn't be backported.

@kumaraditya303

Copy link
Copy Markdown
Contributor Author

This large allocation erroring does make me wonder whether it would be cleaner to allocate this so that thread-local is just the pointer to the cache object?!

That might be a little slower because statically allocated blocks have better locality than block allocated on heap, given that it is not an issue on major platforms I don't think we should change it.

@seberg seberg merged commit fba740c into numpy:main Jan 20, 2026
82 of 83 checks passed
@seberg

seberg commented Jan 20, 2026

Copy link
Copy Markdown
Member

Thanks for the multiple follow-ups on this one @kumaraditya303. Surprisingly weird/tricky/subtle!

@kumaraditya303 kumaraditya303 deleted the fix-alloc branch January 20, 2026 12:06
ngoldbaum added a commit to ngoldbaum/numpy that referenced this pull request Apr 9, 2026
ngoldbaum added a commit to ngoldbaum/numpy that referenced this pull request Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants