Could you check this little piece of C code?

[Dann Corbit]

The original code is simply broken. Your code produces the correct result. The original code works by accident on some compilers.

The original code attempts to modify the variables scan and match multiple times between sequence points, so its behavior is undefined.

No. From the FAQ you posted:

A sequence point is a point in time (at the end of the evaluation of a full expression, or at the ||, &&, ?:, or comma operators, or just before a function call) at which the dust has settled and all side effects are guaranteed to be complete.

You are right that the result is not undefined.

However, it is not clear to me that the result is not unspecified.

No, actually, I am now convinced that the result is not unspecified. Even though the arguments can be evaluated in any order for any fragment between sequence points in the statement, in this case it does not matter.

After all, if it performs the first assignment before the second or the second before the first (for instance) the result is the same, since the assignments are identical.

[Milos]

No, actually, I am now convinced that the result is not unspecified. Even though the arguments can be evaluated in any order for any fragment between sequence points in the statement, in this case it does not matter.

After all, if it performs the first assignment before the second or the second before the first (for instance) the result is the same, since the assignments are identical.

You finally got it. The result of the original expression is in general case defined but unspecified. However, since all the elements are identical it is also specified. Since, compiler cannot distinguish a special case from a general one, it issues the warning.
The warning is syntactically correct but semantically wrong.

[wgarvin]

No, actually, I am now convinced that the result is not unspecified. Even though the arguments can be evaluated in any order for any fragment between sequence points in the statement, in this case it does not matter.

After all, if it performs the first assignment before the second or the second before the first (for instance) the result is the same, since the assignments are identical.

You finally got it. The result of the original expression is in general case defined but unspecified. However, since all the elements are identical it is also specified. Since, compiler cannot distinguish a special case from a general one, it issues the warning.
The warning is syntactically correct but semantically wrong.

If the language guaranteed that scan could not point into the 4 bytes occupied by the pointer variable match, and vice versa, then the original expression would not be unspecified at all, because each subexpression of the form (*++p) has only one evaluation order, and each && is a sequence point. So the warning would be spurious in that case.
[Edit: between each sequence point, the two pointer increments have to occur before any reads that depend on the value of that pointer, so the result is unambigous even if there are multiple possible evaluation orders. Its possible the compiler does not track enough state or analyse that state in a nuanced enough way to detect this, though.]

However, if you allow the possibility of that bizarro aliasing, then the result of each comparison is now influenced by the order in which the increments and dereferences are evaluated. In which case the warning is legitimate, even though its about an aliasing situation which nobody would do deliberately.

Were scan and match declared as char* pointers? If thats the case, it might be forcing the compiler to assume that they could alias anything. Even if the compiler is able to prove in this particular case that the bizarro aliasing is impossible, its under no obligation to do that, and issuing the warning is a reasonable thing for it to do.

