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

Convert _d_arraycast to template - Take 2 #2268

Closed
wants to merge 1 commit into from
Closed

Convert _d_arraycast to template - Take 2 #2268

wants to merge 1 commit into from

Conversation

JinShil
Copy link
Contributor

@JinShil JinShil commented Aug 12, 2018

alloca doesn't work in -betterC. See https://issues.dlang.org/show_bug.cgi?id=19159. malloc won't work either because it isn't pure. So, I just resorted to a hard-coded static array.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @JinShil!

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#2268"

@JinShil
Copy link
Contributor Author

JinShil commented Aug 12, 2018

You can cut and paste the following code into http://run.dlang.io to test the implementation (be sure to add the -betterC flag):

alias UnsignedStringBuf = char[20];

char[] unsignedToTempString()(ulong value, return scope char[] buf, uint radix = 10) @safe
{
    if (radix < 2)
        // not a valid radix, just return an empty string
        return buf[$ .. $];

    size_t i = buf.length;
    do
    {
        if (value < radix)
        {
            ubyte x = cast(ubyte)value;
            buf[--i] = cast(char)((x < 10) ? x + '0' : x - 10 + 'a');
            break;
        }
        else
        {
            ubyte x = cast(ubyte)(value % radix);
            value = value / radix;
            buf[--i] = cast(char)((x < 10) ? x + '0' : x - 10 + 'a');
        }
    } while (value);
    return buf[i .. $];
}

private struct TempStringNoAlloc
{
    // need to handle 65 bytes for radix of 2 with negative sign.
    private char[65] _buf;
    private ubyte _len;
    auto get() return
    {
        return _buf[$-_len..$];
    }
    alias get this;
}

auto unsignedToTempString()(ulong value, uint radix = 10) @safe
{
    TempStringNoAlloc result = void;
    result._len = unsignedToTempString(value, result._buf, radix).length & 0xff;
    return result;
}

private void onArrayCastError()(string fromType, size_t fromSize, string toType, size_t toSize) @trusted pure
{
    const(char)[][8] msgComponents =
    [
        "Cannot cast `"
        , fromType
        , "` to `"
        , toType
        , "`; an array of size "
        , unsignedToTempString(fromSize)
        , " does not align on an array of size "
        , unsignedToTempString(toSize)
    ];

    // `alloca` would be preferred here, but it doesn't work in -betterC
    // See https://issues.dlang.org/show_bug.cgi?id=19159
    char[1024] msg = void;

    size_t index = 0;
    foreach (m ; msgComponents)
        foreach (c; m)
            msg[index++] = c;
    msg[index] = '\0';

    // first argument must evaluate to `false` at compile-time to maintain memory safety in release builds
    assert(false, msg);
}

TTo[] __ArrayCast(TFrom, TTo)(TFrom[] from) @nogc pure @trusted
{
    const fromSize = from.length * TFrom.sizeof;
    const toLength = fromSize / TTo.sizeof;

    if ((fromSize % TTo.sizeof) != 0)
    {
        onArrayCastError(TFrom.stringof, fromSize, TTo.stringof, toLength * TTo.sizeof);
    }

    struct Array
    {
        size_t length;
        void* ptr;
    }
    auto a = cast(Array*)&from;
    a.length = toLength; // jam new length
    return *cast(TTo[]*)a;
}

extern(C) void main()
{
    byte[int.sizeof * 3] b = cast(byte) 0xab;
    long[] l;

    l = __ArrayCast!(byte, long)(b);
}

@n8sh
Copy link
Member

n8sh commented Aug 13, 2018

malloc won't work either because it isn't pure.

You could do the same thing as in core.memory.fakePureMalloc. If core.memory.pureMalloc is D pure because any change to errno is reverted before the function returns, the same ought to apply here since onArrayCastError never returns.

src/object.d Outdated
auto msg = (cast(char*)alloca(length))[0 .. length];
// `alloca` would be preferred here, but it doesn't work in -betterC
// See https://issues.dlang.org/show_bug.cgi?id=19159
char[1024] msg;
Copy link
Member

