Skip to content

Conversation

kleisauke
Copy link
Collaborator

@kleisauke kleisauke commented Mar 16, 2024

No description provided.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 18, 2024

Oh, I guess we should make __builtin_thread_pointer work. I've not seen that before.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 18, 2024

Could you open an upsteam llvm issue regarding fatal error: error in backend: Cannot select: intrinsic %llvm.thread.pointer ?

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Thanks!

Should we wait until dev-slice lands in their main branch? I would guess that would be when they consider it stable, but I don't actually know their dev practices.

return ENOMEM;
}
return 0;
*addr = emmalloc_memalign(try_alignment, size);
Copy link
Member

Choose a reason for hiding this comment

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

Did the emmalloc alignment limitations get fixed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was fixed via PR #21905 in the meantime. I also removed MIN_EMMALLOC_ALIGN initially, but let's leave that for now.

@kleisauke kleisauke changed the title mimalloc: sync with upstream dev-slice branch Update mimalloc to 2.1.7 Nov 23, 2024
@kleisauke
Copy link
Collaborator Author

kleisauke commented Nov 23, 2024

Revised this PR to update mimalloc to 2.1.7 instead. This is ready for review now.

Could you open an upsteam llvm issue regarding fatal error: error in backend: Cannot select: intrinsic %llvm.thread.pointer ?

I just opened issue llvm/llvm-project#117433 for this. Commit 0dc7223 defines MI_LIBC_MUSL to workaround this.

@kleisauke kleisauke marked this pull request as ready for review November 23, 2024 14:15
# disable `assert()` in emmalloc
'-DNDEBUG',
# avoid use of `__builtin_thread_pointer()`
# FIXME: https://github.com/llvm/llvm-project/issues/117433
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we want a FIXME here since we are using musl as our libc so this seems correct in any case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

'-DMI_MALLOC_OVERRIDE',
# TODO: add build modes that include debug checks 1,2,3
'-DMI_DEBUG=0',
# disable `assert()` in emmalloc
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# disable `assert()` in emmalloc
# disable `assert()` in mimalloc

But do we not want this in debug builds?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do, I think that we can/should make that a separate change.

Copy link
Collaborator Author

@kleisauke kleisauke Nov 25, 2024

Choose a reason for hiding this comment

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

I think the comment is correct in this case, mimalloc uses mi_assert, which is already disabled.

#if (MI_DEBUG)
// use our own assertion to print without memory allocation
void _mi_assert_fail(const char* assertion, const char* fname, unsigned int line, const char* func );
#define mi_assert(expr) ((expr) ? (void)0 : _mi_assert_fail(#expr,__FILE__,__LINE__,__func__))
#else
#define mi_assert(x)
#endif

However, we also want to disable the assertions in the underlying emmalloc allocator, which we build as part of mimalloc:

# Build all of mimalloc, and also emmalloc which is used as the system
# allocator underneath it.

src_files += [utils.path_from_root('system/lib/emmalloc.c')]

* Debugging:
*
* - If not NDEBUG, runtime assert()s are in use.

#ifdef NDEBUG
#define assert(x) (void)0
#else
#define assert(x) ((void)((x) || (__assert_fail(#x, __FILE__, __LINE__, __func__),0)))
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it's for the underlying allocator. In that case it sounds ok. But can it be a separate change, or was it necessary for some reason to do in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no specific reason to address it in this PR; it was discovered by accident while working on this. Split into PR #23016.

@kripken kripken merged commit 27d7daa into emscripten-core:main Nov 26, 2024
28 checks passed
@kripken
Copy link
Member

kripken commented Nov 26, 2024

Thanks @kleisauke !

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.

3 participants