Failing std.bitmanip unittest on Win64: peek() wrongly consumes the range

[kinke]

import std.stdio;

void test()
{
    import std.algorithm;
    import std.bitmanip;
    ubyte[] buffer = [1, 5, 22, 9, 44, 255, 7];
    auto range = filter!"true"(buffer);
    auto ui32 = range.peek!uint();
    auto ui16 = range.peek!ushort();
    auto ui8 = range.peek!ubyte();
    writeln("ui32 = ", ui32, "; ui16 = ", ui16, "; ui8 = ", ui8);
    //assert(range.peek!uint() == 17110537);
    //assert(range.peek!ushort() == 261);
    //assert(range.peek!ubyte() == 1);
}

int main()
{
    test();
    return 0;
}

yields ui32 = 17110537; ui16 = 11519; ui8 = 7, i.e., the range is consumed when peek()ing (44*256+255=11519). This triggers an assertion in std.bitmanip:2640.

[kinke]

I've digged a little deeper.

import std.stdio;
import std.range;

ubyte[T.sizeof] peek(T, R)(R range)
{
    ubyte[T.sizeof] bytes;
    //Make sure that range is not consumed, even if it's a class.
    range = range.save;

    foreach(ref e; bytes)
    {
        e = range.front;
        range.popFront();
    }

    return bytes;
}

void testPeek(R)(R range)
{
    auto ui32 = peek!uint(range);
    auto ui16 = peek!ushort(range);
    auto ui8 = peek!ubyte(range);
    writeln("ui32 = ", ui32, "; ui16 = ", ui16, "; ui8 = ", ui8);
}

void test()
{
    ubyte[] buffer = [1, 5, 22, 9, 44, 255, 7];
    testPeek(buffer);
    auto range = filter!(a => a != 0)(buffer);
    testPeek(range);
}

int main()
{
    test();
    return 0;
}

yields:

ui32 = [1, 5, 22, 9]; ui16 = [1, 5]; ui8 = [1]
ui32 = [1, 5, 22, 9]; ui16 = [44, 255]; ui8 = [7]

As soon as you rewrite peek() like this:

ubyte[T.sizeof] peek(T, R)(R originalRange)
{
    ubyte[T.sizeof] bytes;
    //Make sure that range is not consumed, even if it's a class.
    auto range = originalRange.save;

    foreach(ref e; bytes)
    {
        e = range.front;
        range.popFront();
    }

    return bytes;
}

it works as expected. Note that std.algorithm.filter() returns a std.algorithm.FilterResult struct, i.e., saving shouldn't even be necessary as the range passed to peek() should be a copy already, but seems to be passed by ref!

So here's the reduced problem:

import std.stdio;
import std.algorithm;

void testRange(R)(R range)
{
    writeln("&range in testRange(): ", cast(void*) &range);
}

void test()
{
    ubyte[] buffer = [1, 5, 22, 9, 44, 255, 7];
    auto range = filter!(a => a != 0)(buffer);
    writeln("&range in test():      ", cast(void*) &range);
    testRange(range);
}

int main()
{
    test();
    return 0;
}

yielding something like

&range in test():      BE7E9AFA40
&range in testRange(): BE7E9AFA40

while the addresses should clearly differ. Looks like a severe ABI bug which may easily be the culprit of a lot of failing tests.

[kinke]

import std.stdio;

struct Small { long field; }
struct Big { long field, field2; }

void testStruct(T)(T copy)
{
    copy.field = 0;
}

int main()
{
    Small small = { 1L };
    Big big = { 1L, 2L };

    testStruct(small);
    testStruct(big);

    writeln("small.field = ", small.field, "; big.field = ", big.field);

    return 0;
}

yields small.field = 1; big.field = 0, i.e., structs > 64 bit are really passed by ref instead of value. Relevant piece of .ll:

; Function Attrs: uwtable
define i32 @_Dmain({ i64, { i64, i8* }* } %unnamed) #0 {
  %small = alloca %hello.Small, align 8
  %.structliteral = alloca %hello.Small
  %big = alloca %hello.Big, align 8
  %.structliteral1 = alloca %hello.Big
  %1 = getelementptr %hello.Small* %.structliteral, i32 0, i32 0
  store i64 1, i64* %1
  %2 = load i64* %1
  %3 = bitcast %hello.Small* %small to i8*
  %4 = bitcast %hello.Small* %.structliteral to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %3, i8* %4, i64 8, i32 1, i1 false)
  %5 = load %hello.Small* %small
  %6 = getelementptr %hello.Big* %.structliteral1, i32 0, i32 0
  store i64 1, i64* %6
  %7 = load i64* %6
  %8 = getelementptr %hello.Big* %.structliteral1, i32 0, i32 1
  store i64 2, i64* %8
  %9 = load i64* %8
  %10 = bitcast %hello.Big* %big to i8*
  %11 = bitcast %hello.Big* %.structliteral1 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %10, i8* %11, i64 16, i32 1, i1 false)
  %12 = load %hello.Big* %big
  %13 = bitcast %hello.Small* %small to i64*
  %14 = load i64* %13
  call void @_D5hello30__T10testStructTS5hello5SmallZ10testStructFNaNbNiNfS5hello5SmallZv(i64 %14)
  call void @_D5hello28__T10testStructTS5hello3BigZ10testStructFNaNbNiNfS5hello3BigZv(%hello.Big* byval %big)
  %15 = getelementptr %hello.Small* %small, i32 0, i32 0
  %16 = getelementptr %hello.Big* %big, i32 0, i32 0
  %17 = load i64* %15
  %18 = load i64* %16
  call void @_D3std5stdio24__T7writelnTAyaTlTAyaTlZ7writelnFAyalAyalZv(i64 %18, { i64, i8* } { i64 14, i8* getelementptr inbounds ([15 x i8]* @.str3, i32 0, i32 0) }, i64 %17, { i64, i8* } { i64 14, i8* getelementptr inbounds ([15 x i8]* @.str2, i32 0, i32 0) })
  ret i32 0
}

; Function Attrs: uwtable
define weak_odr void @_D5hello30__T10testStructTS5hello5SmallZ10testStructFNaNbNiNfS5hello5SmallZv(i64 %copy_arg) #0 {
  %copy = alloca %hello.Small
  %1 = bitcast %hello.Small* %copy to i64*
  store i64 %copy_arg, i64* %1
  %2 = getelementptr %hello.Small* %copy, i32 0, i32 0
  store i64 0, i64* %2
  %3 = load i64* %2
  ret void
}

; Function Attrs: uwtable
define weak_odr void @_D5hello28__T10testStructTS5hello3BigZ10testStructFNaNbNiNfS5hello3BigZv(%hello.Big* byval %copy_arg) #0 {
  %1 = getelementptr %hello.Big* %copy_arg, i32 0, i32 0
  store i64 0, i64* %1
  %2 = load i64* %1
  ret void
}

[kinke]

As far as I understand this LLVM code, the byval attribute used for the argument to testStruct!Big() implies that LLVM should make a hidden copy and then pass a pointer to that dedicated copy to the callee, which it apparently doesn't. I'll try to come up with an LDC workaround, but I think it's actually an LLVM bug on Win64.

[redstar]

Nice test case reduction. byval is not implemented for Win64. Instead, this is handled by Win64_byval_rewrite in file gen/abi-win64.cpp. Maybe there is something wrong...