* "#define precompose_argv(c,v) /* empty */" is evil @ 2020-08-06 23:47 Junio C Hamano 2020-08-07 0:01 ` brian m. carlson 2020-08-07 7:56 ` Torsten Bögershausen 0 siblings, 2 replies; 10+ messages in thread From: Junio C Hamano @ 2020-08-06 23:47 UTC (permalink / raw) To: git I had seen an interesting compilation breakage today. A topic adds many more uses of argv-array API so I resolved semantic conflict patch until "make builtins/submodule-helper.o" passed. I failed to spot one remaining breakage until I saw https://travis-ci.org/github/git/git/jobs/715668996 The problematic line was precompose_argv(diff_args.argc, diff_args.argv); where diff_args used to be an argv_array and is now a strvec. Why didn't I catch this in my local test? $ git grep -n -e precompose_argv -- \*.h compat/precompose_utf8.h:31:void precompose_argv(int argc, const char **argv); git-compat-util.h:256:#define precompose_argv(c,v) The problematic part is this one used on all platforms other than macOS: /* used on Mac OS X */ #ifdef PRECOMPOSE_UNICODE #include "compat/precompose_utf8.h" #else #define precompose_str(in,i_nfd2nfc) #define precompose_argv(c,v) #define probe_utf8_pathname_composition() #endif I am wondering if it is a good idea to use something like static inline void precompose_argv(int argc, const char **argv) { ; /* nothing */ } instead. As long as the compiler is reasonable enough, this should not result in any code change in the result, except that it would still catch wrong arguments, even if these two parameters are unused and optimized out. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-06 23:47 "#define precompose_argv(c,v) /* empty */" is evil Junio C Hamano @ 2020-08-07 0:01 ` brian m. carlson 2020-08-07 0:23 ` Junio C Hamano 2020-08-07 7:56 ` Torsten Bögershausen 1 sibling, 1 reply; 10+ messages in thread From: brian m. carlson @ 2020-08-07 0:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: git [-- Attachment #1: Type: text/plain, Size: 739 bytes --] On 2020-08-06 at 23:47:34, Junio C Hamano wrote: > I am wondering if it is a good idea to use something like > > static inline void precompose_argv(int argc, const char **argv) > { > ; /* nothing */ > } > > instead. As long as the compiler is reasonable enough, this should > not result in any code change in the result, except that it would > still catch wrong arguments, even if these two parameters are unused > and optimized out. Yes, this seems like a prudent approach. I believe it's widely used by the Linux kernel, so presumably compilers are capable enough to optimize it out. As you noted, it provides type checking for all platforms, which is nice. -- brian m. carlson: Houston, Texas, US [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-07 0:01 ` brian m. carlson @ 2020-08-07 0:23 ` Junio C Hamano 2020-08-07 1:32 ` brian m. carlson 2020-08-07 3:27 ` Jeff King 0 siblings, 2 replies; 10+ messages in thread From: Junio C Hamano @ 2020-08-07 0:23 UTC (permalink / raw) To: brian m. carlson; +Cc: git "brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2020-08-06 at 23:47:34, Junio C Hamano wrote: >> I am wondering if it is a good idea to use something like >> >> static inline void precompose_argv(int argc, const char **argv) >> { >> ; /* nothing */ >> } >> >> instead. As long as the compiler is reasonable enough, this should >> not result in any code change in the result, except that it would >> still catch wrong arguments, even if these two parameters are unused >> and optimized out. > > Yes, this seems like a prudent approach. I believe it's widely used by > the Linux kernel, so presumably compilers are capable enough to optimize > it out. As you noted, it provides type checking for all platforms, > which is nice. So I hope the following (untested and not signed off yet) may lead us in the right direction? -- >8 -- Subject: compat-util: type-check parameters of mocked functions When there is no need to run a specific function on certain platforms, we often #define an empty function to swallow its parameters and make it into a no-op, e.g. #define precompose_argv(c,v) /* no-op */ While this guarantees that no unneeded code is generated, it also discards type and other checks on these parameters, e.g. a new code written with the argv-array API (diff_args is of type "struct argv_array" that has .argc and .argv members): precompose_argv(diff_args.argc, diff_args.argv); must be updated to use "struct strvec diff_args" with .nr and .v members, like so: precompose_argv(diff_args.nr, diff_args.v); after the argv-array API has been updated to the strvec API. However, the "no oop" C preprocessor macro is too aggressive to discard what is unused, and did not catch such a call that was left unconverted. Using a "static inline" function whose body is a no-op should still result in the same binary with decent compilers yet catch such a reference to a missing field or passing a value of a wrong type. While at it, I notice that precompute_str() has never been used anywhere in the code, since it was introduced at 76759c7d (git on Mac OS and precomposed unicode, 2012-07-08). Instead of turning it into a static inline, just remove it. --- git-compat-util.h | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index 5637114b8d..7a0fb7a045 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -252,8 +252,10 @@ typedef unsigned long uintptr_t; #ifdef PRECOMPOSE_UNICODE #include "compat/precompose_utf8.h" #else -#define precompose_str(in,i_nfd2nfc) -#define precompose_argv(c,v) +static inline void precompose_argv(int argc, const char **argv) +{ + ; /* nothing */ +} #define probe_utf8_pathname_composition() #endif @@ -270,7 +272,9 @@ struct itimerval { #endif #ifdef NO_SETITIMER -#define setitimer(which,value,ovalue) +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { + ; /* nothing */ +} #endif #ifndef NO_LIBGEN_H @@ -1231,8 +1235,14 @@ int warn_on_fopen_errors(const char *path); #endif #ifndef _POSIX_THREAD_SAFE_FUNCTIONS -#define flockfile(fh) -#define funlockfile(fh) +static inline void flockfile(FILE *fh) +{ + ; /* nothing */ +} +static inline void funlockfile(FILE *fh) +{ + ; /* nothing */ +} #define getc_unlocked(fh) getc(fh) #endif ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-07 0:23 ` Junio C Hamano @ 2020-08-07 1:32 ` brian m. carlson 2020-08-07 4:03 ` Junio C Hamano 2020-08-07 3:27 ` Jeff King 1 sibling, 1 reply; 10+ messages in thread From: brian m. carlson @ 2020-08-07 1:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: git [-- Attachment #1: Type: text/plain, Size: 2825 bytes --] On 2020-08-07 at 00:23:07, Junio C Hamano wrote: > While this guarantees that no unneeded code is generated, it also > discards type and other checks on these parameters, e.g. a new code > written with the argv-array API (diff_args is of type "struct > argv_array" that has .argc and .argv members): > > precompose_argv(diff_args.argc, diff_args.argv); > > must be updated to use "struct strvec diff_args" with .nr and .v > members, like so: > > precompose_argv(diff_args.nr, diff_args.v); > > after the argv-array API has been updated to the strvec API. > However, the "no oop" C preprocessor macro is too aggressive to Maybe "no op" or no-op? > discard what is unused, and did not catch such a call that was left > unconverted. > > Using a "static inline" function whose body is a no-op should still > result in the same binary with decent compilers yet catch such a > reference to a missing field or passing a value of a wrong type. > > While at it, I notice that precompute_str() has never been used > anywhere in the code, since it was introduced at 76759c7d (git on > Mac OS and precomposed unicode, 2012-07-08). Instead of turning it > into a static inline, just remove it. Great. I was wondering about that when I looked at the patch. If we're not using it, no point in keeping it. I think the name should be "precompose_str", though. > --- > git-compat-util.h | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/git-compat-util.h b/git-compat-util.h > index 5637114b8d..7a0fb7a045 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -252,8 +252,10 @@ typedef unsigned long uintptr_t; > #ifdef PRECOMPOSE_UNICODE > #include "compat/precompose_utf8.h" > #else > -#define precompose_str(in,i_nfd2nfc) > -#define precompose_argv(c,v) > +static inline void precompose_argv(int argc, const char **argv) > +{ > + ; /* nothing */ > +} > #define probe_utf8_pathname_composition() > #endif > > @@ -270,7 +272,9 @@ struct itimerval { > #endif > > #ifdef NO_SETITIMER > -#define setitimer(which,value,ovalue) > +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { The rest of the patch looks fine, but do we know that these structs will exist if NO_SETITIMER is defined? If not, we may need to use a void * here, which would provide us worse type checking, but would work on platforms that lack the interval timers at all, such as NonStop. That does kind of defeat the purpose of this patch, but I still think it's a win, since we end up with some type checking, even if it's not perfect, and almost every platform provides setitimer, so any errors will be caught quickly. -- brian m. carlson: Houston, Texas, US [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-07 1:32 ` brian m. carlson @ 2020-08-07 4:03 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2020-08-07 4:03 UTC (permalink / raw) To: brian m. carlson; +Cc: git "brian m. carlson" <sandals@crustytoothpaste.net> writes: >> #ifdef NO_SETITIMER >> -#define setitimer(which,value,ovalue) >> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { > > The rest of the patch looks fine, but do we know that these structs will > exist if NO_SETITIMER is defined? If not, we may need to use a void * > here, which would provide us worse type checking, but would work on > platforms that lack the interval timers at all, such as NonStop. I thought about that and also making s/FILE/void/ for flockfile() and funlockfile() for the same reason. Indeed my first draft used "void *". But because these no-op macros are designed to be used in the main codepath WITHOUT surrounding #ifdef...#endif for readability, the platforms that use NO_SETITIMER has to declare the variable that the calling site of setitimer() passes as its parameters, so they must have something called "struct itimerval". That is why I ended up using the real type here For example, builtin/log.c defines static struct itimerval early_output_timer; and makes an unconditional call OUTSIDE any #ifdef...#endif to setitimer(), like so: early_output_timer.it_value.tv_sec = 0; early_output_timer.it_value.tv_usec = 500000; setitimer(ITIMER_REAL, &early_output_timer, NULL); I would expect that this is the use pattern any users of these fallback definitions in git-compat-util.h should follow; those who do not have "struct itimerval" natively indeed are using a fallback definition from <git-compat-util.h>. > That does kind of defeat the purpose of this patch, but I still think > it's a win, since we end up with some type checking, even if it's not > perfect, and almost every platform provides setitimer, so any errors > will be caught quickly. Yes, even if we loosen the type to "void *", it does catch certain errors. One thing I wrote in the log message is that moving to "static inline" allows us to catch not just type mismatches but also missing variables (i.e. the original code used a variable that has been renamed, and the instances of the variable used as parameters to these no-op macros were left unmodified). That's not a type mismatch but missing identifier. The motivating example was quite similar; it was a field renamed but left unadjusted. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-07 0:23 ` Junio C Hamano 2020-08-07 1:32 ` brian m. carlson @ 2020-08-07 3:27 ` Jeff King 2020-08-07 4:09 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Jeff King @ 2020-08-07 3:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: brian m. carlson, git On Thu, Aug 06, 2020 at 05:23:07PM -0700, Junio C Hamano wrote: > diff --git a/git-compat-util.h b/git-compat-util.h > index 5637114b8d..7a0fb7a045 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -252,8 +252,10 @@ typedef unsigned long uintptr_t; > #ifdef PRECOMPOSE_UNICODE > #include "compat/precompose_utf8.h" > #else > -#define precompose_str(in,i_nfd2nfc) > -#define precompose_argv(c,v) > +static inline void precompose_argv(int argc, const char **argv) > +{ > + ; /* nothing */ > +} This one looks safe and seems like an improvement, since it's our own function that we're redefining. But this one for example: > -#define flockfile(fh) > -#define funlockfile(fh) > +static inline void flockfile(FILE *fh) > +{ > + ; /* nothing */ > +} > +static inline void funlockfile(FILE *fh) > +{ > + ; /* nothing */ > +} is re-defining a name that's usually reserved for the system (at least on POSIX systems). For most systems that define it, we'd actually use the system version (and not compile this code at all). But there may be systems where we choose not to (either the system version is deficient, or we're testing the fallback code on a more-capable system, or our #ifndef check isn't sufficient on that system for some reason). If the system defines it as a macro, we'd probably get a garbled compiler error as the macro is expanded here. Adding #undef flockfile, etc beforehand would help. I'm not sure if the current code might give us a macro redefinition warning on such a system. If the system defines it as a function, we'd probably get redefinition warnings. So...I dunno. Those are all theoretical complaints. But I also think what it's buying is not very big: - unlike precompose_argv(), modern POSIX-compliant systems (which we all tend to develop on) don't hit this fallback code. So your average developer is likely to see any problems here. - this is really the tip of the portability iceberg anyway. In the example that motivated this thread, you were catching failures to adjust to strvec. But in code like this: #ifdef FOO void some_func(int x, const char *y) { struct argv_array whatever = ARGV_ARRAY_INIT; ... } #else void some_func(int x, const char *y) { /* do nothing */ } #endif You'd still fail to notice the problem if you're not compiling with -DFOO. I think we have to accept that the compiler on a given platform won't be able to catch everything. That's why we have CI on many platforms, plus people building and reporting problems on their own systems. > @@ -270,7 +272,9 @@ struct itimerval { > #endif > > #ifdef NO_SETITIMER > -#define setitimer(which,value,ovalue) > +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { > + ; /* nothing */ > +} > #endif Same reasoning applies to this one, plus the added bonus that we'd need that struct type defined. brian mentioned using "void *". That works as long as the system doesn't define setitimer() itself with the real types. Another option is to forward declare "struct itimerval" (which _should_ be OK even if the system does so, since our forward declaration has no details). But again, I'm not sure if it's worth the hassle. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-07 3:27 ` Jeff King @ 2020-08-07 4:09 ` Junio C Hamano 2020-08-07 4:34 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2020-08-07 4:09 UTC (permalink / raw) To: Jeff King; +Cc: brian m. carlson, git Jeff King <peff@peff.net> writes: > But this one for example: > >> -#define flockfile(fh) >> -#define funlockfile(fh) >> +static inline void flockfile(FILE *fh) >> +{ >> + ; /* nothing */ >> +} >> +static inline void funlockfile(FILE *fh) >> +{ >> + ; /* nothing */ >> +} > > is re-defining a name that's usually reserved for the system (at least > on POSIX systems). For most systems that define it, we'd actually use > the system version (and not compile this code at all). > But there may be > systems where we choose not to (either the system version is deficient, > or we're testing the fallback code on a more-capable system, or our > #ifndef check isn't sufficient on that system for some reason). Hmph, perhaps. We'll cross that bridge when we need to port to such a system, and until then, doing this will more easily catch the need to cross that bridge, I would imagine. > If the system defines it as a macro, we'd probably get a garbled > compiler error as the macro is expanded here. Adding #undef flockfile, > etc beforehand would help. I'm not sure if the current code might give > us a macro redefinition warning on such a system. > > If the system defines it as a function, we'd probably get redefinition > warnings. > > So...I dunno. Those are all theoretical complaints. But I also think > what it's buying is not very big: > > - unlike precompose_argv(), modern POSIX-compliant systems (which we > all tend to develop on) don't hit this fallback code. So your > average developer is likely to see any problems here. That's OK. I am not particularly interested in systems that has to ifdef out flockfile() and funlockfile(). I did these two primarily to reduce the number of instances of the bad pattern to implement a no-op replacement as C-preprocessor macro that can be copied by less experienced developers. > - this is really the tip of the portability iceberg anyway. In the > example that motivated this thread, you were catching failures to > adjust to strvec. But in code like this: > > #ifdef FOO > void some_func(int x, const char *y) > { > struct argv_array whatever = ARGV_ARRAY_INIT; > ... > } > #else > void some_func(int x, const char *y) > { > /* do nothing */ > } > #endif Yes, of course, but as I wrote in my response to Brian, the whole point of using these replacement implementation macros is so that we do not have to sprinkle the main code with such #ifdef/#endif, so I think the code like that is what needs to be corrected ;-) >> @@ -270,7 +272,9 @@ struct itimerval { >> #endif >> >> #ifdef NO_SETITIMER >> -#define setitimer(which,value,ovalue) >> +static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { >> + ; /* nothing */ >> +} >> #endif > > Same reasoning applies to this one, plus the added bonus that we'd need > that struct type defined. brian mentioned using "void *". See my message to Brian. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-07 4:09 ` Junio C Hamano @ 2020-08-07 4:34 ` Jeff King 2020-08-07 6:42 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2020-08-07 4:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: brian m. carlson, git On Thu, Aug 06, 2020 at 09:09:38PM -0700, Junio C Hamano wrote: > > But there may be > > systems where we choose not to (either the system version is deficient, > > or we're testing the fallback code on a more-capable system, or our > > #ifndef check isn't sufficient on that system for some reason). > > Hmph, perhaps. We'll cross that bridge when we need to port to such > a system, and until then, doing this will more easily catch the need > to cross that bridge, I would imagine. I agree my concern is theoretical. It might not bite us. If you're willing to deal with it when it does (and inevitably as maintainer you will ;) ), then I'm OK with trying it out. > > - this is really the tip of the portability iceberg anyway. In the > > example that motivated this thread, you were catching failures to > > adjust to strvec. But in code like this: > > > > #ifdef FOO > > void some_func(int x, const char *y) > > { > > struct argv_array whatever = ARGV_ARRAY_INIT; > > ... > > } > > #else > > void some_func(int x, const char *y) > > { > > /* do nothing */ > > } > > #endif > > Yes, of course, but as I wrote in my response to Brian, the whole > point of using these replacement implementation macros is so that we > do not have to sprinkle the main code with such #ifdef/#endif, so > I think the code like that is what needs to be corrected ;-) I'm not sure if I made my point clearly. At some point you have to define the functions, and they'll have code in them which gets compiled on some systems but not on others. E.g., precompose_argv() uses reencode_string_iconv(). What if we change the signature of that function? You will not ever catch it by compiling on your Linux box, and would only see it in macOS CI, etc. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-07 4:34 ` Jeff King @ 2020-08-07 6:42 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2020-08-07 6:42 UTC (permalink / raw) To: Jeff King; +Cc: brian m. carlson, git Jeff King <peff@peff.net> writes: >> Yes, of course, but as I wrote in my response to Brian, the whole >> point of using these replacement implementation macros is so that we >> do not have to sprinkle the main code with such #ifdef/#endif, so >> I think the code like that is what needs to be corrected ;-) > > I'm not sure if I made my point clearly. ... Sorry, indeed I missed it. Using the implementation of precompose_argv() itself as an example, instead of generic looking some_func(), does help to explain why at some point we are bound to have some API calls that are compiled on some platforms and not on others. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: "#define precompose_argv(c,v) /* empty */" is evil 2020-08-06 23:47 "#define precompose_argv(c,v) /* empty */" is evil Junio C Hamano 2020-08-07 0:01 ` brian m. carlson @ 2020-08-07 7:56 ` Torsten Bögershausen 1 sibling, 0 replies; 10+ messages in thread From: Torsten Bögershausen @ 2020-08-07 7:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Aug 06, 2020 at 04:47:34PM -0700, Junio C Hamano wrote: > I had seen an interesting compilation breakage today. A topic adds > many more uses of argv-array API so I resolved semantic conflict > patch until "make builtins/submodule-helper.o" passed. I failed to > spot one remaining breakage until I saw > > https://travis-ci.org/github/git/git/jobs/715668996 > > The problematic line was > > precompose_argv(diff_args.argc, diff_args.argv); > > where diff_args used to be an argv_array and is now a strvec. > > Why didn't I catch this in my local test? > > $ git grep -n -e precompose_argv -- \*.h > compat/precompose_utf8.h:31:void precompose_argv(int argc, const char **argv); > git-compat-util.h:256:#define precompose_argv(c,v) > > The problematic part is this one used on all platforms other than macOS: > > /* used on Mac OS X */ > #ifdef PRECOMPOSE_UNICODE > #include "compat/precompose_utf8.h" > #else > #define precompose_str(in,i_nfd2nfc) > #define precompose_argv(c,v) > #define probe_utf8_pathname_composition() > #endif > > I am wondering if it is a good idea to use something like > > static inline void precompose_argv(int argc, const char **argv) > { > ; /* nothing */ > } > > instead. As long as the compiler is reasonable enough, this should > not result in any code change in the result, except that it would > still catch wrong arguments, even if these two parameters are unused > and optimized out. > In my every-day-live these kind of "late detection of breakage" is not uncommon. In other words: this patch allows early detection and is much better. Thanks for cleaning up my mess. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-08-07 7:57 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-06 23:47 "#define precompose_argv(c,v) /* empty */" is evil Junio C Hamano 2020-08-07 0:01 ` brian m. carlson 2020-08-07 0:23 ` Junio C Hamano 2020-08-07 1:32 ` brian m. carlson 2020-08-07 4:03 ` Junio C Hamano 2020-08-07 3:27 ` Jeff King 2020-08-07 4:09 ` Junio C Hamano 2020-08-07 4:34 ` Jeff King 2020-08-07 6:42 ` Junio C Hamano 2020-08-07 7:56 ` Torsten Bögershausen
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).