Missing Calling Convention for x86 platform produces debugging issues

[rima1098]

Describe the bug
Upon setting the Licensetype of QuestPDF, on Projects configured with x86 as TargetPlatform, the following error occurs when debugged with Visual Studio (PInvokeStackInbalance Exception has to be checked in Exception-Settings, otherwise it will be ignored).
"A call to the PInvoke function "QuestPDF!QuestPDF.Skia.SkNativeDependencyCompatibilityChecker+API::check_compatibility_by_calculating_sum" has disturbed the balance of the stack. The managed PInvoke signature probably does not match the unmanaged target signature. Check whether the calling convention and the parameters of the PInvoke signature match the unmanaged target signature."

To Reproduce
Create e.g. a .net472 console application in Visual Studio and initialize QuestPDF by setting the License Type. Set Configuration to Debug|AnyCPU but set the TargetFramework of the console project to x86. (I think it also happens if you choose Debug|x86 as Configuration). Debug the application and make sure that the Expection for PInvokeStackInbalance is enabled in debugging options.

Expected behavior
The library should load without issues and the compatibility check should pass when debugging the application.

Environment
Windows 10 x64, QuestPDF 2024.3.6

Additional context
I tried adding the following to the DLLImport Attribute:

[DllImport(SkiaAPI.LibraryName, CallingConvention = CallingConvention.Cdecl)]
public static extern int check_compatibility_by_calculating_sum(int a, int b);

This fixed the issue and the compatibilitycheck was successful. I guess that would be required for all extern methods. However, i tried adding this with a precompiler flag to only apply on x86. However, since it is built with Debug|AnyCPU, it does not hit the precompiler flag.
Another way could be to use a delegate for all API calls which is set. E.g.

private static class API
{
    private delegate int check_compatibility_by_calculating_sum_delegate(int a, int b);

    private static check_compatibility_by_calculating_sum_delegate _check_compatibility_by_calculating_sum_delegate;

    static API()
    {
        if (IntPtr.Size == 4) // 32-bit process
        {
            _check_compatibility_by_calculating_sum_delegate = check_compatibility_by_calculating_sum_x86;
        }
        else
        {
            _check_compatibility_by_calculating_sum_delegate = check_compatibility_by_calculating_sum_x64;
        }
    }

    [DllImport(SkiaAPI.LibraryName, CallingConvention = CallingConvention.Cdecl, EntryPoint = "check_compatibility_by_calculating_sum")]
    private static extern int check_compatibility_by_calculating_sum_x86(int a, int b);

    [DllImport(SkiaAPI.LibraryName, EntryPoint = "check_compatibility_by_calculating_sum")]
    private static extern int check_compatibility_by_calculating_sum_x64(int a, int b);

    public static int check_compatibility_by_calculating_sum(int a, int b)
    {
        return _check_compatibility_by_calculating_sum_delegate(a, b);
    }
}

[rima1098]

@MarcinZiabek i just investigated a little bit further and it might not even be necessary to have the CallingConvention only specified for x86. I believe that other architectures apart from x86 will simply ignore the calling convention and using x64 default calling convention. I tested it in the native QuestPDF repository by adding the Calling Convention to each native method declaration and the tests results are the same regardless of whether the testproject has a TargetFramework of x86 or x64

see also here: https://stackoverflow.com/questions/34832679/is-the-callingconvention-ignored-in-64-bit-net-applications

[MarcinZiabek]

Thank you for sharing this issue. I highly appreciate your involvement ❤️

It turns out that my nightmare of supporting legacy systems is not over yet. As far as I remember, the calling convention is being used. I remember tuning it in between arm64 vs x64 and core vs framework.

In the 2024.3.6 release, I have written many tests that attempt to run the library on various configurations. So far, all tests are working on dotnet 8. I also need to extend it to use the legacy framework.

Let's collaborate! 😄

[rima1098]

@MarcinZiabek
The weird thing is that tests might not even trigger this Exception. If I directly run the .exe instead of debugging the project everything runs fine also in Visual Studio. In Rider I do not even get this Exception while debugging but I noticed that generation of documents takes really long (talking about 1+ minute for <100 pages) so that tells me that there is something off at least. When disabling the breakpoint on the StackImbalance Exception, debugging in Visual studio is fine but at some point in document generation another exception is thrown as there is something that takes to long (sorry I cannot provide the exact exception, as I have no access to this project until next week).

According to https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.dllimportattribute.callingconvention?view=net-8.0, Cdecl is the default calling convention for all platforms except Windows, and on Windows, stdcall is the default calling convention.

The Documentation regarding Cdecl also specifically points out that it is ignored on ARM and x64 processors. https://learn.microsoft.com/en-us/cpp/cpp/cdecl?view=msvc-170

The main thing why "everything still works in x86" is that cdecl and stdcall use the same parameter passing and the same register for return values, therefore we might never experience any issues or errors on the output. However, stack cleanup is handled differently and produces the Stack Imbalance Exception and I think such things are only checked specifically during debugging but not in release mode. So in my opinion, the Calling Convention attribute is defenitely necessary on x86 for all API calls.

If you have concerns that the specification of calling convention will have effect on other platforms/architectures, there is still the option of using the function delegate as I have suggested in the inital post. I know it's a pain to do this for every API call but I'm happy to help and implement this if you agree on that solution :) (Allthough I would leave the testing on your side since you have a lot more experience in that for sure).

[MarcinZiabek]

Thank you so much for providing me with an in-depth summary. I have also done some investigation on my own. It's pretty sad that keeping backward compatibility is causing this type of issue. Honestly, I didn't foresee that supporting legacy platforms would come with so many challenges.

As far as I understand the topic, the solution is pretty straightforward. We only need to add the explicit calling convention configuration for each API call.

Replace

    [DllImport(SkiaAPI.LibraryName)]
    private static extern int check_compatibility_by_calculating_sum_x64(int a, int b);

With

    [DllImport(SkiaAPI.LibraryName, CallingConvention = CallingConvention.Cdecl)]
    private static extern int check_compatibility_by_calculating_sum_x86(int a, int b);

What are your thoughts?

[rima1098]

I agree with you, according to all the posts I've read, this should be sufficient. It does get rid of the Exception as far as i've tested it. And as Cdecl is standard to all other platforms except Windows, it should at least not affect any other platforms. Regarding the architecture, it should be ignored on x64 and ARM so let's give it a try :)

[rima1098]

@MarcinZiabek once that's working, will you create a new release for that fix or will we have to wait until the next planned release?

[MarcinZiabek]

Considering that this issue could be more severe, I decided to release the fix as soon as possible. Once again, thank you for the successful collaboration!

Could you please test the 2024.3.7 version?

[rima1098]

Thank you too, especially for your fast reaction and response time. I really appreciate that! It seems to be working as expected :)