Skip to content

Fix pthread_attr_t having 9 elems instead of 10 #15530

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
merged 1 commit into from
Nov 15, 2021

Conversation

aardappel
Copy link
Collaborator

@aardappel aardappel commented Nov 15, 2021

Regression introduced here: #13006
We are relying on them having 10 elems thru the _a_prio field (in wasm64) and also offsets in library_pthread.js

@aardappel aardappel requested a review from sbc100 November 15, 2021 20:26
@@ -91,9 +91,9 @@ typedef long suseconds_t;
#if defined(__NEED_pthread_attr_t) && !defined(__DEFINED_pthread_attr_t)
typedef struct {
union {
int __i[9];
volatile int __vi[9];
unsigned __s[9];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect this to match the code in upstream musl:

https://github.com/emscripten-core/musl/blob/85e0e3519655220688e757b9d5bfd314923548bd/include/alltypes.h.in#L85

i.e. it would expect it to with be 14 or 9 for each other.. what am I missing here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure.. we have:

#define __SU (sizeof(size_t)/sizeof(int))
#define _a_prio __u.__i[3*__SU+3]

so accessing that field causes:

system\lib\libc\musl\src\thread\pthread_attr_get.c:22:26: error: array index 9 is past the end of the array (which contains 9 elements)
      [-Werror,-Warray-bounds]
        param->sched_priority = a->_a_prio;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you copy what musl does and do sizeof(long)==8?14:9 .. for consistency with upstream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that code was never in there and would give us suddenly 14 fields in wasm32, which also would require our JS bindings to further become conditional on wasm32/64, see e.g. https://github.com/emscripten-core/emscripten/pull/15229/files#diff-31ce84d9e9748b123befe225a55c67718b58011f9b0a099c821e5c5f0019b1fa

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK fair enough. This change lgtm for now then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, but don't the JS bindings already need to have different offsets for wasm64 since _a_prio is different for wasm64 (for example)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure if we access _a_prio in JS (we access it in C), but we access the canvas names field which is currently at a single offset in both. See link above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, it does look like we access _a_stacksize, _a_stackaddr and _a_detach using hardcoded constants in the JS code:

stackSize = {{{ makeGetValue('attr', 0/*_a_stacksize*/, 'i32') }}};
stackBase = {{{ makeGetValue('attr', 8/*_a_stackaddr*/, 'i32') }}};
if ({{{ makeGetValue('attr', 12/*_a_detach*/, 'i32') }}}) {

These probably need to be fixes (although not necessarily as part of this PR).

The reference to the canvas is not longer hardcoded and uses the struct info so it should be fine, even if the struct layout varies between 32 and 64 (which it does):

var transferredCanvasNames = attr ? {{{ makeGetValue('attr', C_STRUCTS.pthread_attr_t._a_transferredcanvases, POINTER_TYPE) }}} : 0;

Regression introduced here: #13006
We are relying on them having 10 elems thru the `_a_prio` field (in wasm64) and also offsets in library_pthread.js
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.

lgtm for now since it fixes the wasm64 libc build.

@aardappel aardappel merged commit 72fd2b4 into emscripten-core:main Nov 15, 2021
mmarczell-graphisoft pushed a commit to GRAPHISOFT/emscripten that referenced this pull request Jan 5, 2022
Regression introduced here: emscripten-core#13006
We are relying on them having 10 elems thru the `_a_prio` field (in wasm64) and also offsets in library_pthread.js
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