Choose a reason for hiding this comment

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

= void if you keep this approach.

@JinShil
Copy link
Contributor Author

JinShil commented Aug 14, 2018

You could do the same thing as in core.memory.fakePureMalloc.

Thanks, I wasn't aware of that technique. I'd rather go with the current approach unless someone objects. Neither approach is ideal, but it's all we have until https://issues.dlang.org/show_bug.cgi?id=18788 or https://issues.dlang.org/show_bug.cgi?id=19159 is fixed.

Edit: I take that back. I noticed pureMalloc was public, so let's give it a try.

Edit: Nope, it doesn't work because the druntime is not linked in for -betterC, and that method requires certain druntime implementations. We just need to get dynamic stack allocation working. Back to using a static array.

@PetarKirov
Copy link
Member

Nope, it doesn't work because the druntime is not linked in for -betterC, and that method requires certain druntime implementations.

How about making pureMalloc a template? It would also be helpful for solving a related problem in phobos: dlang/phobos#6660 (comment).

@JinShil
Copy link
Contributor Author

JinShil commented Aug 14, 2018

How about making pureMalloc a template?

But then I'd have to make all the other function calls inside of pureMalloc templates too. I'm a little worried about going too overboard with the templates.

If you think it should be a template for reasons beyond the scope of this PR, please submit a separate PR for it. If that passes scrutiny, then I'll change the implementation in this PR.

auto msg = (cast(char*)alloca(length))[0 .. length];
// `alloca` would be preferred here, but it doesn't work in -betterC
// See https://issues.dlang.org/show_bug.cgi?id=19159
char[1024] msg = void;
Copy link
Member

@PetarKirov PetarKirov Aug 14, 2018

Choose a reason for hiding this comment

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

This looks like a classic buffer overrun opportunity. With templates, type names in D can easily get pretty long. Instead of trying to find a one size fits all number, summing the string length as before + using dynamic allocation would be more appropriate.

@PetarKirov
Copy link
Member

PetarKirov commented Aug 14, 2018

But then I'd have to make all the other function calls inside of pureMalloc templates too

No, if you compile with -betterC, every call should be extern(C) and defined in libc:

extern (C) private pure @system @nogc nothrow
{
    pragma(mangle, "fakePureErrnoImpl") ref int fakePureErrno();

    pragma(mangle, "malloc") void* fakePureMalloc(size_t);
    pragma(mangle, "calloc") void* fakePureCalloc(size_t nmemb, size_t size);
    pragma(mangle, "realloc") void* fakePureRealloc(void* ptr, size_t size);

    pragma(mangle, "free") void fakePureFree(void* ptr);
}

void* pureMalloc(bool saveRestoreErrno = true)(size_t size) @trusted pure @nogc nothrow
{
    static if (saveRestoreErrno)
        const errnosave = fakePureErrno();

    void* ret = fakePureMalloc(size);

    static if (saveRestoreErrno)
        fakePureErrno() = errnosave;
    else if (!ret)
    {
        version (D_BetterC)
            assert(0, "Memory allocation failed");
        else
        {
            import core.exception : onOutOfMemoryError;
            onOutOfMemoryError();
        }
    }
    
    return ret;
}

@JinShil
Copy link
Contributor Author

JinShil commented Aug 14, 2018

No, if you compile with -betterC, every call should be extern(C) and defined in libc:

Except for...

ref int fakePureErrnoImpl()

But I suppose that can be dealt with.

@PetarKirov
Copy link
Member

You're right, I missed that one, because I only instantiated the template with false

@JinShil
Copy link
Contributor Author

JinShil commented Aug 16, 2018

I'm marking this "Blocked" for now. I really want to see about implementing @WalterBright's suggestion at https://issues.dlang.org/show_bug.cgi?id=18788 to have scope char[] buf allocate on the stack. I don't know if I'll be able to pull it off (help is welcome), but that would be the preferred direction for this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants