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

cmake: honor custom CMAKE_UNITY_BUILD_BATCH_SIZE #14626

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Aug 21, 2024

This value tells how many sources files to bundle in a single "unity"
compilation unit.

The CMake default is 8 sources, curl's CMake set this to 0, meaning
to bundle all sources into a single unit.

This patch makes it possible to override the 0 value, and potentially
optimize the build process further by better utilizing multiple cores
in conjunction with make -jN.

The number of sources in lib is 172 at the time of writing this. For
a 12-core CPU, this can give a job for them all:
-DCMAKE_UNITY_BUILD_BATCH_SIZE=15

(Compile time may be affected by a bunch of other factors.)

This value tells how many sources files to bundle in a single "unity"
compilation unit.

The CMake default is 8 sources, curl's CMake set this to 0, meaning
to bundle all sources into a single unit.

This patch makes it possible to override the 0 value, and potentially
optimize the build process further by better utilizing multiple cores
in conjunction with `make -jN`.

The number of sources in lib is 172 at the time of writing this. For
a 12-core CPU, this can give a job for them all:
`-DCMAKE_UNITY_BUILD_BATCH_SIZE=15`

(the end result may be affected by a bunch of other factors.)
@bagder
Copy link
Member

bagder commented Aug 21, 2024

For some reason this feature breaks the build for me in ways it does not for "normal" unity build:

$ cmake -B build -DCMAKE_UNITY_BUILD=ON -DCMAKE_UNITY_BUILD_BATCH_SIZE=15
$ cd build
$ make -sj
...

