graalvm / sulong

Obsolete repository. Moved to oracle/graal.
Other
628 stars 63 forks source link

UnsupportedSpecializationException #618

Closed ghost closed 7 years ago

ghost commented 7 years ago

This may be a bug but I don't read LLVM well enough to be sure. This executes correctly when length < 64.

void md5_update( md5_context *ctx, uint8 *input, uint32 length )
{
    uint32 left, fill;

    if( ! length ) return;

    left = ctx->total[0] & 0x3F;
    fill = 64 - left;

    ctx->total[0] += length;
    ctx->total[0] &= 0xFFFFFFFF;

    if( ctx->total[0] < length )
        ctx->total[1]++;

    if( left && length >= fill )
    {
        memcpy( (void *) (ctx->buffer + left),
                (void *) input, fill );
        md5_process( ctx, ctx->buffer );
        length -= fill;
        input  += fill;
        left = 0;
    }

    while( length >= 64 )
    {
        md5_process( ctx, input );
        length -= 64;
        input  += 64;
    }

    if( length )
    {
        memcpy( (void *) (ctx->buffer + left),
                (void *) input, length );
    }
}

In the < 64 caseI get an UNBOX message for the < 64 case and convert it to natively allocated memory. This is on this instruction:

  %59 = call i8* @__memcpy_chk(i8* %54, i8* %.1, i64 %.12, i64 %58) nounwind

md5_process types its second argument as uint8 data[64] and then proceeds to extract bytes, i.e.

define void @md5_process(%struct.md5_context* %ctx, i8* %data) nounwind uwtable ssp {
  %X = alloca [16 x i64], align 16
  %1 = getelementptr inbounds i8* %data, i64 0
  %2 = load i8* %1, align 1
  %3 = zext i8 %2 to i64

When I trace execution I get:

[sulong] assignment of %X in first basic block in function @md5_process in md5
[sulong] assignment of %1 in first basic block in function @md5_process in md5
[sulong] assignment of %2 in first basic block in function @md5_process in md5
should not reach here (see fastr_errors.log)

The reported message is:

Caused by: java.lang.RuntimeException: LLVM error in assignment of %2 in first basic block in function @md5_process in md5 - com.oracle.truffle.r.engine.interop.NativeCharArray cannot be cast to com.oracle.truffle.llvm.types.LLVMAddress

Prior to that there is an UnsupportedSpecializationException in LLVMAnyToI64Node with an argument value of LLVMTruffleObject. Now the class has a specialization for a plain TruffleObject but not one for an LLVMTruffleObject. This occurs in md5_update on the same instruction as above:

  %59 = call i8* @__memcpy_chk(i8* %54, i8* %.1, i64 %.12, i64 %58) nounwind

Unlike in the < 64 case I never see an UNBOX message presumably due to the argument at %59 being LLVMTruffleObject and not TruffleObject. The question is why the different behavior?

Unfortunately it turns out that the behavior is different when the < 64 case is run first and then then > 64 case is run second in the same run, which confused me for a while. In that case a ClassCastException occurs in LLVMAddressReadNode.executePointee also on the "%2 = load i8* %1, align 1" instruction.

When I just run the > 64 case I see an LLVMObject being created wrapping my NativeCharArray from LLVMI8ProfilingNode.executeI8 on the "%2 = load i8* %1, align 1" instruction and then the UnsupportedSpecializationException noted above much later

mrigger commented 7 years ago

@grimmerm @chrisseaton any ideas?

chrisseaton commented 7 years ago
%59 = call i8* @__memcpy_chk(i8* %54, i8* %.1, i64 %.12, i64 %58) nounwind

You're converting to native just to call memcpy? That's the problem I had just on Monday. I added truffle_managed_memcpy that can copy either way from native or managed memory.

I do

#ifdef memcpy
#undef memcpy
#endif

#define memcpy truffle_managed_memcpy

In my API header to use truffle_managed_memcpy.

due to the argument at %59 being LLVMTruffleObject and not TruffleObject

LLVMTruffleObject is an inner pointer, TruffleObject is a conventional pointer. I think that's the difference. If we don't unbox a LLVMTruffleObject then that's just missing functionality. In other words, I implemented converting a managed object to native for a head pointer, but not yet an inner pointer. If you need inner pointers if should be trivial to implement that - just add the offset.

ghost commented 7 years ago

Given already incredible complexity of the R header files I am loathe to start messing with #define; in fact I need to check it is not already redefined! It was only when I started tracing that I realized that the UNBOX was initially coming from memcpy.

As fas as LLVMTruffleObject goes, I did notice the offset field and it does make sense given that code is doing x[i + k] (once the macros are expanded. But LLVMTruffleObject is hardly a very intuitive name for an "inner pointer". And there is no javadoc pretty much anywhere in the internals of sulong. Of course, if it "just works" that isn't an issue but it doesn't. I tried adding a specialization for LLVMTruffleObject in LLVMToI64Node but that caused the system break pretty much at the start - why I didn't figure out yet. The question is whether that is the correct solution? The UNBOX message doesn't have any way to carry the offset, so is the READ somehow adjusted to include the offset? Oh, and it would be a good idea to use "instance of LLVMAddress" and throw a sulong exceptioi instead of a cast as ClassCastExceptions are a pain for debugging.

chrisseaton commented 7 years ago

I think LLVMTruffleObject is not called InnerPointer because the offset is often 0. Maybe we shouldn't allow that and be strict about not using it for head pointers.

The question is whether that is the correct solution?

I think the correct solution is adding a specialisation for it and fixing whatever your problem was, and then adding the offset from LLVMTruffleObject.

You've got some ideas for improvements there - like more documentation, more asserts, better names and so on. Please go ahead and make them where you see the need.

ghost commented 7 years ago

Fixed with https://github.com/graalvm/sulong/pull/625