ericsink / SQLitePCL.raw

A Portable Class Library (PCL) for low-level (raw) access to SQLite
Apache License 2.0
512 stars 106 forks source link

Add support for ARM64 varargs calling convention #425

Closed filipnavara closed 2 years ago

filipnavara commented 2 years ago

Workaround for https://github.com/dotnet/runtime/issues/48796. Tested on macOS with Apple Silicon. I kept it enabled on all ARM64 platforms for now since the ABI should be identical on Linux and Windows (until ARM64EC is supported by .NET at least).

filipnavara commented 2 years ago

The Windows ABI seems to be different. I'll update the PR to apply only to macOS/iOS/tvOS to be on the safe side.

filipnavara commented 2 years ago

@ericsink Took me months to change the last two lines but it should be ready for review ;-)

ericsink commented 2 years ago

Interesting. I want to review this a bit more, but mostly just to get myself up to speed.

filipnavara commented 2 years ago

I want to review this a bit more, but mostly just to get myself up to speed.

The gist is that .NET doesn't have a mechanism to express the varargs calling convention of the native code. The ABI that specifies the calling convention usually handles it in one of two ways: 1) The caller puts the variable arguments to stack (eg. x86, Apple's ARM64) 2) The caller puts the variable arguments into registers like it would for normal call (over-simplified, this works only for "integer" types smaller than register size and pointers; floating point and structs likely still need different rules). The callee then dumps the registers into stack so there is a continuous view of all the arguments on the stack (ie. those that fit into registers and those that didn't and caller places them on stack anyway) which can be manipulated with the va_list C APIs.

Conveniently, the normal C calling convension on x86 pushes all arguments to stack anyway, so there's effectively no difference between 1) and 2). That means that on the most common ABIs (x86, x64) you can just declare the .NET P/Invoke signature as if there was nothing special about varargs and just use a signature with specific parameters in place of the C-style ....

On Apple's ARM64, however, the ABI is defined as case 1). Normal methods put first eight arguments into registers. So using the approach above (just put the extra arguments where ... is in C signature) doesn't work. .NET would put the extra arguments into registers and the native code would expect them on stack and crash or misbehave. The workaround is to detect this case of mismatched ABI and put enough "fake" arguments into the signature to occupy all eight parameter slots that are passed through registers. The remaining arguments are spilled to the stack which is exactly what the native code expects.

ericsink commented 2 years ago

Excellent. One final thing that I'm not completely clear about:

How has this problem gone unfixed for so long without coming to my/our attention?

If I understand it correctly, prior to this PR, things like sqlite3_config don't work on arm64. What does "don't work" mean in terms of a symptom? A crash?

Even if I don't run test suites on that chip, it seems like somebody would have run into this problem. Wouldn't it affect, say, every modern iPhone for the last several years?

filipnavara commented 2 years ago

How has this problem gone unfixed for so long without coming to my/our attention?

It affects only very limited set of APIs, sqlite3_db_config was added only recently. sqlite3_config was affected but I assume people may not call it that much? Actually, you may have a good point. I'll do one more test on iOS to ensure it behaves correctly since I am pretty sure we use sqlite3_config there and I didn't see an error with the released version.

On macOS the arm64 support was only added for .NET 6.

filipnavara commented 2 years ago

Ah, I see. We only use sqlite3_config with SQLITE_CONFIG_SERIALIZED option. Since the first argument is fixed (ie. not part of varargs) it still behaves correctly. So none of the options with "no arguments" would be affected.

ericsink commented 2 years ago

Well, these APIs aren't terribly common. I suppose it's not that surprising that nobody has ever used them on an iPhone.

ericsink commented 2 years ago

Thanks for the PR!