Skip to content

system.h: remove some macros #17498

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

Closed
wants to merge 1 commit into from
Closed

system.h: remove some macros #17498

wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented May 31, 2025

Since curl_off_t is always 64 bit these days, we can simplify and avoid using some macros.

@bagder
Copy link
Member Author

bagder commented May 31, 2025

Hm ,that windows CE build failure looks weird...

@bagder
Copy link
Member Author

bagder commented May 31, 2025

clearly that (cmake) Windows CE build gets curl_off_t of the wrong size but I can't figure out why, and the corresponding automake build does not cause those errors?

@jay
Copy link
Member

jay commented May 31, 2025

Replacing the integer suffix macro CURL_OFF_T_C(123) with long-long suffix 123LL is probably not going work everywhere. LL is only guaranteed in C99 and besides that we don't know necessarily that it's going to be 64-bit. For example some platforms the L suffix is appropriate because longs are 64-bit, maybe LL is 128-bit (edit: seems unlikely from what I just read), or anyway you would be converting the curl_off_t long type to a long long so it's not the right suffix

@bagder
Copy link
Member Author

bagder commented Jun 1, 2025

Thanks, I'll avoid the LL suffix.

@bagder
Copy link
Member Author

bagder commented Jun 2, 2025

If I understand things correctly, in this Windows CE build that fails, it runs cmake and defines HAVE_CONFIG_H which makes the source not include config-win32.h and therefore UNDER_CE is never defined. It seems to make the build get the wrong data type for curl_off_t somehow, so the build rightfully complains and errors out.

But we have a similar build using automake that should result in the same thing, but it succeeds?

Puzzling. And Windows CE support is already marked for removal so...

@bagder
Copy link
Member Author

bagder commented Jun 2, 2025

Oh wait, UNDER_CE is never defined by us, it is a define done internally by the Visual Studio compiler.

@bagder bagder marked this pull request as ready for review June 2, 2025 08:13
@vszakats
Copy link
Member

vszakats commented Jun 2, 2025

It seems gcc 4.4.0 of mingw32ce is sensitive by default to unsuffixed long long integer literals. It's causing the CMake Windows CE failures. autotools detects GNU99 mode and enables it automatically, but not CMake.

This should fix it:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -88,6 +88,10 @@ elseif(WIN32 AND WINCE AND CMAKE_C_COMPILER_ID STREQUAL "GNU")  # mingw32ce buil
   set(CMAKE_RC_COMPILE_OBJECT "<CMAKE_RC_COMPILER> -O coff <DEFINES> <INCLUDES> <FLAGS> <SOURCE> <OBJECT>")
   enable_language(RC)
 
+  # To compile long long integer literals
+  set_property(DIRECTORY APPEND PROPERTY COMPILE_OPTIONS "-std=gnu99")
+  string(APPEND CMAKE_REQUIRED_FLAGS " -std=gnu99")
+
   set(CMAKE_C_COMPILE_OPTIONS_PIC "")  # CMake sets it to '-fPIC', confusing the toolchain and breaking builds. Zap it.
 
   set(CMAKE_STATIC_LIBRARY_PREFIX "lib")

→ PR #17523

vszakats added a commit to vszakats/curl that referenced this pull request Jun 2, 2025
To sync with autotools, which auto-detects this option and enables
it by default.

It also makes it possible to compile unsuffixes long long integer
literals correctly, allowing to drop some legacy macros without
bumping into build errors like this:
```
lib/vtls/schannel.c: In function 'schannel_send':
lib/vtls/schannel.c:1815: error: integer constant is too large for 'long' type
```
Ref: https://github.com/curl/curl/actions/runs/15374705821/job/43286736583?pr=17498#step:9:20

Bug: curl#17498 (comment)
Reported-by: Daniel Stenberg
@jay
Copy link
Member

jay commented Jun 3, 2025

Not having any suffix may also be a problem with some pre-c99 compilers because of the way c89 is written 3.1.3.2 Integer constants

   The type of an integer constant is the first of the corresponding
list in which its value can be represented.  **Unsuffixed decimal: int,
long int, unsigned long int;** unsuffixed octal or hexadecimal: int,
unsigned int, long int, unsigned long int; suffixed by the letter u
or U: unsigned int, unsigned long int; suffixed by the letter l or
L: long int, unsigned long int; suffixed by both the letters u or U
and l or L: unsigned long int .

So some compilers may try to fit the large numbers in a 32-bit long even if some 64-bit long long equivalent is available. I have no objection to #17523 but my point is it may not be the only compiler that does that

@bagder
Copy link
Member Author

bagder commented Jun 3, 2025

So some compilers may try to fit the large numbers in a 32-bit long even if some 64-bit long long equivalent is available

Sure, but we might also never run into such a compiler. I suspect our now mandatory support for a working 64 bit type removes some of the quirkiest old compilers from the equation.

vszakats added a commit that referenced this pull request Jun 3, 2025
To sync with autotools, which auto-detects this option and enables it by
default.

It also makes it possible to compile unsuffixed long long integer
literals correctly, allowing to drop some legacy macros without bumping
into build errors like:
```
lib/vtls/schannel.c: In function 'schannel_send':
lib/vtls/schannel.c:1815: error: integer constant is too large for 'long' type
```
Ref: https://github.com/curl/curl/actions/runs/15374705821/job/43286736583?pr=17498#step:9:20

Bug: #17498 (comment)
Reported-by: Daniel Stenberg

Closes #17523
Since curl_off_t is always 64 bit these days, we can simplify and avoid
using some macros.
@bagder
Copy link
Member Author

bagder commented Jun 4, 2025

If we find any problems with this down the line, we should fix those with a set of private macros/defines, not public ones.

@bagder bagder closed this in 614313f Jun 5, 2025
@bagder bagder deleted the bagder/system branch June 5, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants