Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c > #162558 > unrolled thread

Working code

Back to article view | Back to comp.lang.c

Contents

  Working code antispam@math.uni.wroc.pl - 2021-09-01 22:23 +0000
    Re: Working code Tim Rentsch <tr.17687@z991.linuxsc.com> - 2021-09-01 16:58 -0700
      Re: Working code Andrey Tarasevich <andreytarasevich@hotmail.com> - 2021-09-01 20:57 -0700
        Re: Working code Ike Naar <ike@rie.sdf.org> - 2021-09-02 05:22 +0000
          Re: Working code Andrey Tarasevich <andreytarasevich@hotmail.com> - 2021-09-01 22:33 -0700
          Re: Working code Ben Bacarisse <ben.usenet@bsb.me.uk> - 2021-09-02 11:54 +0100
            Re: Working code Richard Damon <Richard@Damon-Family.org> - 2021-09-02 07:08 -0400
              Re: Working code Ben Bacarisse <ben.usenet@bsb.me.uk> - 2021-09-02 13:37 +0100
                Re: Working code James Kuyper <jameskuyper@alumni.caltech.edu> - 2021-09-02 12:03 -0400
                Re: Working code Tim Rentsch <tr.17687@z991.linuxsc.com> - 2021-09-02 09:57 -0700
    Re: Working code Andrey Tarasevich <andreytarasevich@hotmail.com> - 2021-09-01 21:02 -0700
      Re: Working code John McCue <jmccue@obsd2.mhome.org> - 2021-09-02 13:31 +0000

#162558 — Working code

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

[toc] | [next] | [standalone]

#162562

antispam@math.uni.wroc.pl writes:

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

I think it has undefined behavior, and needlessly so:

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

    ...

[toc] | [prev] | [next] | [standalone]

#162569

On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>> 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?
> 
> I think it has undefined behavior, and needlessly so:
> 
>      #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
> 
>      ...

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

-- 
Best regards,
Andrey Tarasevich

[toc] | [prev] | [next] | [standalone]

#162572

On 2021-09-02, Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
> On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>>> 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?
>> 
>> I think it has undefined behavior, and needlessly so:
>> 
>>      #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
>> 
>>      ...
>
> That intermediate variable `p` is indeed completely unnecessary. But 
> where did you find undefined behavior in the original version?

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.

[toc] | [prev] | [next] | [standalone]

#162573

On 9/1/2021 10:22 PM, Ike Naar wrote:
> On 2021-09-02, Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
>> On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>>>> 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?
>>>
>>> I think it has undefined behavior, and needlessly so:
>>>
>>>       #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
>>>
>>>       ...
>>
>> That intermediate variable `p` is indeed completely unnecessary. But
>> where did you find undefined behavior in the original version?
> 
> 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.

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

-- 
Best regards,
Andrey Tarasevich

[toc] | [prev] | [next] | [standalone]

#162576

Ike Naar <ike@rie.sdf.org> writes:

> On 2021-09-02, Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
>> On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>>>> 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?
>>> 
>>> I think it has undefined behavior, and needlessly so:
>>> 
>>>      #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
>>> 
>>>      ...
>>
>> That intermediate variable `p` is indeed completely unnecessary. But 
>> where did you find undefined behavior in the original version?
>
> 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.

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

-- 
Ben.

[toc] | [prev] | [next] | [standalone]

#162577

On 9/2/21 6:54 AM, Ben Bacarisse wrote:
> Ike Naar <ike@rie.sdf.org> writes:
> 
>> On 2021-09-02, Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
>>> On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>>>>> 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?
>>>>
>>>> I think it has undefined behavior, and needlessly so:
>>>>
>>>>      #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
>>>>
>>>>      ...
>>>
>>> That intermediate variable `p` is indeed completely unnecessary. But 
>>> where did you find undefined behavior in the original version?
>>
>> 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.
> 
> There is a sequence point after each assignment.  Am I missing something?
> 

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.

[toc] | [prev] | [next] | [standalone]

#162581

Richard Damon <Richard@Damon-Family.org> writes:

> On 9/2/21 6:54 AM, Ben Bacarisse wrote:
>> Ike Naar <ike@rie.sdf.org> writes:
>> 
>>> On 2021-09-02, Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
>>>> On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>>>>>> 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?
>>>>>
>>>>> I think it has undefined behavior, and needlessly so:
>>>>>
>>>>>      #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
>>>>>
>>>>>      ...
>>>>
>>>> That intermediate variable `p` is indeed completely unnecessary. But 
>>>> where did you find undefined behavior in the original version?
>>>
>>> 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.
>> 
>> There is a sequence point after each assignment.  Am I missing
>> something?
>
> No, there is a dependency that that assignment can't happen until after
> the right hand side is evalutated, but no full sequence point.

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.

[toc] | [prev] | [next] | [standalone]

#162587

On 9/2/21 8:37 AM, Ben Bacarisse wrote:
> Richard Damon <Richard@Damon-Family.org> writes:
> 
>> On 9/2/21 6:54 AM, Ben Bacarisse wrote:
>>> Ike Naar <ike@rie.sdf.org> writes:
>>>
>>>> On 2021-09-02, Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
>>>>> On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>>>>>>> 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?
>>>>>>
>>>>>> I think it has undefined behavior, and needlessly so:
>>>>>>
>>>>>>      #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
>>>>>>
>>>>>>      ...
>>>>>
>>>>> That intermediate variable `p` is indeed completely unnecessary. But 
>>>>> where did you find undefined behavior in the original version?
>>>>
>>>> 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.
>>>
>>> There is a sequence point after each assignment.  Am I missing
>>> something?
>>
>> No, there is a dependency that that assignment can't happen until after
>> the right hand side is evalutated, but no full sequence point.
> 
> 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?
> 

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.

[toc] | [prev] | [next] | [standalone]

#162588

Ben Bacarisse <ben.usenet@bsb.me.uk> writes:

> Richard Damon <Richard@Damon-Family.org> writes:
>
>> On 9/2/21 6:54 AM, Ben Bacarisse wrote:
>>
>>> Ike Naar <ike@rie.sdf.org> writes:
>>>
>>>> On 2021-09-02, Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
>>>>
>>>>> On 9/1/2021 4:58 PM, Tim Rentsch wrote:
>>>>>
>>>>>>> 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?
>>>>>>
>>>>>> I think it has undefined behavior, and needlessly so:
>>>>>>
>>>>>>      #define DO_IT(n)   (handlers[n] ? handlers[n]() : 0)
>>>>>>
>>>>>>      ...
>>>>>
>>>>> That intermediate variable `p` is indeed completely unnecessary.
>>>>> But where did you find undefined behavior in the original
>>>>> version?
>>>>
>>>> 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.
>>>
>>> There is a sequence point after each assignment.  Am I missing
>>> something?
>>
>> No, there is a dependency that that assignment can't happen until
>> after the right hand side is evalutated, but no full sequence
>> point.
>
> What's a full sequence point?

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

[toc] | [prev] | [next] | [standalone]

#162571

On 9/1/2021 3:23 PM, antispam@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.

-- 
Best regards,
Andrey Tarasevich

[toc] | [prev] | [next] | [standalone]

#162583

Andrey Tarasevich <andreytarasevich@hotmail.com> wrote:
> On 9/1/2021 3:23 PM, antispam@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>

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.c

csiph-web