"#define precompose_argv(c,v) /* empty */" is evil

"#define precompose_argv(c,v) /* empty */" is evil

[Junio C Hamano]

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.

Re: "#define precompose_argv(c,v) /* empty */" is evil

[brian m. carlson]

[-- 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 --]

Re: "#define precompose_argv(c,v) /* empty */" is evil

[Junio C Hamano]

"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
 

Re: "#define precompose_argv(c,v) /* empty */" is evil

[brian m. carlson]

[-- 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 --]

Re: "#define precompose_argv(c,v) /* empty */" is evil

[Junio C Hamano]

"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.

Re: "#define precompose_argv(c,v) /* empty */" is evil

[Jeff King]

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

Re: "#define precompose_argv(c,v) /* empty */" is evil

[Junio C Hamano]

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.

Re: "#define precompose_argv(c,v) /* empty */" is evil

[Jeff King]

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

Re: "#define precompose_argv(c,v) /* empty */" is evil

[Junio C Hamano]

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.

Re: "#define precompose_argv(c,v) /* empty */" is evil

[Torsten Bögershausen]

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.