Fixes compilation of ucontext_t based Fibers implementations

[denizzzka]

Fixes issue brought in #16695 (#16695 (comment))

But also I found that (probably) CI execution was broken before #16695 for ucontext_t-based Fiber implementations

I have no systems that using ucontext_t and I "emulated" them on my x86_64 Linux system by disabling other context switching approaches by using temporary code commenting:

diff --git a/druntime/src/core/thread/fiber/package.d b/druntime/src/core/thread/fiber/package.d
index 2112e23258..10101c7075 100644
--- a/druntime/src/core/thread/fiber/package.d
+++ b/druntime/src/core/thread/fiber/package.d
@@ -147,12 +147,12 @@ package
 
     version (Posix)
     {
-        version (AsmX86_Windows)    {} else
-        version (AsmX86_Posix)      {} else
-        version (AsmX86_64_Windows) {} else
-        version (AsmX86_64_Posix)   {} else
-        version (AsmExternal)       {} else
-        {
+        //~ version (AsmX86_Windows)    {} else
+        //~ version (AsmX86_Posix)      {} else
+        //~ version (AsmX86_64_Windows) {} else
+        //~ version (AsmX86_64_Posix)   {} else
+        //~ version (AsmExternal)       {} else
+        //~ {
             // NOTE: The ucontext implementation requires architecture specific
             //       data definitions to operate so testing for it must be done
             //       by checking for the existence of ucontext_t rather than by
@@ -160,7 +160,7 @@ package
             //       an obsolescent feature according to the POSIX spec, so a
             //       custom solution is still preferred.
             import core.sys.posix.ucontext : getcontext, makecontext, MINSIGSTKSZ, swapcontext, ucontext_t;
-        }
+        //~ }
     }
 }
 
@@ -330,7 +330,7 @@ package
                 jmp ECX;
             }
         }
-        else version (AsmX86_64_Posix)
+        else version (AsmX86_64_Posix_disabled)
         {
             asm pure nothrow @nogc
             {

After that I successfully compiled and run tests by command:

$ make druntime-test

and got error:

****** FAIL debug64 core.thread.fiber.base
core.exception.AssertError@src/core/thread/fiber/base.d(690): Derived fiber increment.

But same error I got then compiling commit e3aee67 that was made few days earlier than #16695

[dlang-bot]

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 + dmd#20923"

[denizzzka]

In the future I plan to get rid of ucontext_t in base.d completely. But first it is need to fix test runs for such platforms (if unless this is some kind of glitch in my "emulation", of course)

[denizzzka]

But same error I got then compiling commit e3aee67 that was made few days earlier than #16695

I replaced Fiber implementation by older one from f67d940 (Wed Jan 18 04:36:06 2023 +0000) with same result

This is that Fiber version after the "Great druntime Merge" #14281

To make bisection for this further back is need to build old versions of the compiler. If someone has this set up, it would be great to get help by bisection result with the commit where ucontext_t swapcontext was broken.

Don't want to lose Posix swapcontext opportunity because it can suddenly help if problems are found in our assembler code or on implementing new architectures/platforms

[ibuclaw]

But same error I got then compiling commit e3aee67 that was made few days earlier than #16695

I replaced Fiber implementation by older one from f67d940 (Wed Jan 18 04:36:06 2023 +0000) with same result

This is that Fiber version after the "Great druntime Merge" #14281

To make bisection for this further back is need to build old versions of the compiler. If someone has this set up, it would be great to get help by bisection result with the commit where ucontext_t swapcontext was broken.

Don't want to lose Posix swapcontext opportunity because it can suddenly help if problems are found in our assembler code or on implementing new architectures/platforms

I think you're forgetting to update initStack too. There's no issues with using ucontext_t on any targets gdc supports.

[denizzzka]

Thank you, confirmed! Indeed, AsmX86_64_Posix version had to be disabled in two places (second one at line 1016)

I can add tests for ucontext_t support, but I think it's better to do this in a separate PR because testing method itself can cause discussion

So looks like this PR can be merged

[denizzzka]

I can add tests for ucontext_t support, but I think it's better to do this in a separate PR because testing method itself can cause discussion

...My proposal about testing: c13b055

By specifying -version=ucontext_Posix it will be possible to force turn on ucontext_t

[denizzzka]

I can add tests for ucontext_t support, but I think it's better to do this in a separate PR because testing method itself can cause discussion

I decided not to move ucontext_t stuff into core.thread.fiber since this will lead to additional pointer dereferences when calling virtual functions, which we would like to avoid in such a high-speed thing as Fibers. Just considering it a special case and leave it as is - related code amount is not enough to create a mess.

I added -version=ucontext_Posix to test, but I didn't add it to CI because I don't know where it would be appropriate to add it. It should be some kind of Posix system like x86_64 Linux, but compiling whole build just to test this very rarely used feature seems excessive.

[ibuclaw]

I can add tests for ucontext_t support, but I think it's better to do this in a separate PR because testing method itself can cause discussion

I decided not to move ucontext_t stuff into core.thread.fiber since this will lead to additional pointer dereferences when calling virtual functions, which we would like to avoid in such a high-speed thing as Fibers. Just considering it a special case and leave it as is - related code amount is not enough to create a mess.

I added -version=ucontext_Posix to test, but I didn't add it to CI because I don't know where it would be appropriate to add it. It should be some kind of Posix system like x86_64 Linux, but compiling whole build just to test this very rarely used feature seems excessive.

It's not a special case, x86_64 CET uses this path for example (at least until I add support for shadow stacks in the asm Fibers), as well as on many ports of all major distributions. So it's been battle tested for years.

[denizzzka]

I expressed myself not properly: I mean the use ucontext_t as an emergency one in case our native method is not implemented yet - as for RISC-V now. So, let this be a special case, which we do not want to use but if need we can.

[ibuclaw]

If things are being functionally changed, then documentation of the code should be reflected also please.

For instance, I see that whoever changed this to a selective import, didn't realise importing the entire module was deliberate, as there is a chance that the type doesn't exist on the target according to the comment.

[ibuclaw]

Maybe restore the original intention with

Suggested change

	// NOTE: The ucontext implementation requires architecture specific
	// data definitions to operate so testing for it must be done
	// by checking for the existence of ucontext_t rather than by
	// a version identifier. Please note that this is considered
	// an obsolescent feature according to the POSIX spec, so a
	// custom solution is still preferred.
	import core.sys.posix.ucontext : getcontext, makecontext, MINSIGSTKSZ, swapcontext, ucontext_t;
	// NOTE: The ucontext implementation requires architecture specific
	// data definitions to operate so testing for it must be done
	// by checking for the existence of ucontext_t rather than by
	// a version identifier. Please note that this is considered
	// an obsolescent feature according to the POSIX spec, so a
	// custom solution is still preferred.
	static if (__traits(compiles, { import core.sys.posix.ucontext : ucontext_t; })
	import core.sys.posix.ucontext : getcontext, makecontext, MINSIGSTKSZ, swapcontext, ucontext_t;
	else
	static assert(false, "No native support for Fibers detected and ucontext_t not available for use as a fallback");

[denizzzka]

It doesn't work even if I write:

        static if (true)
            import core.sys.posix.ucontext : getcontext, makecontext, MINSIGSTKSZ, swapcontext, ucontext_t;
        else
            static assert(false, "No native support for Fibers detected and ucontext_t not available for use as a fallback");

base.d compilation fails if such changes added

Looks similar as #20487

[denizzzka]

Compilation error will happen in such case and source of issue will become clear

(Please close conversation if you have no ideas what to do with this)