/home/dast/src/curl/src/tool_getparam.c: In function ‘getparameter’:
/home/dast/src/curl/src/tool_getparam.c:2409:11: error: implicit declaration of function ‘msnprintf’; did you mean ‘vsnprintf’? [-Wimplicit-function-declaration]
 2409 |           msnprintf(buffer, sizeof(buffer), "%" CURL_FORMAT_CURL_OFF_T "-",
      |           ^~~~~~~~~
      |           vsnprintf
/home/dast/src/curl/src/tool_getparam.c:2409:11: warning: nested extern declaration of ‘msnprintf’ [-Wnested-externs]
In file included from /home/dast/src/curl/build/src/CMakeFiles/curl.dir/Unity/unity_1_c.c:28:
/home/dast/src/curl/src/tool_ipfs.c: In function ‘ipfs_gateway’:
/home/dast/src/curl/src/tool_ipfs.c:83:19: error: implicit declaration of function ‘aprintf’; did you mean ‘dprintf’? [-Wimplicit-function-declaration]
   83 |       ipfs_path = aprintf("%s/.ipfs/", home);
      |                   ^~~~~~~
      |                   dprintf
/home/dast/src/curl/src/tool_ipfs.c:83:19: warning: nested extern declaration of ‘aprintf’ [-Wnested-externs]
/home/dast/src/curl/src/tool_ipfs.c:83:17: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
   83 |       ipfs_path = aprintf("%s/.ipfs/", home);
      |                 ^
/home/dast/src/curl/src/tool_ipfs.c:90:30: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
   90 |   gateway_composed_file_path = aprintf("%sgateway", ipfs_path);
      |                              ^
/home/dast/src/curl/src/tool_ipfs.c: In function ‘ipfs_url_rewrite’:
/home/dast/src/curl/src/tool_ipfs.c:240:14: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  240 |   pathbuffer = aprintf("%s%s/%s%s", gwpath, protocol, cid,
      |              ^
In file included from /home/dast/src/curl/build/src/CMakeFiles/curl.dir/Unity/unity_1_c.c:43:
/home/dast/src/curl/src/tool_operate.c: In function ‘single_transfer’:
/home/dast/src/curl/src/tool_operate.c:986:20: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  986 |             header = aprintf("If-None-Match: %s", etag_from_file);
      |                    ^
/home/dast/src/curl/src/tool_operate.c:990:20: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  990 |             header = aprintf("If-None-Match: \"\"");
      |                    ^
/home/dast/src/curl/src/tool_operate.c:1197:23: error: initialization of ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
 1197 |             char *d = aprintf("%s/%s", config->output_dir, per->outfile);
      |                       ^~~~~~~
In file included from /home/dast/src/curl/build/src/CMakeFiles/curl.dir/Unity/unity_1_c.c:46:
/home/dast/src/curl/src/tool_operhlp.c: In function ‘add_file_name_to_url’:
/home/dast/src/curl/src/tool_operhlp.c:144:19: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  144 |           newpath = aprintf("%s%s", path, encfile);
      |                   ^
/home/dast/src/curl/src/tool_operhlp.c:147:19: error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  147 |           newpath = aprintf("%s/%s", path, encfile);
      |                   ^
make[2]: *** [src/CMakeFiles/curl.dir/build.make:115: src/CMakeFiles/curl.dir/Unity/unity_1_c.c.o] Error 1

@vszakats
Copy link
Member Author

Right, this shouldn't of course happen, but with this setting there is any number of new mash-up combinations, and it isn't without side-effects. I could replicate, and having a look.

@bagder
Copy link
Member

bagder commented Aug 21, 2024

I don't think you need to spend a lot of energy on fixing that if it turns out cumbersome. It's such a niche thing.

@vszakats
Copy link
Member Author

vszakats commented Aug 21, 2024

I think it's useful because issues like this may pop up in other places in the future. Even in UNITY_BATCH=0.

In this case it was the common one where a header guarded against multiple inclusions has two operating modes depending on a macro set before including it: curlx.h and ENABLE_CURLX_PRINTF.

With BATCH=15 a source included it first without setting ENABLE_CURLX_PRINTF, so the next source trying to get these redefines did not get them!

Basically the same as the libssh.h issue fixed yesterday.

This should fix it, though the full fix will be a little longer, by removing the per-file #define ENABLE_CURLX_PRINTF lines:

--- a/src/tool_setup.h
+++ b/src/tool_setup.h
@@ -25,6 +25,7 @@
  ***************************************************************************/
 
 #define CURL_NO_OLDIES
+#define ENABLE_CURLX_PRINTF
 
 /*
  * curl_setup.h may define preprocessor macros such as _FILE_OFFSET_BITS and

vszakats added a commit to vszakats/curl that referenced this pull request Aug 21, 2024
Sources used `lib/curlx.h` with both `ENABLE_CURLX_PRINTF` set and unset
before including it.

In a cmake "unity" batch where the first included source had it unset,
the next sources did not get the macros requested with
`ENABLE_CURLX_PRINTF` because `lib/curl.x` has already been included
without them.

Fix it by setting `ENABLE_CURLX_PRINTF` globally for all `src`.

This came up while testing unity builds with smaller batches. The full,
default unity build where all `src` is bundled up in a single unit, was
not affected.

Reported-by: Daniel Stenberg
Bug: curl#14626 (comment)
@vszakats vszakats closed this in a62e3be Aug 21, 2024
@vszakats vszakats deleted the cm-honor-unity-batch-size branch August 21, 2024 21:37
vszakats added a commit to vszakats/curl that referenced this pull request Aug 21, 2024
Sources used `lib/curlx.h` with both `ENABLE_CURLX_PRINTF` set and unset
before including it.

In a cmake "unity" batch where the first included source had it unset,
the next sources did not get the macros requested with
`ENABLE_CURLX_PRINTF` because `lib/curl.x` has already been included
without them.

Fix it by setting `ENABLE_CURLX_PRINTF` globally for all `src`.

This came up while testing unity builds with smaller batches. The full,
default unity build where all `src` is bundled up in a single unit, was
not affected.

Reported-by: Daniel Stenberg
Bug: curl#14626 (comment)
vszakats added a commit that referenced this pull request Aug 22, 2024
Sources used `lib/curlx.h` with both `ENABLE_CURLX_PRINTF` set and unset
before including it.

In a cmake "unity" batch where the first included source had it unset,
the next sources did not get the macros requested with
`ENABLE_CURLX_PRINTF` because `lib/curl.x` had already been included
without them.

Fix it by by making the macros enabled permanently and globally for
internal sources, and dropping `ENABLE_CURLX_PRINTF`.

This came up while testing unity builds with smaller batches. The full,
default unity build where all `src` is bundled up in a single unit, was
not affected.

Fixes:
```
$ cmake -B build -DCMAKE_UNITY_BUILD=ON -DCMAKE_UNITY_BUILD_BATCH_SIZE=15
$ make -C build
...
curl/src/tool_getparam.c: In function ‘getparameter’:
curl/src/tool_getparam.c:2409:11: error: implicit declaration of function ‘msnprintf’; did you mean ‘vsnprintf’? [-Wimplicit-function-declaration]
 2409 |           msnprintf(buffer, sizeof(buffer), "%" CURL_FORMAT_CURL_OFF_T "-",
      |           ^~~~~~~~~
      |           vsnprintf
curl/src/tool_getparam.c:2409:11: warning: nested extern declaration of ‘msnprintf’ [-Wnested-externs]
[...]
```

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

Closes #14632
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.

2 participants