If they are char* pointers and that is the source of the problem, then it means two things:
(1) the compiler is probably generating stupid code for it also, and
(2) changing them to unsigned char* might remove the bizarro potential aliasing, which would fix the warning and the bad codegen (but I'm not at all sure about this... try it and see.)


Edit: You could probably also replace it with this:

Code: Select all

do {
      } while(
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            scan < strend); 

But I think your version is clearer.

[michiguel]

Do you still get the same warnings (remarks) with this version?

Code: Select all

do {
    if (*++scan != *++match) break;
    if (*++scan != *++match) break;
    if (*++scan != *++match) break;
    if (*++scan != *++match) break;
    if (*++scan != *++match) break;
    if (*++scan != *++match) break;
    if (*++scan != *++match) break;
    if (*++scan != *++match) break;
} while (scan < strend);

Yes, it gets the same warning:

Code: Select all

compression/zlib/deflate.c(1193): remark #981: operands are evaluated in unspecified order
      if (*++scan != *++match) break;
                  ^

Miguel

If yes then the operands being "evaluated in unspecified order" are most probably the operands of the == resp. != operator. Otherwise I would think that the && operands are meant here.

I would not expect any problem related to the "*++scan" type of expression, at least.

What puzzles me is that both C and C++ standards specify exactly the order of evaluation (left-to-right) and operator precedence (pre-increment then dereference then == then &&) for your original piece of code:

1. perform ++scan
2. apply * to the result of ++scan
3. perform ++match
4. apply * to the result of ++match
5. compare both results of * with ==
6. if == evaluates to non-zero then repeat the same for the next (*++scan == *++match) expression
7. apply && to both results of ==
and so on ...

So according to the standard I can't see any possibility of an "unspecified order", which leads me to the conclusion that the Intel compiler gives a remark which is either kind of "wrong", or indicates that Intel generates dubious code here. Since you state that your code is tested and works fine, I would assume the former and would therefore simply suppress that compiler remark with an appropriate option (maybe "-wd981"). This looks better for me than changing correct code based on a dubious compiler remark only.

Sven

[michiguel]

I am trying to silence compiler warnings of the compression libraries for the Gaviota TBs (Zlib in this case).
I think it is important, so the people won't have to turn off their warnings when they incorporate foreign code. I am really close to achieve that, by casting many stupid conversions. However, there is one piece of code that needs to be rewritten. I hate to do that with tested bug-free code (Zlib), but if I change this, the whole set of APIs will compile cleanly with the most strict warnings of the intel compiler. Since I am paranoid, could you double check that I am not screwing anything up? Several people may end up using it.

This is the code

Code: Select all

		/* We check for insufficient lookahead only every 8th comparison;
		 * the 256th check will be made at strstart+258.
		 */
		do {
		} while(
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				scan < strend);

I am planning to comment it out and replace it with

Code: Select all

		/*MAB: to silence compiler warning, intel 981*/
		do {
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
		} while (scan < strend);

I cannot see why it should be slower. It is more verbose but it is more straightforward and 8 warnings are gone (#981: operands are evaluated in unspecified order).

Miguel

You could try this - it looks more like the original code:

Code: Select all

		/* We check for insufficient lookahead only every 8th comparison;
		 * the 256th check will be made at strstart+258.
		 */
		do {
		} while(
				(*++scan == *++match) && (*++scan == *++match) &&
				(*++scan == *++match) && (*++scan == *++match) &&
				(*++scan == *++match) && (*++scan == *++match) &&
				(*++scan == *++match) && (*++scan == *++match) &&
				scan < strend);

It gives the same warning. It is apparently the == that does not like.

Miguel

[Ron Murawski]

I am trying to silence compiler warnings of the compression libraries for the Gaviota TBs (Zlib in this case).
I think it is important, so the people won't have to turn off their warnings when they incorporate foreign code. I am really close to achieve that, by casting many stupid conversions. However, there is one piece of code that needs to be rewritten. I hate to do that with tested bug-free code (Zlib), but if I change this, the whole set of APIs will compile cleanly with the most strict warnings of the intel compiler. Since I am paranoid, could you double check that I am not screwing anything up? Several people may end up using it.

This is the code

Code: Select all

		/* We check for insufficient lookahead only every 8th comparison;
		 * the 256th check will be made at strstart+258.
		 */
		do {
		} while(
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				scan < strend);

I am planning to comment it out and replace it with

Code: Select all

		/*MAB: to silence compiler warning, intel 981*/
		do {
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
			++scan; ++match; if (*scan != *match) break;
		} while (scan < strend);

I cannot see why it should be slower. It is more verbose but it is more straightforward and 8 warnings are gone (#981: operands are evaluated in unspecified order).

Miguel

When I encounter problematic code with warnings. I tend to disable that particular warning just before the warnings start, and I re-enable the warning just after.

That way I'm happy that all other warnings are taken care of and I don't have to worry about nonsensical warnings. I hate changing working code.

If it ain't broke, don't fix it!

Ron

[michiguel]

No, actually, I am now convinced that the result is not unspecified. Even though the arguments can be evaluated in any order for any fragment between sequence points in the statement, in this case it does not matter.

After all, if it performs the first assignment before the second or the second before the first (for instance) the result is the same, since the assignments are identical.

You finally got it. The result of the original expression is in general case defined but unspecified. However, since all the elements are identical it is also specified. Since, compiler cannot distinguish a special case from a general one, it issues the warning.
The warning is syntactically correct but semantically wrong.

If the language guaranteed that scan could not point into the 4 bytes occupied by the pointer variable match, and vice versa, then the original expression would not be unspecified at all, because each subexpression of the form (*++p) has only one evaluation order, and each && is a sequence point. So the warning would be spurious in that case.
[Edit: between each sequence point, the two pointer increments have to occur before any reads that depend on the value of that pointer, so the result is unambigous even if there are multiple possible evaluation orders. Its possible the compiler does not track enough state or analyse that state in a nuanced enough way to detect this, though.]

However, if you allow the possibility of that bizarro aliasing, then the result of each comparison is now influenced by the order in which the increments and dereferences are evaluated. In which case the warning is legitimate, even though its about an aliasing situation which nobody would do deliberately.

Were scan and match declared as char* pointers? If thats the case, it might be forcing the compiler to assume that they could alias anything. Even if the compiler is able to prove in this particular case that the bizarro aliasing is impossible, its under no obligation to do that, and issuing the warning is a reasonable thing for it to do.

If they are char* pointers and that is the source of the problem, then it means two things:
(1) the compiler is probably generating stupid code for it also, and
(2) changing them to unsigned char* might remove the bizarro potential aliasing, which would fix the warning and the bad codegen (but I'm not at all sure about this... try it and see.)

It is already unsigned char *scan, *match;

Edit: You could probably also replace it with this:

Code: Select all

do {
      } while(
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            scan < strend); 

But I think your version is clearer.

Yes, your code works without warning.

I think that the compiler does not like "side effects" before and after the == in general, regardless of their correctness.

Miguel

[Sven]

[...] What puzzles me is that both C and C++ standards specify exactly the order of evaluation (left-to-right) and operator precedence (pre-increment then dereference then == then &&) for your original piece of code: [...]

I guess you are thinking about Java?

In C and C++ there is definitely no left-to-right evaluation order in an expression. Operations 3 and 4 could easily be performed before ops 1 and 2. Or in between.

At least the logical operators like && and || have left-to-right evaluation. I admit I'm not so sure about == and != anymore.

But that should not matter much for the given case, as it is completely unimportant whether in the expression "*++scan == *++match" you execute *++scan first or *++match.

Furthermore, since the standard states that the order of evaluation of function arguments is undefined I think it is more than annoying to see a compiler issue remarks about unspecified evaluation order *anywhere* in the program. We do not want to see such remarks for each function call with more than one argument, do we?

Sven

[Sven]

If the language guaranteed that scan could not point into the 4 bytes occupied by the pointer variable match, and vice versa, then the original expression would not be unspecified at all, because each subexpression of the form (*++p) has only one evaluation order, and each && is a sequence point. So the warning would be spurious in that case.
[Edit: between each sequence point, the two pointer increments have to occur before any reads that depend on the value of that pointer, so the result is unambigous even if there are multiple possible evaluation orders. Its possible the compiler does not track enough state or analyse that state in a nuanced enough way to detect this, though.]

However, if you allow the possibility of that bizarro aliasing, then the result of each comparison is now influenced by the order in which the increments and dereferences are evaluated. In which case the warning is legitimate, even though its about an aliasing situation which nobody would do deliberately.

Could you give a concrete example how your "aliasing" situation could look like in order to create different behaviour between left-to-right and right-to-left evaluation of the operands of == in the expression "*++scan == *++match", where scan and match are both pointers?

If I understand you correctly you say that something like

Code: Select all

scan = (unsigned char *) &match;

could create such a problem. But even in this case the modification '++scan' does not change the contents of the 'match' variable. This would be the case only when dealing with references instead of pointers, which is not given here.

But I think the point may be a different one which is only valid for C++, not for C: the compiler might assume that there may be some overloaded operator++ which could in theory have a (weird) side effect on the value of one of the two pointer variables.

If that were the reason then I would propose that all C/C++ compilers always issue one general warning:

Code: Select all

#00001: You are using the C/C++ language. Be aware that anything can go wrong.

Sven

[bob]

No, actually, I am now convinced that the result is not unspecified. Even though the arguments can be evaluated in any order for any fragment between sequence points in the statement, in this case it does not matter.

After all, if it performs the first assignment before the second or the second before the first (for instance) the result is the same, since the assignments are identical.

You finally got it. The result of the original expression is in general case defined but unspecified. However, since all the elements are identical it is also specified. Since, compiler cannot distinguish a special case from a general one, it issues the warning.
The warning is syntactically correct but semantically wrong.

If the language guaranteed that scan could not point into the 4 bytes occupied by the pointer variable match, and vice versa, then the original expression would not be unspecified at all, because each subexpression of the form (*++p) has only one evaluation order, and each && is a sequence point. So the warning would be spurious in that case.
[Edit: between each sequence point, the two pointer increments have to occur before any reads that depend on the value of that pointer, so the result is unambigous even if there are multiple possible evaluation orders. Its possible the compiler does not track enough state or analyse that state in a nuanced enough way to detect this, though.]

However, if you allow the possibility of that bizarro aliasing, then the result of each comparison is now influenced by the order in which the increments and dereferences are evaluated. In which case the warning is legitimate, even though its about an aliasing situation which nobody would do deliberately.

Were scan and match declared as char* pointers? If thats the case, it might be forcing the compiler to assume that they could alias anything. Even if the compiler is able to prove in this particular case that the bizarro aliasing is impossible, its under no obligation to do that, and issuing the warning is a reasonable thing for it to do.

If they are char* pointers and that is the source of the problem, then it means two things:
(1) the compiler is probably generating stupid code for it also, and
(2) changing them to unsigned char* might remove the bizarro potential aliasing, which would fix the warning and the bad codegen (but I'm not at all sure about this... try it and see.)

It is already unsigned char *scan, *match;

Edit: You could probably also replace it with this:

Code: Select all

do {
      } while(
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            (++scan, ++match, *scan == *match) && (++scan, ++match, *scan == *match) &&
            scan < strend); 

But I think your version is clearer.

Yes, your code works without warning.

I think that the compiler does not like "side effects" before and after the == in general, regardless of their correctness.

Miguel

One simple question... why not just write this as a simple loop and let the compiler "unroll" it into the above mess??? Would be easier to read, understand, and change.