gcc: Operation on 'xxx' may be undefined

English Programming/Scripting
#1

This is a classic situation (example taken from Michel Xhaards jpegenc library):

  for (i = rows; i > 0; i--)
    {
      for (j = cols; j > 0; j--)
        *Y1_Ptr++ = *input_ptr++ - 128;
      for (j = 8 - cols; j > 0; j--)
        *Y1_Ptr++ = *(Y1_Ptr - 1);
      input_ptr += incr;
    }

Look at line 6 of this code snippet. It throws a warning like this:

encoder.c:392: warning: operation on 'Y1_Ptr' may be undefined

The warning is justified because things can go wrong. It is entirely up to the compiler what the value of Y1_Ptr would be in the right part of the line. It is undefined according to the standard. Only the original coder knows what the code should do in his opinion.

Code like this can not be packaged. I have to patch it. Would you think that this fix meets the original intentions:

*Y1_Ptr = *(Y1_Ptr - 1); Y1_Ptr++;

Any comments would be helpful.

#2

Would you mind telling what language this is about. Preferable in the thread title. This will attrackt the gurus of that language (better for you) and in the same time not waste the time of those having no aqaintance with that particualar language.

#3

Would you mind telling what language this is about.

I forgot the obvious: It is C, compiled with gcc.

#4

Shalll I change the title to:
Operation on ‘xxx’ may be undefined in C

#5

Shall I change the title to:
Operation on ‘xxx’ may be undefined in C

Yes, please; or perhaps make it:

gcc: Operation on ‘xxx’ may be undefined

as this is a typical gcc warning. I don’t know what other compilers would do.

#6

vodoo wrote:

> Code like this can not be packaged. I have to patch it. Would you think
> that this fix meets the original intentions:
>
>
> Code:
> --------------------
> *Y1_Ptr = *(Y1_Ptr - 1); Y1_Ptr++;
> --------------------
>
Maybe I am blind but how does this fix it?

If Y1_Ptr is undefined (the situation the warning is about) then

Y1_Ptr - 1 is also undefined

#7

Done …

#8

It’s not the kind of ‘undefined’ you have in mind in the sense of not initialized. Look at line 4 of the code in post#1.

“Undefined” here means that the compiler may (or may not – this is undefined) increment Y1_Ptr before evaluation of the right side of the assignment.

My fix avoids the compiler warning and forces a defined behaviour. The first statement is evaluated and then, in the second statement the pointer is increased. If the compiler did exactly this with the flaky code, then my fix is valid. Otherwise not.

The reason why I’m insisting here is that I am building a package and the code is for a type of webcam (gspcav1) which I don’t own. So I can’t test it and would like to stay on the safe side, to avoid injecting broken packages into the community.

#9

Done …

@hcvv: thank you; looks much better now.

#10

Sorry I misread your post and the exact warning.

It is C not C++ so I am the wrong person to comment on it at all (otherwise
my opinion would be not to use unsafe pointer arithmetic at all but safe
iterators). I think the last time I used pointer arithmetic was about 14-15
years ago.

Why not ask the original author of the code, is he/she no longer available.
Because then the issue could be solved upstream by the author and is
available to everyone using it.

#11

Why not simply

tmp = Y1_Ptr;
*Y1_Ptr++ = *(tmp - 1);

to avoid the warning?

#12

Why not simply

tmp = Y1_Ptr;
*Y1_Ptr++ = *(tmp - 1);

to avoid the warning?

As far as I can see this is equivalent to my proposed fix but needs more registers and CPU cycles (I am still thinking in terms of the old Z80 CPU when writing C code rotfl!).

Anyway, I asked upstream and found a dev who made a fork. He made the same corrections as I did. So far this seems to be ok. The question remains if this slows things down. We are processing image data here in real time and I guess the original coder wanted to write it as compressed as possible at a time when compilers did not yet spit out warnings at this style of code.

(Upstream: SourceForge.net: MJPG-streamer: Topic: gcc warnings for input_gspcav1/encoder.c)

#13

The problem with the original code is that it produces indeterminate results, depending on what instructions the compiler generates. The C standard does not require that the pointer increment happen after the pointer dereference. Given the choice, it’s better to have slower code that works than fast code that is wrong.

You should be able to rewrite this as two steps as you did without loss of efficiency.

#14

Thank you ken_yap. In my secret thoughts I was hoping to see something like your statement.

#15

The argument about performance is of course valid (but you only posted a
very short code snippet and I did not look up the rest of all the code).

As long as no severe performance loss is involved I personally prefer
readability over premature optimization (please note: I do NOT say that what
you did is premature optimization).

Keep in mind I am not a C programmer.

My personal experience is (was) that these kind of tricks do often enough
not improve performance in a significant way, but if it is a deeply nested
loop it can be of course a bottleneck. (My experience is from programming
nonlinear image filters in C++, so it may be probably completely worthless
for a real time video streaming software)