Skip to content

Fill correct stack info in pthread_getattr_np stub #23887

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

Merged

Conversation

hoodmane
Copy link
Collaborator

See use case here:
https://github.com/python/cpython/pull/130398/files#diff-c22186367cbe20233e843261998dc027ae5f1f8c0d2e778abfa454ae74cc59deR365-R380

I guess for stack protection, this only enables introspecting whether we run out of the spill stack. I'm not sure whether to expect running out of spill stack or running out of true stack to be more common. But at least this unbreaks the code.

See use case here:
https://github.com/python/cpython/pull/130398/files#diff-c22186367cbe20233e843261998dc027ae5f1f8c0d2e778abfa454ae74cc59deR365-R380

I guess for stack protection, this only enables introspecting whether we run out
of the spill stack. I'm not sure whether to expect running out of spill stack or
running out of true stack to be more common. But at least this unbreaks the
code.
@hoodmane hoodmane force-pushed the pthread-getattr-np-stub-return-stack-info branch from 7c2c401 to db7b108 Compare March 11, 2025 10:30
hoodmane added a commit to hoodmane/cpython that referenced this pull request Mar 11, 2025
… Emscripten

Emscripten reports having `pthread_get_stackaddr_np` but it does nothing.
I opened an upstream PR to actually fill in the stack information:
emscripten-core/emscripten#23887

Until we can update to an Emscripten version that includes this fix, we
can work around it by using `#define`s to replace `pthread_get_stackaddr_np`
with the modified version.
@sbc100 sbc100 enabled auto-merge (squash) March 11, 2025 21:30
@hoodmane
Copy link
Collaborator Author

The closure tests are now failing for me locally with:

building:ERROR: Closure compiler run failed:

building:ERROR: /tmp/emtest_aihv0i03/emscripten_temp_z4b7qfms/a.out.js:121:4: ERROR - [JSC_UNDEFINED_VARIABLE] variable assert is undeclared
  121|     assert(typeof data == 'object');

@sbc100
Copy link
Collaborator

sbc100 commented Mar 12, 2025

I experimented with using static struct initializer instead but it looks like it actually caused and overall regression: #23904

hoodmane added a commit to hoodmane/cpython that referenced this pull request Mar 12, 2025
… Emscripten

Emscripten reports having `pthread_get_stackaddr_np` but it does nothing.
I opened an upstream PR to actually fill in the stack information:
emscripten-core/emscripten#23887

Until we can update to an Emscripten version that includes this fix, we
can work around it by using `#define`s to replace `pthread_get_stackaddr_np`
with the modified version.
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Nice! LGTM

@hoodmane
Copy link
Collaborator Author

hoodmane commented Mar 12, 2025

I guess this is still a few bytes bigger than before because we added pthread_getattr_np.c.

@@ -1 +1 @@
129166
129206
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like a 40 byte increase is more than I would expect from this change.. I was hoping for more like 6 bytes (2 bytes each for the 3 extra store instructions).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well we also added pthread_getattr_np.c which looks like:

int pthread_getattr_np(pthread_t t, pthread_attr_t *a)
{
	*a = (pthread_attr_t){0};
	a->_a_detach = t->detach_state>=DT_DETACHED;
	a->_a_guardsize = t->guard_size;
	a->_a_stackaddr = (uintptr_t)t->stack;
	a->_a_stacksize = t->stack_size;
	return 0;
}

Presumably the other 34 bytes come from that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But that file should not linking into any of those tests. It should only be linked into the (rare) programs that use it.

@sbc100 sbc100 merged commit d6ebc74 into emscripten-core:main Mar 12, 2025
28 checks passed
@hoodmane hoodmane deleted the pthread-getattr-np-stub-return-stack-info branch March 13, 2025 07:55
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.

2 participants