Make core.internal.convert usable in -betterC

[lesderid]

Currently core.internal.convert.toUbyte (a dependency of core.internal.hash) doesn't compile with -betterC because the CTFE version uses new. This fix disables the CTFE branch when the D_BetterC version is defined.

The diff is kind of ugly, but I'm not sure there's a better way.

Related: https://issues.dlang.org/show_bug.cgi?id=19268

[dlang-bot]

Thanks for your pull request and interest in making D better, @lesderid! 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#2710"

[JinShil]

At a minimum, this needs a test. See other tests in https://github.com/dlang/druntime/tree/master/test/betterc

[JinShil]

I'm not sure that this is the right solution. There are others who have made such changes and they have been merged, but I fear that if we stay on that trend, druntime is going to become a difficult to comprehend mess.

I think there may be a way, in the DMD frontend, to not emit an error if compiling with -betterC and evaluating with CTFE. I also think such a change would implicitly solve a number of other betterC CTFE bugs. I'll investigate some if a test case can be provided to demonstrate the error being encountered in betterC CTFE.

[JinShil]

The diff is kind of ugly, but I'm not sure there's a better way.

The diff is quite clean if viewing without whitespace changes.

[lesderid]

I'll investigate some if a test case can be provided to demonstrate the error being encountered in betterC CTFE.

Using toUbyte() from -betterC at all fails to compile, e.g.

void f()
{
    import core.internal.convert : toUbyte;

    int[0] a;
    toUbyte(a);   
}

fails with
/dlang/dmd/linux/bin64/../../src/druntime/import/core/internal/convert.d(28): Error: TypeInfo cannot be used with -betterC (https://run.dlang.io/is/oJ3jWN).

[lesderid]

I feel like if we fix this in DMD, if (__ctfe) will become a little disingenuous because it's not (effectively) a runtime check anymore at all.

[JinShil]

Using toUbyte() from -betterC at all fails to compile

Anything in core.internal is not intended to be called directly by users. What's the user's test case? Also, this fix is supposed to only affect CTFE evaluations, so the test case should not be a runtime check.

I feel like if we fix this in DMD, if (__ctfe) will become a little disingenuous because it's not (effectively) a runtime check anymore at all.

betterC imports druntime, but doesn't link to it. That means all of druntime is available at compile-time but not runtime. Therefore, I'm beginning to think that DMD should not emit any betterC errors if evaluating CTFE. That can probably be accomplished by simply checking Scope.Flags & SCOPE.ctfe in DMD and skipping over the error.

See https://github.com/dlang/dmd/blob/7c827fdf6d503d3e75197223896f917315689396/src/dmd/typinf.d#L42-L49

[JinShil]

OK, as I look into this more, I'm beginning to understand. The test case you presented does indeed trigger the if (__ctfe) branch. However, why is it that betterC code can get away with return (cast(const(ubyte)*)&val)[0 .. T.sizeof];, but non-betterC code has to be subject to such an elaborate algorithm in the if (__ctfe) branch?

[lesderid]

However, why is it that betterC code can get away with return (cast(const(ubyte)*)&val)[0 .. T.sizeof];, but non-betterC code has to be subject to such an elaborate algorithm in the if (__ctfe) branch?

I assume because the cast isn't allowed in CTFE. The version(D_BetterC) code is the same as the non-__ctfe branch. But yes, that means that this fix won't work for -betterC + CTFE.

[JinShil]

I assume because the cast isn't allowed in CTFE.

I think the cast is allowed in CTFE:

void f()
{
    enum a = [1];
    
    import core.internal.convert : toUbyte;
    enum x = toUbyte(a);   
}

void main() {}

https://run.dlang.io/is/UsBG6l

that means that this fix won't work for -betterC + CTFE.

I thought that was the whole point of this fix: To enable the cast in betterC+CTFE. But from what you've said and demonstrated, why is the compiler even evaluating the if (__ctfe) branch if we're only concerned about runtime?

Ah, as I write this, I think I understand. It's because we don't have the equivalent of version(CTFE). __ctfe is a runtime variable, which means the compiler is still going to try to compile code in the if(__ctfe) branch under -betterC, even though it will only ever utilize the else branch. OK, I need to think about this some more, but this may be the right fix, given the limitation that we don't currently have version(CTFE).

[JinShil]

After thinking about this some more, the primary limitation is the call to return new ubyte[] which the compiler lowers to one of the _d_newarray runtime hooks (e.g.

druntime/src/rt/lifetime.d

Line 911 in d835e18

extern (C) void[] _d_newarrayU(const TypeInfo ti, size_t length) pure nothrow

).

There is currently a GSOC project underway to convert some of these array runtime hooks to templates that no longer depend on TypeInfo. That would be the appropriate fix and would allow this feature to work in betterC both at runtime and compile-time.

See also:
#2673
#2656
#2648
#2632

cc @Vild

[lesderid]

I think the cast is allowed in CTFE:

I meant that the current code simply uses a reinterpret cast[1] for the non-__ctfe branch and uses the elaborate algorithm for the __ctfe branch because such a cast isn't allowed during CTFE[2].

(Possibly reiterating what you said:)
Because the algorithm that's used for the __ctfe branch uses dynamic allocation as you pointed out, toUbyte (and as a result hashOf) is currently unusable in betterC, both during CTFE and during normal runtime, indeed because __ctfe is a runtime variable. This fix just makes the -betterC version only have the code of the non-__ctfe branch, so we can at least use toUbyte from -betterC at runtime.

There is currently a GSOC project underway to convert some of these array runtime hooks to templates that no longer depend on TypeInfo.

Once the _d_newarray hooks have been replaced by templates this should indeed not be an issue anymore. Is there an issue filed for each runtime hook? If not, I could file one for this one. Then we could add a comment here to remove the version(D_BetterC)-specific code once the issue is fixed but still have a (hopefully temporary) fix for it now (this is blocking -betterC support for rcarray and rcmap).

[1]

druntime/src/core/internal/convert.d

Line 640 in 29eb98c

return (cast(const(ubyte)*)(arr.ptr))[0 .. T.sizeof*arr.length];

[2] https://run.dlang.io/is/05aGYz

[JinShil]

Then we could add a comment here to remove the version(D_BetterC)-specific code once the issue is fixed but still have a (hopefully temporary) fix for it now (this is blocking -betterC support for rcarray and rcmap)

But all that does is pass the buck on to someone else. Is converting the hooks to a template not within your ability?

[lesderid]

But all that does is pass the buck on to someone else. Is converting the hooks to a template not within your ability?

Sure, if @Vild hasn't already started on these, I don't mind taking a look at it.

[n8sh]

You could make this PR much smaller. You can just change the ctfe_alloc function to this:

/// A @nogc function can allocate memory during CTFE.
@nogc nothrow pure @trusted
private ubyte[] ctfe_alloc()(size_t n)
{
    version (D_BetterC)
        assert(0, "Not available during CTFE when compiling with -betterC.");
    else if (!__ctfe)
        assert(0, "CTFE only");
    else
    {
        static ubyte[] alloc(size_t x) nothrow pure
        {
            if (__ctfe) // Needed to prevent _d_newarray from appearing in compiled prorgam.
                return new ubyte[x];
            else
                assert(0);
        }
        return (cast(ubyte[] function(size_t) @nogc nothrow pure) &alloc)(n);
    }
}

[lesderid]

You could make this PR much smaller.

True, but as @JinShil pointed out this is the wrong fix anyway, so closing.