gcc 4.5 and copy_flag in libubigen.c

gcc 4.5 and copy_flag in libubigen.c

[Andre Naujoks]

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

Re: gcc 4.5 and copy_flag in libubigen.c

[Matthieu CASTET]

Andre Naujoks a écrit :
> Hi all.
> 
> I am compiling a board support package with mtd-utils-1.3.1.
> 
Why don't you update your mtd-utils version ?


Matthieu

Re: gcc 4.5 and copy_flag in libubigen.c

[Andre Naujoks]

Am 14.03.2011 18:53, schrieb Matthieu CASTET:
> Andre Naujoks a écrit :
>> Hi all.
>>
>> I am compiling a board support package with mtd-utils-1.3.1.
>>
> Why don't you update your mtd-utils version ?

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

> 
> 
> Matthieu

Re: gcc 4.5 and copy_flag in libubigen.c

[Artem Bityutskiy]

On Tue, 2011-03-15 at 08:55 +0100, Andre Naujoks wrote:
> Am 14.03.2011 18:53, schrieb Matthieu CASTET:
> > Andre Naujoks a écrit :
> >> Hi all.
> >>
> >> I am compiling a board support package with mtd-utils-1.3.1.
> >>
> > Why don't you update your mtd-utils version ?
> 
> 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.

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 (Артём Битюцкий)

Re: gcc 4.5 and copy_flag in libubigen.c

[Andre Naujoks]

Am 15.03.2011 09:06, schrieb Artem Bityutskiy:
> On Tue, 2011-03-15 at 08:55 +0100, Andre Naujoks wrote:
>> Am 14.03.2011 18:53, schrieb Matthieu CASTET:
>>> Andre Naujoks a écrit :
>>>> Hi all.
>>>>
>>>> I am compiling a board support package with mtd-utils-1.3.1.
>>>>
>>> Why don't you update your mtd-utils version ?
>>
>> 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.
> 
> 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
> 

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

Re: gcc 4.5 and copy_flag in libubigen.c

[Artem Bityutskiy]

On Mon, 2011-03-14 at 17:37 +0100, Andre Naujoks wrote:
> 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:

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 (Артём Битюцкий)

Re: 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

Re: gcc 4.5 and copy_flag in libubigen.c

[Andre Naujoks]

Am 15.03.2011 09:45, schrieb 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

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