Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable winsqlite3 on 32 bit targets #1151

Merged
merged 1 commit into from
Apr 3, 2022

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Apr 2, 2022

winsqlite3.dll actually has a totally custom ABI on 32-bit targets which is different than SQLite typically uses on those platforms (and on any other -- literally this is the only time that SQLite uses an ABI other than extern "C", as far as I can tell -- it even would have required modification to SQLite itself (and its header) to do so).

This is what it the bindgen output looks for winsqlite3: https://gist.github.com/thomcc/4bbbecd0bd79e5ed181fcc9960c59e77. Note that this applies both to the exported functions, and to callbacks, which makes it very hard for us to support. (On 64 bit targets, the ABI is the same as everywhere else -- extern "C").

In practice, the feature currently doesn't actually work at all on these targets, since we try to pass extern "C" fn as callbacks to functions that expect extern "stdcall", which gives a strange compile error. This makes that explicit, by emitting a compile error on these targets, rather than a mysterious error.

Eventually it could be possible to support this, but I think it might require some new Rust features, such as allowing a macro after extern (as in extern abi!() { ... } and extern abi!() fn, so that we could swap extern "stdcall" and extern "C")... but this would still be pretty unfortunate, as we'd have a feature that would break code when enabled, which is generally bad form. We could emit wrappers (for non-varargs funcs), but that's a big maintenance burden.

It's very tempting to me to say that we should remove this feature, as it has been the cause of several headaches now, and people should almost certainly just use bundled instead... But I guess it's not really that bad to keep just for 64 bit targets.

@thomcc
Copy link
Member Author

thomcc commented Apr 2, 2022

In practice, the feature currently doesn't actually work at all on these targets, but this makes that explicit, by emitting a compile error for them.

Note that if it did work, it would probably crash on the first call into SQLite. (At least, without some mitigation to address ABI mismatch).

@codecov
Copy link

codecov bot commented Apr 2, 2022

Codecov Report

Merging #1151 (fd502b5) into master (3f6570f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1151   +/-   ##
=======================================
  Coverage   78.21%   78.21%           
=======================================
  Files          47       47           
  Lines        5927     5927           
=======================================
  Hits         4636     4636           
  Misses       1291     1291           
Impacted Files Coverage Δ
libsqlite3-sys/src/lib.rs 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f6570f...fd502b5. Read the comment docs.

@thomcc thomcc merged commit f8b9ad8 into rusqlite:master Apr 3, 2022
@thomcc thomcc deleted the disable-winsqlite3-32bit branch April 3, 2022 15:08
@bdbai
Copy link

bdbai commented Apr 30, 2023

Hi @thomcc , just wondering if extern "system" will work out of the box? Actually the first issue that I have encountered on x86 with winsqlite3 is unresolved external symbol due to mismatched name manging rules.

Update: I tried to replace all extern "C"s with extern "system"s (except varadic functions) and it seemed just work. Shall I raise a PR for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants