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

Remove unnecessary ssize_t posix-ism #265

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Conversation

bnoordhuis
Copy link
Contributor

ssize_t is not always available and the cast it was used in wasn't necessary in the first place, the value already has the right type.

ssize_t is not always available and the cast it was used in wasn't
necessary in the first place, the value already has the right type.
@bnoordhuis bnoordhuis merged commit a0f5077 into quickjs-ng:master Feb 15, 2024
35 checks passed
@bnoordhuis bnoordhuis deleted the ssize branch February 15, 2024 10:32
@saghul
Copy link
Contributor

saghul commented Feb 16, 2024

We don't run UBsan on all targets.

@chqrlie
Copy link
Collaborator

chqrlie commented Feb 16, 2024

The goal of the cast was to ensures sign extension on 32-bit targets for the case where s->malloc_limit was set to -1 as is performed in JS_NewRuntime2. With this patch, it will show as 4294967295 instead of -1 on 32-bit targets.

I thought the behavior was actually undefined because I was confusing s->malloc_limit which is defined as an int64_t and the malloc_limit field in JSMallocState which has type size_t.

I was surprised the CI target linux-32bit did not cause at least a compilation warning for this UB. What CFLAGS are used for CI builds, but s->malloc_limit is a 64-bit int on all platforms. Both gcc -Wall and clang -Wall would produce the warning for a printf argument type mismatch even without UBsan.

Also ssize_t is defined as #define ssize_t ptrdiff_t on windows (#ifdef _MSC_VER) and used in quickjs-libc.c for the read and write wrappers and the js_get_errno() ancillary function, but I don't see this as a portability issue. Targets that do not support read and write system calls or an emulation of them would need a separate implementation anyway.

@saghul
Copy link
Contributor

saghul commented Feb 16, 2024

Ping @bnoordhuis

@bnoordhuis
Copy link
Contributor Author

I think the reasonable change is to make malloc_limit, malloc_size, etc. be the same type in both JSMallocState and JSMemoryUsage? Different types for no good reason is just confusing. size_t or int64_t?

@chqrlie
Copy link
Collaborator

chqrlie commented Feb 18, 2024

Good question. Let me check with Fabrice if there was any good reason to make these int64_t in JSMemoryUsage.

chqrlie pushed a commit to chqrlie/quickjs-ng that referenced this pull request May 8, 2024
Author:    Kasper Isager Dalsgarð <kasperisager@hey.com>
Date:      Wed May 8 23:19:15 2024 +0200
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.

3 participants