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

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.

On 2020-08-06 at 23:47:34, Junio C Hamano wrote:

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" <sandals@crustytoothpaste.net> writes:

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(-)

On 2020-08-07 at 00:23:07, Junio C Hamano wrote:

Maybe "no op" or no-op?

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.

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" <sandals@crustytoothpaste.net> writes:

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

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.

On Thu, Aug 06, 2020 at 05:23:07PM -0700, Junio C Hamano wrote:

This one looks safe and seems like an improvement, since it's our own function that we're redefining.

But this one for example:

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.

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

Jeff King <peff@peff.net> writes:

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.

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.

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 ;-)

See my message to Brian.

On Thu, Aug 06, 2020 at 09:09:38PM -0700, Junio C Hamano wrote:

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.

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

Jeff King <peff@peff.net> writes:

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.

On Thu, Aug 06, 2020 at 04:47:34PM -0700, Junio C Hamano wrote:

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.