sharpdx / SharpDX

SharpDX GitHub Repository
http://sharpdx.org
MIT License
1.7k stars 639 forks source link

Direct3D12 samples/pinvoke are broken #1058

Closed xoofx closed 6 years ago

xoofx commented 6 years ago

All Direct3D12 samples are currently generating AccessViolationException. Can't tell exactly what's going on, but it was working previously, so I suspect that it is codegen related.

jkoritzinsky commented 6 years ago

Figured it out. It's because the "special-size"-ness of PointerSize (the case that even though its size is greater than CsMethod.MaxSizeReturnParameter it needs to be treated as smaller) is not transitively applied.

There's an easy way to fix this and a hard way to fix it.

Easy way: Does CsMethod.MaxSizeReturnParameter have to equal 4 for accurate codegen? Or can it equal 8 like CsFunction.MaxSizeReturnParameter. I don't have any reference for why these values were set since that's from before I took over SharpGen. If we can set it to 8 then everything gets much easier.

Hard way: Have a special "pointer" size value in the mapping process that is ignored when calculating max size and is transitively applied to structures that are only made up of a single pointer parameter.

xoofx commented 6 years ago

Figured it out. It's because the "special-size"-ness of PointerSize (the case that even though its size is greater than CsMethod.MaxSizeReturnParameter it needs to be treated as smaller) is not transitively applied.

Yeah, it needs to be. The rule to manage it is if it is a pointer (IntPtr, void* directly or indirectly), it should be treated as a regular return, otherwise treat it as large. It is not 100% ABI correct, but, afair, that was the only special case. Otherwise, it would not be possible to have the interop working actually without targeting specific CPU/ABI (which is something I would like to avoid)

So just make it compatible with the behavior we had previously (a struct with a pointer had a size of 4 I believe)

jkoritzinsky commented 6 years ago

I tried treating it as a regular return and I'm getting the same access violation. I'm really not sure what's going on with it now.

jkoritzinsky commented 6 years ago

It's failing on both 32 and 64 bit builds of the samples.

Here's the generated managed wrapper for the offending function after a fix to make it return correctly:

        internal unsafe SharpDX.Direct3D12.CpuDescriptorHandle GetCPUDescriptorHandleForHeapStart()
        {
            SharpDX.Direct3D12.CpuDescriptorHandle __result__;
            SharpDX.Direct3D12.CpuDescriptorHandle.__Native __result__native = default (SharpDX.Direct3D12.CpuDescriptorHandle.__Native);
            __result__ = default (SharpDX.Direct3D12.CpuDescriptorHandle);
            __result__native = SharpDX.Direct3D12.LocalInterop.CalliStdCallSharpDXDirect3D12CpuDescriptorHandle__Native(this._nativePointer, (*(void ***)this._nativePointer)[9]);
            __result__.__MarshalFrom(ref __result__native);
            return __result__;
        }

Do you see anything off with the generated code?

xoofx commented 6 years ago

Wow... is it related to this issue maybe https://stackoverflow.com/a/34322863/1356325 ?

So that in that case, we would actually need to pass a pointer to the returned struct instead? So it is only working when the returned type is actually directly a pointer, but if it is a struct (even with a single pointer field), it should be passed by arg pointer then...

amerkoleci commented 6 years ago

Would you supply PR otherwise I can send PR?

xoofx commented 6 years ago

@jkoritzinsky could you have a look at this? (this is probably the last issue remaining)

jkoritzinsky commented 6 years ago

I'm trying to figure out a fix for this right now. @amerkoleci If you want to also take a look then go for it! You might see something that I'm missing right now.

amerkoleci commented 6 years ago

@jkoritzinsky just dunno how to remap the signature, maybe manually patch d3d12.h file? @xoofx?

xoofx commented 6 years ago

@amerkoleci there is nothing to remap. This should be fixed in SharpGenTools.

jkoritzinsky commented 6 years ago

There's a fix for this AV in the next SharpGenTools drop. We're getting an E_INVALIDARG return code on CreatePipelineState after the fix though. Can someone with more experience in Direct3D12 take a look at that issue?

amerkoleci commented 6 years ago

@jkoritzinsky CreatePipelineState or CreateGraphicsPipelineState?

jkoritzinsky commented 6 years ago

CreateGraphicsPipelineState

amerkoleci commented 6 years ago

I found the issue:

class GraphicsPipelineStateDescription internal unsafe void MarshalTo(ref Native @ref) { @ref.RootSignature = CppObject.ToCallbackPtr(RootSignature); .... }

Should just be RootSignature.NativePointer ? Or maybe not, seams something related to marshaling code, not sure what, maybe ShaderBytecode changes?

amerkoleci commented 6 years ago

Seams something related to #1056 as well.

jkoritzinsky commented 6 years ago

CppObject.ToCallbackPtr(RootSignature) is correct. It might be related to #1056.

jkoritzinsky commented 6 years ago

@xoofx Now that this is fixed, are there any other fixes needed in SharpGenTools? If not, I'll release 1.1.1 and then upgrade SharpDX, at which point we can release another SharpDX version.

xoofx commented 6 years ago

@jkoritzinsky I don't think so. I'm on holidays without a computer, can you remove previous latest tag and reassign it to latest commit to push a new CI nuget preview?

jkoritzinsky commented 6 years ago

I think we're all good for the next release. The fuget API diff isn't showing anything missing. I'll release 1.1.1 of SharpGenTools this week so SharpDX can get a new release once you get back from holiday.