gcc 4.5 and copy_flag in libubigen.c

Hi all.

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.

I had a look into the source at ubi-utils/old-utils/src/libubigen.c and it says:

int
ubigen_write_leb(ubi_info_t u, ubigen_action_t action)

{
        int rc = 0;
        size_t read = 0;

        clear_buf(u);
        write_ec_hdr(u);

        rc = fill_data_buffer_from_file(u, &read);
        if (rc != 0)
                return rc;

        if (u->v->vol_type == UBI_VID_STATIC)  {
                add_static_info(u, read, action);
        }

        u->v->lnum = cpu_to_be32(u->blks_written);

// This is the part gcc 4.5 complains about
// ---------------------

        if (action & MARK_AS_UPDATE) {
                u->v->copy_flag = (u->v->copy_flag)++;
        }

// --------------------

        write_vid_hdr(u, action);
        rc = write_to_output_stream(u);
        if (rc != 0)
                return rc;

        /* Update current handle */
        u->bytes_read += read;
        u->blks_written++;
        return 0;
}

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?

I found no other references to the copy_flag ecxept in this file and in the struct definition in include/mtd/ubi-media.h. So the warning may be right, but I really have no idea about the whole ubifs thing.

I think the -Werror in the build is intentional, so I would rather not disable it without knowing that this is not a bug or anything.

I would really be grateful for any advice on this.

Regards and thanks in advance.
Andre

Why don't you update your mtd-utils version ?

Matthieu

Am 14.03.2011 18:53, schrieb Matthieu CASTET:

Because it would be more of a hassle and a risk to do that. There are patches applied that were build from the suppliers of the BSP, which I would have to possibly reproduce for a newer version. As far as I can see, they are all patches to the build system, but nevertheless.

The current version is the version I would stick to just because it takes the maintenance of the package of of me. I would rather add a little patch myself instead of handling the whole package.

I additionally have no idea about the implications an update of this particular package would have. It just broke, because the gcc people decided a new warning was a good idea.

Andre

Well, you can use 1.3.1, but do not use old UBI utils, use the new ones, i.e., use ubi-utils/src/libubigen.c, not ubi-utils/old-utils/src/libubigen.c

Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Am 15.03.2011 09:06, schrieb Artem Bityutskiy:

Thank You. That helped a lot. I just removed SUBDIRS from the Makefile in ubi-utils. It seems as if those were not used at all from the beginning.

Sorry for the noise for something that old I did not even use :-)

Thanks for the help
Andre

You are using old UBI utils, they are not supported for long time. They were removed from the mtd-utils tree recently. Try to use the new UBI utilities.

Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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)++;

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

Am 15.03.2011 09:45, schrieb Ricard Wanderlof:

Thanks for the detailed answer. At least I learned something today :-). Since this code is considered old and no longer used I was able to just disable this part in the build without consequences (AFAICS).

Thanks a lot anyway. I really appreciate it.

Andre