[sane-devel] Fixing sequencing issues (was Re: Proposed Upgrade (pu) branches on GitLab)

[Olaf Meeuwissen]

Hi Johannes,

I'm just chiming in to make a point to the list in general here.
Nothing personal ;-)

Johannes Meixner writes:

> Hello,
>
> On Sep 24 22:22 Olaf Meeuwissen wrote (excerpt):
>> One, in sanei_ir.c, deserves some attention as it produces potentially
>> undefined behaviour.  It's really the same issue as reported in 311857,
>> which I recently reopened.
>>
>>  ../../../sanei/sanei_ir.c:481:11: warning: multiple unsequenced modifications to 'outi' [-Wunsequenced]
>>     *outi++ = *outi++ >> is;
>>
>> The two increments may occur in any given order, IIUC[3].
>>
>> [3] http://c-faq.com/expr/seqpoints.html
>
> In general I would even demand that nowadays such kind
> of fancy coding stlye should no longer be done at all.

Not totally clear on what the FAQ is saying, I decided to put code like
the above to the test.  Consider this code:

  #include <stdio.h>
  #include <stdlib.h>

  int
  main (int argc, char *argv[])
  {
    int a[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
    int i;

    int *p = a;
    for (i = 0; i < sizeof (a) / sizeof (*a) / 2; ++i)
      {
        *p++ = *p++ >> 3;
      }

    fprintf (stdout, "a = { %i", a[0]);
    for (i = 1; i < sizeof (a) / sizeof (*a); ++i)
      {
         fprintf (stdout, ", %i", a[i]);
      }
    fprintf (stdout, " }\n");

    return EXIT_SUCCESS;
  }

Those who think they know what the output will be, by all means, write
it down now before reading any further.

I compiled the above code on Debian stable (amd64) with the default
versions of clang and gcc.  The only compile-time flag I used was -ansi.
BTW, optimization level does not matter.  I ran both programs and this
is what I got:

  clang: a = { 0, 0, 2, 0, 4, 0, 6, 0, 8, 1 }
  gcc  : a = { 0, 1, 0, 3, 0, 5, 0, 7, 1, 9 }

If any of you think either compiler is buggy (or even both?), feel free
to submit a bug report with the respective compiler projects.  In the
mean time, I think we should any code that is flagged as -Wunsequenced
(by clang) or -Wsequence-point (by gcc).

# If any of you think everyone uses gcc, think again.  Debian, for one,
# has been very busy compiling *all* of its packages with clang.

I "reverse engineered" the compilers' sequencing order.  It turns out
that gcc uses the equivalent of

  int q = *p >> 3;
  *p = q;
  p++;
  p++;

where as clang decided to use the equivalent of

  int q = *p >> 3;
  p++;
  *p = q;
  p++;

When forcing the sequence order as above, both compilers yield the same
results.

So, if we know which of these compilers the code was written against, we
can fix our code.  If not, and that includes the "yet-another-compiler"
scenario, we cannot and need the original author of the code to step in
and fix this *bug*.

> With nowadays compiler optimizations it does not matter
> if one writes
>
>    y = x++ * 2;
>
> or
>
>    y = x * 2;
>    x++;
>
> The former is selfish and oversophisticated coding.
> In this case it is a mix-up of two separated things.
>
> The latter is altruistic and explicit coding that
> makes it obvious for others what actually is meant.

I'm not that much of a source code "extremist", but I do subscribe to
the (self-invented?) AWARE principle:

  All Warnings Are Really Errors

As such, warnings should be fixed.  I am well aware (no pun intended!)
of the various versions of various compilers on various platforms all
having their own ideas of what should and should not be flagged as a
warning when.  That notwithstanding, a project can select a canonical
setup up, use -Werror there and not release unless the compile passes
on that setup.

# Now here's an idea for 1.0.26!

Hope this helps,
-- 
Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
Support Free Software               Support the Free Software Foundation
https://my.fsf.org/donate                        https://my.fsf.org/join