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.

Sign up for GitHub

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

Jump to bottom

Avoid warnings when compiling under Wpedantic #437

Merged
merged 8 commits into from
Jul 30, 2024
Merged

Avoid warnings when compiling under Wpedantic #437

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5a6dea3
Use ISO C syntax for flexible array members
andrjohns Jun 23, 2024
6d628d6
Mark __int128 as extension
andrjohns Jun 23, 2024
acc7758
Forward reference to enum type
andrjohns Jun 23, 2024
4b5e033
Avoid void pointer to function pointer casts
andrjohns Jun 23, 2024
a28c53c
Mark indirect goto and address-as-label as extensions
andrjohns Jun 23, 2024
5f6ec19
Remove unnecessary forward reference for OpCodeEnum
andrjohns Jun 23, 2024
43b4edf
Guard int128_t typedef
andrjohns Jun 24, 2024
da0746b
Add recommended workaround for dlsym void* return cast
andrjohns Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Mark __int128 as extension
  • Loading branch information
@andrjohns
andrjohns committed Jun 23, 2024
commit 6d628d60e3c8f8e3396cdc3def76756e0dba0fdf
9 changes: 5 additions & 4 deletions libbf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5365,19 +5365,20 @@ int bf_acos(bf_t *r, const bf_t *a, limb_t prec, bf_flags_t flags)
#if LIMB_BITS == 64

/* Note: we assume __int128 is available */
/* uint128_t defined in libbf.h */
#define muldq(r1, r0, a, b) \
do { \
unsigned __int128 __t; \
__t = (unsigned __int128)(a) * (unsigned __int128)(b); \
uint128_t __t; \
__t = (uint128_t)(a) * (uint128_t)(b); \
r0 = __t; \
r1 = __t >> 64; \
} while (0)

#define divdq(q, r, a1, a0, b) \
do { \
unsigned __int128 __t; \
uint128_t __t; \
limb_t __b = (b); \
__t = ((unsigned __int128)(a1) << 64) | (a0); \
__t = ((uint128_t)(a1) << 64) | (a0); \
Comment on lines +5368 to +5381
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was definitely an oversight. Should have used uint128_t from day one.

q = __t / __b; \
r = __t % __b; \
} while (0)
Expand Down
4 changes: 2 additions & 2 deletions libbf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@
#define LIMB_BITS (1 << LIMB_LOG2_BITS)

#if LIMB_BITS == 64
typedef __int128 int128_t;
typedef unsigned __int128 uint128_t;
__extension__ typedef __int128 int128_t;
__extension__ typedef unsigned __int128 uint128_t;
typedef int64_t slimb_t;
typedef uint64_t limb_t;
typedef uint128_t dlimb_t;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change OK as long as __extension__ is properly defined elsewhere if required.

We should probably not define int128_t and uint128_t if they do not exist and use bf_ prefixed names for all public types. Big Floats are expected to be replaced with a simpler and faster BigNum package anyway so fixing this is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change OK as long as extension is properly defined elsewhere if required.

Great! Just to clarify though, __extension__ is a standard compiler keyword, is there a place in the doc or similar where I should define it?

We should probably not define int128_t and uint128_t if they do not exist and use bf_ prefixed names for all public types.

According to GCC, __int128 should exist if long long is at least 128 bits wide. So these definitions and their usage in libbf.c could be guarded by a sizeof(long long) >= 16 conditional or similar. Is that something I should do here or save for a separate PR?

Copy link
Collaborator

@chqrlie chqrlie Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not define int128_t and uint128_t if they do not exist and use bf_ prefixed names for all public types.

According to GCC, __int128 should exist if long long is at least 128 bits wide. So these definitions and their usage in libbf.c could be guarded by a sizeof(long long) >= 16 conditional or similar. Is that something I should do here or save for a separate PR?

int128_t is potentially defined in <stdint.h>, I think you should use #ifndef INT128_MAX as a guard to avoid redefining int128_t if it exists already. sizeof cannot be used in preprocessor expressions.

Copy link
Collaborator

@chqrlie chqrlie Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change OK as long as extension is properly defined elsewhere if required.

Great! Just to clarify though, __extension__ is a standard compiler keyword, is there a place in the doc or similar where I should define it?

__extension__ is not a standard keyword, it is a reserved name. This name is used by gcc, clang and other compilers, but not msvc. I was unsure when it was introduced in gcc and clang. Testing with godbolt.org, I checked that it is supported since at least gcc 3.4.3 and clang 3.0, so no problem with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extension is not a standard keyword, it is a reserved name. This name is used by gcc, clang and other compilers, but not msvc.

Good to know, I had no idea!

int128_t is potentially defined in <stdint.h>, I think you should use #ifndef INT128_MAX as a guard to avoid redefining int128_t if it exists already

Do any implementations provide int128_t? I can only find references to __int128 and __int128_t

Testing with the latest clang & gcc, the only int128-related define is SIZEOF_INT128:

% echo "#include <stdint.h>" | clang-18 -dM -E - | grep -i int128
#define __SIZEOF_INT128__ 16

% echo "#include <stdint.h>" | gcc-14 -dM -E - | grep -i int128
#define __SIZEOF_INT128__ 16

Apparently this is defined from gcc >= 4.6 and clang >= 3.3.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting the C23 standard:

7.22.1.1 Exact-width integer types

1 The typedef name intN_t designates a signed integer type with width N and no padding bits. Thus, int8_t denotes such a signed integer type with a width of exactly 8 bits.
2 The typedef name uintN_t designates an unsigned integer type with width N and no padding bits. Thus, uint24_t denotes such an unsigned integer type with a width of exactly 24 bits.
3 If an implementation provides standard or extended integer types with a particular width and no padding bits, it shall define the corresponding typedef names.

J.6.1 Rule based identifiers
1 The following 53 regular expressions characterize identifiers that are systematically reserved by some clause in this document.
[...] int[a-zA-Z0-9_]*_t, [...] uint[a-zA-Z0-9_]*_t

This makes int128_t and uint128_t reserved names.

For some reason, neither gcc not clang define int128_t or uint128_t eventhough they do support 128-bit integer arithmetics, but don't accept 128-bit integer literals and the standard library does not support 128-bit printf or scanf conversions, albeit there is a way to specify these in the format string: %w128d, %w128u, etc.

Using different identifiers would avoid potential potability issues and testing for INT128_MAX would be a portable way to support 128-bit arithmetics on targets that have standard support for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes int128_t and uint128_t reserved names.

Ah apologies, I misunderstood.

For the current changes, should I also add the change to the bf_ identifiers like below:

#ifndef INT128_MAX
__extension__ typedef __int128 int128_t;
__extension__ typedef unsigned __int128 uint128_t;
#endif
typedef int128_t bf_int128_t;
typedef uint128_t bf_uint128_t;

Or was there a different naming scheme for the bf_ prefix that you'd prefer? I can also handle that in a separate PR if that's easier

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, only add the #ifndef guard. Do not bother defining the bf_xxx types as this would create too many changes in both libbf.c and quickjs.c. This module is going to change to keep minimal changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, current changes should be good to go then

Expand Down