gcc 4.5 and copy_flag in libubigen.c

[Ricard Wanderlof]

On Mon, 14 Mar 2011, Andre Naujoks wrote:

> I am compiling a board support package with mtd-utils-1.3.1.
>
> After a gcc update to version 4.5 on my host the build fails and it
> tells me that:
>
> ./src/libubigen.c: In function 'ubigen_write_leb':
> ./src/libubigen.c:204:19: error: operation on 'u->v->copy_flag' may be
> undefined.
>
> This seems like a recently introduced warning by the new gcc version,
> but because of the -Werror halts my build.

> // This is the part gcc 4.5 complains about
> // ---------------------
>
>        if (action & MARK_AS_UPDATE) {
>                u->v->copy_flag = (u->v->copy_flag)++;
>        }
>
> // --------------------
>

I can understand why gcc warns about this, the operation is undefined. I 
would think the problem is not with the pointer dereference, it is the way 
the copy flag is used both before and after the ++. What you have is

   foo = foo++;

which is an undefined operation, for the following reason. While

   foo++;

says that foo has one value before this operation and the value +1 after 
(the C standard talks about 'sequence points', i.e points in the code 
where all variables have well defined values),

   foo = foo++;

is ambiguous in that it is not clear whether the ++ takes place after
the assignment or vice versa. Should the compiler bump the value of foo, 
but return the old value, assign it to foo, and then assign foo the 
incremented value, or increment the value, return the old value, and 
assign it to foo, in effect not changing the value?

It might have worked by chance on an older compiler that didn't notice the 
problem and consistently picked one of the two possibilities above 
(preferebly the first one); however, according to the C standard, once an 
operation becomes undefined all aspects of it become undefined so the 
compiler could technically stuff any value in foo (or none at all).

Anyhow, technicalities aside, it looks like someone made a mental error 
when simply trying to set the copy_flag. I'm fairly convinced that the 
author simply meant:

   (u->v->copy_flag)++;

> My question is this. If this is indeed just a flag, can I just set
> copy_flag to 1 instead of incrementing it? Witout breaking anything?

It is (or perhaps was?) commonplace to set flags by incrementing them. I'm 
not sure why this is considered a good idea. Possibly because

   flag++;

looks more elegant and cool than

   flag = 1;

although the latter is most certainly faster; in the first case the value 
must be read, incremented and copied back to the variable. Of course, if 
the variable is in a register there's no real overhead, but most 
processors have fast ways of loading small immediate values so the latter 
version will probably be just as quick and not need more code either.

I don't know if it could have something to do with the machines on which 
Unix was originally written, that it was faster, or, more likely, used up 
less code space, to increment something than to load it with 1.

I would agree with you in principle, especially since the variable is 
called 'flag' something, then again, without a thorough understanding of 
what the flag is used for, someone could conceivably have decided to use 
it as a counter instead of a flag, but retain the name.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30