Skip to content

curl_multibyte: fixup low-level calls, include in unity builds #16742

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 8 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Mar 17, 2025

Also adjust () around low-level calls preventing macro overrides via
e.g. memdebug.h:

  • add for malloc and free.
  • drop for _open. (We do not override _open in curl.)

Tidy-up: also sync libcurlu custom macro order in cmake with autotools.

Follow-up to f42a279 #11928

@vszakats vszakats added the build label Mar 17, 2025
@vszakats vszakats marked this pull request as draft March 17, 2025 02:36
@vszakats vszakats changed the title build: try moving lib/curl_multibyte.c into unity build: move lib/curl_multibyte.c into unity Mar 17, 2025
@vszakats vszakats marked this pull request as ready for review March 17, 2025 09:39
@github-actions github-actions bot added the CI Continuous Integration label Mar 17, 2025
@vszakats vszakats changed the title build: move lib/curl_multibyte.c into unity curl_multibyte: move into unity builds, fixup low-level call syntax Mar 17, 2025
@vszakats vszakats changed the title curl_multibyte: move into unity builds, fixup low-level call syntax curl_multibyte: fixup low-level calls, integrate with unity builds Mar 17, 2025
@jay
Copy link
Member

jay commented Mar 17, 2025

The comment at the head of this file should be updated to explain why all functions in the file must now call the original versions of memory functions. For example,.

/*
 * The functions in this file are curlx functions which are not tracked by the
 * curl memory tracker memdebug so they do not use the macro replacements
 * free, malloc, etc. Instead wrap the names in parentheses to call the
 * original versions: `ptr = (malloc)(123)`, `(free)(ptr)`, etc.
 */

@vszakats
Copy link
Member Author

That comment could use an update indeed. I also learned about test 1132,
which hopefully may be unnecessary one day.

I guess the root reason we don't want to call the debug allocators from these
functions, is because these might be called by the debug allocators. The
memdebug.h requirement assumes a per-file compilation model, which is
no longer true with unity and bundle builds. I think the root-root cause is
redefining system symbols, and the () trick serves as a workaround (with
caveats).

Updated the comment!

vszakats added a commit to vszakats/curl that referenced this pull request Mar 19, 2025
Also adjust `()` around low-level calls preventing macro overrides via
e.g. `memdebug.h`:
- add for `malloc` and `free`.
- drop for `_open`. (We do not override `_open` in curl.)

Tidy-up: also sync libcurlu custom macro order in cmake with autotools.

Follow-up to f42a279 curl#11928

Closes curl#16742
vszakats added a commit to vszakats/curl that referenced this pull request Mar 19, 2025
Also adjust `()` around low-level calls preventing macro overrides via
e.g. `memdebug.h`:
- add for `malloc` and `free`.
- drop for `_open`. (We do not override `_open` in curl.)

Tidy-up: also sync libcurlu custom macro order in cmake with autotools.

Follow-up to f42a279 curl#11928

Closes curl#16742
@vszakats vszakats changed the title curl_multibyte: fixup low-level calls, integrate with unity builds curl_multibyte: fixup low-level calls, include in unity builds Apr 4, 2025
@vszakats vszakats closed this in 04c78c8 Apr 7, 2025
@vszakats vszakats deleted the mem2 branch April 7, 2025 20:34
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
Also adjust `()` around low-level calls preventing macro overrides via
e.g. `memdebug.h`:
- add for `malloc` and `free`.
- drop for `_open`. (We do not override `_open` in curl.)

Tidy-up: also sync libcurlu custom macro order in cmake with autotools.

Follow-up to f42a279 curl#11928

Closes curl#16742
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