Skip to content
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

Errno fix #463

Merged
merged 4 commits into from
Feb 24, 2022
Merged

Errno fix #463

merged 4 commits into from
Feb 24, 2022

Conversation

rmn30
Copy link
Contributor

@rmn30 rmn30 commented Feb 21, 2022

Here is a fix for #461 and the associated test. I've done separate commits for the test and the bug fix. Not sure if you would prefer them to be squashed?

Previously this test would emit an error message but not
abort if the errno was not as expected on failure. This
was because the return in the null == true case prevented the
check for failed == true at the end of check_result from
being reached. To resolve this just abort immediately instead
of instead of setting failed to true, as in the null case.

See microsoft#461.
@rmn30 rmn30 force-pushed the errno_fix branch 2 times, most recently from cfbb55a to f88e521 Compare February 21, 2022 17:31
@rmn30 rmn30 marked this pull request as draft February 21, 2022 17:34
@rmn30
Copy link
Contributor Author

rmn30 commented Feb 21, 2022

After a bit of tinkering and some more thought I am withdrawing this PR because I think this is not the correct solution: snmalloc should not be setting errno except for functions that are defined to do so. Probably the override wrapper is the correct place for this (at the cost of a nullptr check).

This was not being done for certain cases as per microsoft#461.
Note that malloc and calloc are probably still not setting errno for
those cases but this is not currently tested...
@mjp41
Copy link
Member

mjp41 commented Feb 21, 2022

I don't want a branch on the fast path of malloc, and setting errno at the outer level will lead to this. It has to be buried deep inside.

@rmn30
Copy link
Contributor Author

rmn30 commented Feb 22, 2022

In that case setting errno should become part of snmalloc's API and we will have to save and restore errno in the posix_memalign implementation because it explicitly does not set it. Implementations of the other functions already check for alloc returning null and set errno, although the realloc implementation is a bit broken and I will fix it.

@mjp41
Copy link
Member

mjp41 commented Feb 22, 2022

Does posix_memalign require it not to be set, or that you can't depend on it being set?

In particular, I read
https://sourceware.org/bugzilla/show_bug.cgi?id=83#c1

Which implies we can set it.

@davidchisnall
Copy link
Collaborator

POSIX2018 is pretty liberal WRT errno:

  • It is never set to 0 by any POSIX function.
  • It is undefined after any call to a POSIX function that is not explicitly stated to set it.
  • It is undefined after a successful call to a POSIX function that is documented to set errno on failure.

posix_memalign is not defined to set errno, so we're free to set or not set it as we desire on failure. On success we may set it to whatever we want as long as we do not transition it from a non-zero to a zero state.

@mjp41
Copy link
Member

mjp41 commented Feb 22, 2022

To get this right, some of the other pals should probably set it. Like

# ifdef PLATFORM_HAS_VIRTUALALLOC2
template<bool state_using>
static void* reserve_aligned(size_t size) noexcept
{
SNMALLOC_ASSERT(bits::is_pow2(size));
SNMALLOC_ASSERT(size >= minimum_alloc_size);
DWORD flags = MEM_RESERVE;
if (state_using)
flags |= MEM_COMMIT;
// If we're on Windows 10 or newer, we can use the VirtualAlloc2
// function. The FromApp variant is useable by UWP applications and
// cannot allocate executable memory.
MEM_ADDRESS_REQUIREMENTS addressReqs = {NULL, NULL, size};
MEM_EXTENDED_PARAMETER param = {
{MemExtendedParameterAddressRequirements, 0}, {0}};
// Separate assignment as MSVC doesn't support .Pointer in the
// initialisation list.
param.Pointer = &addressReqs;
void* ret = VirtualAlloc2FromApp(
nullptr, nullptr, size, flags, PAGE_READWRITE, &param, 1);
return ret;
}
# endif
static void* reserve(size_t size) noexcept
{
return VirtualAlloc(nullptr, size, MEM_RESERVE, PAGE_READWRITE);
}

Windows Pal should probably set errno when VirtualAlloc* returns nullptr.

src/test/func/malloc/malloc.cc Outdated Show resolved Hide resolved
src/test/func/malloc/malloc.cc Outdated Show resolved Hide resolved
@rmn30
Copy link
Contributor Author

rmn30 commented Feb 22, 2022

I think this version avoids extra overhead on the fast path except in the case of calloc which would previously attempt to zero a null pointer if allocation failed. Looks like Qemu is crashing on arm build :-(

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing

@rmn30 rmn30 marked this pull request as ready for review February 24, 2022 09:58
@rmn30 rmn30 merged commit 86aa286 into microsoft:main Feb 24, 2022
@rmn30 rmn30 deleted the errno_fix branch February 24, 2022 11:14
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.

4 participants