memdebug: avoid macro overrides for low-level allocator functions #16746
Conversation
vszakats
changed the title
| #else | ||
| # define sclose(x) close((x)) | ||
| # define CURL_SCLOSE(x) close((x)) |
There was a problem hiding this comment.
Why this? Are you actually having something else that collides with sclose?
There was a problem hiding this comment.
No, but sclose is easy to mistook for a system function.
But most importantly: There are two things going on here:
- the platform-dependent mapping here above.
- the memdebug mapping in memdebug/curl_memory.
Without a stable internal macro keeping the platform-dependent
mapping around, this is lost when undefining and redefining later.
Such interim macro may be dropped once we avoided temporarily
redefining things for memdebug, but this may be a few steps
down the road. It's hard (for me) to see it at this point.
A curl-specific name is IMO nicer here regardless to match with
other SOCK-things and avoid mistaking it for a system function.
It has only a dozen callers in libcurl and test code. With the rest in
test server code.
There was a problem hiding this comment.
No, but
scloseis easy to mistook for a system function.
I'm certainly biased on this topic since I introduced this name. But it is sort of the point that it is similar to close but not identical. I can't recall when this has ever been a problem?
I suppose I don't understand what problem you're fixing here.
There was a problem hiding this comment.
It took a while for me to figure out this is a curl macro. But I don't much care about
the name (any name). The main issue is that the platform-specific definitions (from
curl_setup_once.h) may be lost depending on #include order.
The safe way is to make one macro with the platform defs, and map that to another
macro that can be undefined, redefined as needed. Once there is no longer any
undef / redef, it's still useful because curl_dbg_sclose() will always want to call
the "raw", platform-specific sclose from memdebug.c.
These names can be sclose and SCLOSE, but they need to be two names.
curl_setup_once.h:# define sclose(x) closesocket((x))
curl_setup_once.h:# define sclose(x) CloseSocket((x))
curl_setup_once.h:# define sclose(x) close_s((x))
curl_setup_once.h:# define sclose(x) lwip_close((x))
curl_setup_once.h:# define sclose(x) close((x))
curl_memory.h:#undef sclose
memdebug.h:/* sclose is probably already defined, redefine it! */
memdebug.h:#undef sclose
memdebug.h:#define sclose(sockfd) curl_dbg_sclose(sockfd,__LINE__,__FILE__)
memdebug.c: int res = sclose(sockfd);
There was a problem hiding this comment.
The (single) macro has existed like this for twenty-five years and aren't things building fine in CI? You say "they need to be two names" - I assume then that there's some breakage somewhere this fixes?
There was a problem hiding this comment.
I'm not saying it's broken, but fragile. Might break when moving around other things.
(it may well have gotten broken locally, that I had to touch it while prototyping these changes. I have no logs to point to, and there is just too many things going on to remember precisely. I'm quite sure it wasn't a l'art pour l'art change and there was a reason.)
To make it resistent to a `memdebug.h` included before compiling these functions and bringing it one step closer for inclusion in unity builds.
to try lessen the confusion that `sclose` is all curl-specific, and to untie the target-specific logic from the memdebug logic.
|
If the goal is to get the memdebug to work in unity builds, isn't my unit test split the easier route? If we just make sure each unity bundle has the same memory situation, things are easier. No? |
It seems so, yes. I still think there is room for tidying up around these macro. These What would be real nice if all the |
That can only work if each unity bundle has the same memory setup, but sure. It can probably get done. |
Before this patch `memdebug.c` was compiled as a separate source in unity builds. This was necessary because `memdebug.c` failed to compile if `memdebug.h` was included before it, in `CURLDEBUG` mode. This patch fixes this issue and allows to compile `memdebug.c` as part of the unity source batch. This removes an exception and makes builds perform a notch better. - introduce `CURL_SCLOSE()` macro as an immutable synonym of `sclose()`. - memdebug: replace `sclose()` reference with `CURL_SCLOSE()` to compile as expected when `sclose()` is overridden by `memdebug.h`. - memdebug: make it not break when including `memdebug.h` before it in `CURLDEBUG` mode. Do this by calling low-level functions as `(function)`. - autotools, cmake: drop memdebug exception, include it like any other source file. This is now possible because `memdebug.c` doesn't break if `memdebug.h` was included before it, in `CURLDEBUG` builds. - mk-unity: drop `--exclude` option. No longer used after this patch. - drop `MEMDEBUG_NODEFINES` macro hack. No longer necessary. Ref: curl#16747 Closes curl#16746 Closes curl#16738 Closes curl#17631
To make it resistent to a
memdebug.hincluded before compiling it inCURLDEBUGmode, and making theMEMDEBUG_NODEFINEShack unnecessary.Also bringing it a step closer for inclusion in unity builds.
fopen()inCURLDEBUGbuilds #16747