core.thread refactor: osthread module

[baziotis]

This is the start of an effort to refactor core.thread. With Nicholas Wilson's help, this takes the form of a separate folder that will split thread to modules. Currently, the plan is to split it to:

• osthread : Low-level, OS-dependent code for threads.
• fiber : The interface for fibers.
• context : The stack context needed for threads and fibers.

osthread seems to be the easiest to start from so that the review is easier.

[thewilsonator]

You should start by moving core/thread.d into core/thread/package.d and then move things out of there into other files, making private symbols package(core.thread) where needed.

It will be much easier to keep track of things that way.

[baziotis]

Ok, I just didn't get the last phrase:

making private symbols package(core.thread) where needed.

[thewilsonator]

some symbols in core.thread are private.

When you move symbol around that require access to those symbols, as they are now in different modules they will give an error if used.

making those symbols package(core.thread) will resolve that problem and still have the symbols not be visible outside of core.thread

[baziotis]

Ok, thanks. I didn't know that there's the package specifier. I'll learn about it and update the PR.

[PetarKirov]

@baziotis thanks for working on this! I will try to review your later today or tomorrow.

[baziotis]

@ZombineDev Don't mention it. It might seem like a big PR but it's actually not. I just moved the OS-dependent stuff from core/thread.d to /thread/osthread.d. I'll continue with Nicholas' proposal.

[Geod24]

I'll continue with Nicholas' proposal.

@thewilsonator 's proposal was to start with something else. And IMO it makes a lot of sense.
This PR is currently only adding code, nothing is removed yet, so I assume this is in essence unused code ? Which also means untested. It's harder to review when the changes are actually disconnected, like adding in a PR and removing in another (unless there's a good reason to do so, e.g. when dealing with cross-repository dependencies).

[baziotis]

@Geod24 To be honest, I'm not sure what is your point. This PR is a draft because it is not supposed to be merged as it is. It basically asks "have I moved the correct amount of things (i.e. the OS stuff) from thread.d to osthread". Practically, it adds no code.

[thewilsonator]

have I moved the correct amount of things

In this case, no, because there should be a (roughly) equal amount of code removed.

[baziotis]

Can I have some help on why the tests are failing? I can't seem to find a way to fix them.

[aG0aep6G]

Can I have some help on why the tests are failing? I can't seem to find a way to fix them.

I think you forgot to add thread/package.d to the make files.

[baziotis]

I think you forgot to add thread/package.d to the make files.

I thought it wasn't needed, thanks for the response. I added in all but DOCS. Unfortunately, it doesn't seem to fix the error.

[aG0aep6G]

Unfortunately, it doesn't seem to fix the error.

The error is different now. You've got a merge conflict in mak/DOCS. You need to rebase.

[baziotis]

The error is different now. You've got a merge conflict in mak/DOCS. You need to rebase.

Yes, I had this error in the previous commit as well. But, besides this, there's also the previous error for example in circleci.
Edit: Thanks for the proposal to rebase.

[baziotis]

Oh crap, I thought Ready for review makes it an actual PR, not requesting actual reviews.

Edit: Should this somehow be undone or is it supposed to be that way?

[dlang-bot]

Thanks for your pull request and interest in making D better, @baziotis! 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 fetch digger
dub run digger -- build "master + druntime#2689"

[lesderid]

Edit: Should this somehow be undone or is it supposed to be that way?

Supposed to be that way.

[thewilsonator]

There are a couple of things I'd like to see changed, but theres no point keeping thins waiting any further.

[baziotis]

There are a couple of things I'd like to see changed, but theres no point keeping thins waiting any further.

You can tell me. :)
As this is a PR in a series, trying to break the changes in reasonable PRs.
To the best of my knowledge, I have done the things that were discussed in this one.

[thewilsonator]

#2689 (comment)
Fair enough.
Also fair enough.

For the next one split out the context stuff into thread/context.d like in the diff I gave you. (it has some typos and naming inconsistencies but I'm sure you'll discover that when you try to compile it ;) )

[baziotis]

#2689 (comment)
Fair enough.
Also fair enough.

Yes as said here: #2689 (comment)
I think it makes this easier to do in a separate PR.

For the next one split out the context stuff into thread/context.d like in the diff I gave you. (it has some typos and naming inconsistencies but I'm sure you'll discover that when you try to compile it ;) )

Haha yes I don't compile it. As you can see from this PR there are a lot of subtle things
in these changes so I take your idea and try to figure out how to make the implementation work.

[thewilsonator]

Indeed, I look forward to it.

[rainers]

As one of the tests failing in https://dev.azure.com/dlanguage/dmd/_build/results?buildId=4590 and other PRs is runnable/test15779.d which checks exceptions in fibers, I suspect it has been introduced by this PR. Any idea what might be causing this?

Apart from the auto tester, druntime just runs a partial win64 test on Appveyor. @wilzbach Can we easily run the azure tests from dmd for druntime, too?

[thewilsonator]

Any idea what might be causing this?

No, but I note that there are a couple of unittests related to exceptions and fibers that are explicitly disabled for Win32. Maybe a subtle change has caused it to hit that bug?

[rainers]

Indeed, the disabled unittests look very similar. I have failed to reproduce the issues reported by the azure CI locally, will try the unittests tonight.

[thewilsonator]

Of note, that test enables stack stomping.

[baziotis]

Follow-up: #2821