Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fiber: malloc call fix #3241

Closed
wants to merge 3 commits into from
Closed

Conversation

denizzzka
Copy link
Contributor

@denizzzka denizzzka commented Oct 19, 2020

Current code silently ignores malloc call even if it available - import is forgotten.

This PR fixes it and adds assert checks to avoid this for other whan Windows or Posix platforms.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @denizzzka! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3241"

{
free( m_pmem );
}
}
else
static assert(false);
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
static assert(false);
static assert(false, "`Fiber` support is not available on this platform, please raise an issue");

Or something similar.

Copy link
Contributor Author

@denizzzka denizzzka Oct 19, 2020

Choose a reason for hiding this comment

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

Not needed for now - there are no easy ways to get to the versions other than windows or posix.

Copy link
Member

Choose a reason for hiding this comment

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

It's not rare to have people stepping in and trying to port D to an architecture / OS we currently do not support.
Also, some people do funky stuff with druntime, building their own.
Those errors message ensure that they get some kind of clue of what is failing at a minimum cost to us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If everything was as you say, then malloc already would work. :-)

Proposed text is a comment, and I consider what obvious comments like this are bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the contrary, the next poor schmuck that tries to build for a micro controller, e.g. this guy https://forum.dlang.org/thread/erwfgtigvcciohllvaos@forum.dlang.org , is going to hit that error and get

src/core/thread/fiber.d(1018): Error: static assert:  *false* is false.

Rather than

src/core/thread/fiber.d(1018): Error: static assert:  "*Fiber* support is not available on this platform, please raise an issue"

Copy link
Contributor Author

@denizzzka denizzzka Oct 19, 2020

Choose a reason for hiding this comment

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

She uses Posix, as I understand. Both cases will force to look into code.

I think the main focus should be on the fundamental removal of such branching code, and not commenting on it with pretty messages.

@n8sh
Copy link
Member

n8sh commented Oct 19, 2020

Current code silently ignores malloc call even if it available - import is forgotten.

This PR fixes it

Good! If not removing valloc perhaps also import core.sys.posix.stdlib.

and adds assert checks to avoid this for other whan Windows or Posix platforms.

Why? In allocStack & freeStack the only concern is the ability to allocate & deallocate memory. If malloc and free are defined in core.stdc.stdlib then they must work according to ANSI C. There are places where system-specific behavior is required and there is an error if the present system lacks support, like initStack. Let's leave the error messages for those places!

@denizzzka
Copy link
Contributor Author

denizzzka commented Oct 20, 2020

Good! If not removing valloc perhaps also import core.sys.posix.stdlib.

It is already imported above. I added redundand to clear things.

If malloc and free are defined in core.stdc.stdlib then they must work according to ANSI C.

My target (outside of ~master druntime) is to adding support for any platforms, including which not providing C API.

Let's leave the error messages for those places!

Here is many similar places where similar assertion will be triggered (or also must be added):

$ grep -r "version (Posix)" --include='*.d' . | wc -l
309

{
version (Posix) import core.sys.posix.sys.mman; // mmap, MAP_ANON
import core.sys.posix.sys.mman; // mmap, MAP_ANON
Copy link
Member

Choose a reason for hiding this comment

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

This imported module will always define mmap or else compilation will fail with static assert(false, "Unsupported platform"). So there seems to be no way the malloc branch can ever be reached.

version (CRuntime_Glibc)
{
static if (__USE_LARGEFILE64) void* mmap64(void*, size_t, int, int, int, off_t);
static if (__USE_FILE_OFFSET64)
alias mmap = mmap64;
else
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (Darwin)
{
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (FreeBSD)
{
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (NetBSD)
{
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (OpenBSD)
{
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (DragonFlyBSD)
{
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (Solaris)
{
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (CRuntime_Bionic)
{
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (CRuntime_Musl)
{
static if (__USE_LARGEFILE64) void* mmap64(void*, size_t, int, int, int, off_t);
static if (__USE_FILE_OFFSET64)
alias mmap = mmap64;
else
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else version (CRuntime_UClibc)
{
static if (__USE_LARGEFILE64) void* mmap64(void*, size_t, int, int, int, off64_t);
static if (__USE_FILE_OFFSET64)
alias mmap = mmap64;
else
void* mmap(void*, size_t, int, int, int, off_t);
int munmap(void*, size_t);
}
else
{
static assert(false, "Unsupported platform");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I brought this fix by accident and did git push -f. :-(

I think we just need to get rid of these __traits( compiles, mmap ) at whole file replacing them by version(POSIX_MMAP), etc.

@denizzzka denizzzka closed this Oct 28, 2020
@denizzzka
Copy link
Contributor Author

This bug is still here, but I don't have time at the moment to deal with it. It will be great if someone fixes it. :-)

@denizzzka denizzzka reopened this Oct 28, 2020
@denizzzka denizzzka closed this Nov 15, 2020
@denizzzka denizzzka deleted the fiber_malloc_fix branch August 8, 2021 13:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants