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.

Sign up for GitHub

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
Merged

Fix pthread_attr_t having 9 elems instead of 10 #15530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions system/lib/libc/musl/arch/emscripten/bits/alltypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

emscripten/src/library_pthread.js

Lines 778 to 780 in ec42ea2

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):

emscripten/src/library_pthread.js

Line 667 in ec42ea2

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

int __i[10];
volatile int __vi[10];
unsigned __s[10];
} __u;
#ifdef __EMSCRIPTEN__
// For canvas transfer implementation in Emscripten, use an extra control field
Expand Down
4 changes: 2 additions & 2 deletions tests/reference_struct_info.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,8 +1343,8 @@
"tsd": 64
},
"pthread_attr_t": {
"__size__": 40,
"_a_transferredcanvases": 36
"__size__": 44,
"_a_transferredcanvases": 40
},
"sockaddr": {
"__size__": 16,
Expand Down