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

Use fixed width integers (at least in ABI) #343

Closed
steve-s opened this issue Sep 8, 2022 · 4 comments · Fixed by #394
Closed

Use fixed width integers (at least in ABI) #343

steve-s opened this issue Sep 8, 2022 · 4 comments · Fixed by #394
Assignees
Milestone

Comments

@steve-s
Copy link
Contributor

steve-s commented Sep 8, 2022

What is the thinking w.r.t. types such as int, long, which can vary in size even between different compilers even on the same system? Shouldn't the ABI be defined in terms of unambiguous fixed width types such as int32_t?

The question is then what the API should expose. I am not sure if switching everything to fixed width integers would be good porting experience, maybe it's fine? Without switching to fixed width integers in user code, people would see compiler warnings for conversions if they use (with -Wconversion), but maybe that's a good thing and it would guide them to fix them.

Alternative: the API can still expose generic int etc. and the trampolines would do the conversions, but then what if the ABI result does not fit into the API type (i.e., ABI returns int32_t, API returns int and sizeof(int) == 16 for some compiler). We could fail at compilation time in such situation.

Example of the conversion warnings:

$ cat /tmp/test.c 
#include <stdint.h>
int32_t do_int() { return 42; }
int64_t do_int64() { return 42; }
int main() {
	int res1 = do_int();
	int res2 = do_int64();
	int16_t res3 = do_int64();
	return res1 + res2 + res3;
}

$ gcc -Wconversion /tmp/test.c 
/tmp/test.c: In function ‘main’:
/tmp/test.c:6:13: warning: conversion from ‘int64_t’ {aka ‘long int’} to ‘int’ may change value [-Wconversion]
    6 |  int res2 = do_int64();
      |             ^~~~~~~~
/tmp/test.c:7:17: warning: conversion from ‘int64_t’ {aka ‘long int’} to ‘int16_t’ {aka ‘short int’} may change value [-Wconversion]
    7 |  int16_t res3 = do_int64();
      |                 ^~~~~~~~
@timfel timfel added this to the Version 0.9 milestone Nov 1, 2022
@steve-s
Copy link
Contributor Author

steve-s commented Nov 3, 2022

Some interesting food for thought: https://stackoverflow.com/a/13424208/321302.

tl;dr: below is an interesting example. The question this brings is whether the fixed with integers do really provide such portability guarantees that we can reliably support mixing of outputs of compilers that use different widths for int/long/etc.

the behavior of expression (i-1) > 5 when i is zero will vary depending upon whether a uint32_t is smaller than int

@mattip
Copy link
Contributor

mattip commented Dec 1, 2022

We discussed this in the monthly call. It seems the consensus is to use specific types: int32_t, int64_t, ssize_t, and so on.

@fangerer
Copy link
Contributor

I've merged the PR that uses fixed-width integers in the ABI instead of long and long long (and their unsigned pendants).

I'm reopening this issue because I'm working on a change that uses a fixed-width for HPy_ssize_t but that is more involved. We currently assume that HPy_ssize_t is the same as Py_ssize_t and use it in CPython trampolines and slot functions etc. If they are not the same anymore, we need to properly convert between them.

@fangerer fangerer reopened this Jan 23, 2023
@fangerer
Copy link
Contributor

I'm closing this issue since this is basically done except of the HPy_ssize_t part. I'll create follow-up issue for that.

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

Successfully merging a pull request may close this issue.

4 participants