Working code

[anti...@math.uni.wroc.pl]

The following is simplified version of "working code": it
is more than 20 years old (probably closer to 30 years old)
and apparently did what its author intended:

int (*handlers[2])();

#define DO_IT(n) ((p = handlers[n]) ? (*p)() : 0)

static void foo() {
register int (*p)();
register int bar = DO_IT(0) | DO_IT(1);
/* ... */
/* Use of bar */
}

/* Code that fills 'handlers' and calls foo() */

I wonder what you think about this code?

--
Waldek Hebisch

[Tim Rentsch]

I think it has undefined behavior, and needlessly so:

#define DO_IT(n) (handlers[n] ? handlers[n]() : 0)

...

[Andrey Tarasevich]

That intermediate variable `p` is indeed completely unnecessary. But
where did you find undefined behavior in the original version?

--
Best regards,
Andrey Tarasevich

[Andrey Tarasevich]

On 9/1/2021 3:23 PM, anti...@math.uni.wroc.pl wrote:
>
> I wonder what you think about this code?
>

The macro is idiotically named: `DO_IT`? Seriously?

The reliance on the caller-provided local variable `p` doesn't make much
sense. Why would they even need an intermediate variable?

Bitwise operator is applied to a signed type - that doesn't look good.
Can't say more since it depends on the semantics of the values involved.

Other than that the code looks fine.

[Ike Naar]

In the expression 'DO_IT(0) | DO_IT(1);' p is being modified more than once.
The behaviour of the expression depends on how its subexpressions are sequenced.

[Andrey Tarasevich]

Ah... I see. Indeed. Good point.

[Ben Bacarisse]

There is a sequence point after each assignment. Am I missing something?

--
Ben.

[Richard Damon]

No, there is a dependency that that assignment can't happen until after
the right hand side is evalutated, but no full sequence point.

There is no sequencing between the assignments of DO_IT(0) and DO_IT(1)
and very importantly between the (*p)()s, both assignements can happen
then both (*p)() happen, using whichever value of p was assigned last.

[Ben Bacarisse]

What's a full sequence point?

> There is no sequencing between the assignments of DO_IT(0) and DO_IT(1)
> and very importantly between the (*p)()s, both assignements can happen
> then both (*p)() happen, using whichever value of p was assigned last.

There is one between each assignment and the corresponding use, but you
are saying that that is not enough. Have I got that about right? Is
this derived from the new C11 "sequenced before" wording that replaced
the old wording C99 or was it UB in C99 as well?

--
Ben.

[John McCue]

Andrey Tarasevich <andreyta...@hotmail.com> wrote:
> On 9/1/2021 3:23 PM, anti...@math.uni.wroc.pl wrote:

<snip>

>
> The macro is idiotically named: `DO_IT`? Seriously?

Must be an old timer thing, I have doit()s in a lot
of places. But never called a macro that :)

<snip>

[James Kuyper]

Expanding the macros, the relevant expression becomes

((p = handlers[0]) ? (*p)() : 0)|((p = handlers[1]) ? (*p)() : 0)

That contains the following sub-expressions involving p:

A: p = handlers[0]
B: (*p)()
C: p = handlers[1]
D: (*p)()

There's a sequence point associated with the first ?: expression
separating A and B(6.5.15p4), guaranteeing that A is sequenced before B
(5.1.2.3p3) . There's a sequence point associated with the second ?:
expression separating C and D, guaranteeing that C is sequenced before
D. There is no sequence point associated with the |, so the two
evaluations of DO_IT() are unsequenced relative to each other.
Importantly, this means that it is permissible for the evaluation of the
two ?: expressions to be interleaved (see footnote 13).
In itself, this would be bad enough, because the possibility of
interleaving those evaluations means that, in each of the two places
where (*p)() is evaluated, there's no guarantee whether p has the value
handlers[0] or handlers[1].
However, because A and B both assign values to the same variable, the
fact that they are not sequenced relative to each other has worse
implications: the behavior is undefined (6.5p2).

While I've worded this argument in terms of the new "sequencing"
terminology introduced in C2011, the same conclusion applied (though
less clearly) in older versions of the standard. The relevant clause was
one I have occasionally referred to as one of the most confusing in the
standard: "Between the previous and next sequence point an object shall
have its stored value modified at most once by the evaluation of an
expression. 72) Furthermore, the prior value shall be read only to
determine the value to be stored." (C99, 6.5p2).
That version of the standard didn't explicitly permit interleaved
evaluation, it simply failed to prohibit it. A defect report resolution
confirmed that failing to prohibit interleaving was deliberate.

[Tim Rentsch]

The C standard doesn't use the term "full sequence point". It
does define the term "full expression", which includes expression
statements and declaration initializers (and a few others), and
says there is a sequence point at the end of each full expression.

>> There is no sequencing between the assignments of DO_IT(0) and
>> DO_IT(1) and very importantly between the (*p)()s, both
>> assignements can happen then both (*p)() happen, using whichever
>> value of p was assigned last.
>
> There is one between each assignment and the corresponding use,
> but you are saying that that is not enough. Have I got that about
> right? Is this derived from the new C11 "sequenced before"
> wording that replaced the old wording C99 or was it UB in C99 as
> well?

To answer the second question first, the new phrasing in C11
using "sequenced before", etc, clarified the rules but did not
the change the rules. The example here is just as much UB in
C90 and C99 as it is in C11.

Regarding the first question, the problem is that there are two
assignments to the variable 'p' without any sequence point
between the two assignments. It is the assignments that cause
the problem; the subsequent uses don't matter. An AST for the
expression would look something like this:

"bitwise or(|)"
/ \
/ \
/ \
/ \
?: ?:
/ | \ / | \
/ | \ / | \
/ | \ / | \
p=... (*p)() 0 p=... (*p)() 0

A bitwise-or operator has no sequencing, so it is free to
evaluate its subexpressions simultaneously. The ?: operator on
the left branch does impose a sequencing between 'p=...' and its
other operands, and similarly for the right branch. However, no
sequencing is imposed between the first 'p=...' and the second
'p=...', because the bitwise-or doesn't specify an order of
evaluation, and the two ?: operators impose an ordering only
within each subexpression, not between the two subexpressions.

If the bitwise-or ('|') were replaced with a logical-or ('||')
then there would be no undefined behavior. Of course doing that
would change the semantics.

If anyone is interested the principle here is addressed in an
early defect report a few years after C90 was issued:

http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_087.html