Broken compilers, ignore or ???

[Martin J. Evans]

I recently received an rt (https://rt.cpan.org/Ticket/Display.html?id=79190) saying some code in DBD::ODBC was wrong and a patch to fix it. The code was:

while (*cp != '\0') {
*cp++ = toupper(*cp);
}

and the fix (which shouldn't be required as the above code is fine) was:

while (*cp != '\0') {
*cp = toupper(*cp);
cp++;
}

When I provided some test code and asked the reporter to tell me the compiler he reported a load of gcc versions which when using -O (above 0) produce incorrect results.

The test code and his results are below. I can obviously avoid this issue with a simple change to the code but how many other places might this occur? Do I ignore this (as the reporter is happy to use a newer working compiler) or do I somehow check for a broken compiler and abort the Makefile? Has anyone else come across something like this and what did they do?

#include <stdio.h>
#include <ctype.h>
#include <string.h>
main() {
char f[20];
char *cp;
strcpy(f, "abcdefg");
cp = f;
while(*cp != '\0') {
*cp++ = toupper(*cp);
}
printf("%s\n", f);
}

and his results:

Here are my results (gcc version, optimalization, program output):
3.2 -O0 ABCDEFG
3.2 -O1 BCDEFG
3.2 -O2 BCDEFG
3.2 -O3 BCDEFG
3.4 -O0 ABCDEFG
3.4 -O1 BCDEFG
3.4 -O2 BCDEFG
3.4 -O3 BCDEFG
4.1 -O0 ABCDEFG
4.1 -O1 ABCDEFG
4.1 -O2 ABCDEFG
4.1 -O3 ABCDEFG
4.7.1 -O0 ABCDEFG
4.7.1 -O1 ABCDEFG
4.7.1 -O2 ABCDEFG
4.7.1 -O3 ABCDEFG

Martin
--
Martin J. Evans
Easysoft Limited
http://www.easysoft.com

[Leon Timmermans]

On Tue, Sep 4, 2012 at 3:40 PM, Martin J. Evans
<martin...@easysoft.com> wrote:
> I recently received an rt (https://rt.cpan.org/Ticket/Display.html?id=79190)
> saying some code in DBD::ODBC was wrong and a patch to fix it. The code was:
>
> while (*cp != '\0') {
> *cp++ = toupper(*cp);
> }
>
> and the fix (which shouldn't be required as the above code is fine) was:
>
> while (*cp != '\0') {
> *cp = toupper(*cp);
> cp++;
> }

The order of execution is undefined if you read a variable and assign
to it in the same (sub)statement. The compiler is allowed to read the
*cp on the right both before the cp++, as the only sequence point is
the semicolon at the end.

The patch as offered does the correct and safe thing of splitting it
up in two statement.

> The test code and his results are below. I can obviously avoid this issue
> with a simple change to the code but how many other places might this occur?
> Do I ignore this (as the reporter is happy to use a newer working compiler)
> or do I somehow check for a broken compiler and abort the Makefile? Has
> anyone else come across something like this and what did they do?

You fix it. GCC will actually want you against this if you enable
-Wall/-Wsequence-point

Leon

[Shawn H Corey]

On Tue, 04 Sep 2012 14:40:01 +0100

"Martin J. Evans" <martin...@easysoft.com> wrote:

> Has anyone else come across something like this and what did they do?

Not with gcc but with an older cross-compiler. Since then, I have
always put increments and decrements in their own statements.


--
Just my 0.00000002 million dollars worth,
Shawn

Programming is as much about organization and communication
as it is about coding.

_Perl links_
official site : http://www.perl.org/
beginners' help : http://learn.perl.org/faq/beginners.html
advance help : http://perlmonks.org/
documentation : http://perldoc.perl.org/
news : http://perlsphere.net/
repository : http://www.cpan.org/
blog : http://blogs.perl.org/
regional groups : http://www.pm.org/

[Paul Johnson]

On Tue, Sep 04, 2012 at 02:40:01PM +0100, Martin J. Evans wrote:
> I recently received an rt (https://rt.cpan.org/Ticket/Display.html?id=79190) saying some code in DBD::ODBC was wrong and a patch to fix it. The code was:
>
> while (*cp != '\0') {
> *cp++ = toupper(*cp);
> }
>
> and the fix (which shouldn't be required as the above code is fine) was:

I don't think your code is fine. = isn't a sequence point.

> while (*cp != '\0') {
> *cp = toupper(*cp);
> cp++;
> }

And this seems to be a correct fix.

--
Paul Johnson - pa...@pjcj.net
http://www.pjcj.net

[Martin J. Evans]

On 04/09/12 14:50, Leon Timmermans wrote:
> On Tue, Sep 4, 2012 at 3:40 PM, Martin J. Evans
> <martin...@easysoft.com> wrote:
>> I recently received an rt (https://rt.cpan.org/Ticket/Display.html?id=79190)
>> saying some code in DBD::ODBC was wrong and a patch to fix it. The code was:
>>
>> while (*cp != '\0') {
>> *cp++ = toupper(*cp);
>> }
>>
>> and the fix (which shouldn't be required as the above code is fine) was:
>>
>> while (*cp != '\0') {
>> *cp = toupper(*cp);
>> cp++;
>> }
>
> The order of execution is undefined if you read a variable and assign
> to it in the same (sub)statement. The compiler is allowed to read the
> *cp on the right both before the cp++, as the only sequence point is
> the semicolon at the end.

Thanks, I did not know that.

> The patch as offered does the correct and safe thing of splitting it
> up in two statement.
>
>> The test code and his results are below. I can obviously avoid this issue
>> with a simple change to the code but how many other places might this occur?
>> Do I ignore this (as the reporter is happy to use a newer working compiler)
>> or do I somehow check for a broken compiler and abort the Makefile? Has
>> anyone else come across something like this and what did they do?
>
> You fix it. GCC will actually want you against this if you enable
> -Wall/-Wsequence-point

Actually, I had tried -Wall but the warning is not issued when -Oanything is also on the command line (and that happens by default here).

Thanks.

[Leon Timmermans]

On Tue, Sep 4, 2012 at 4:00 PM, Martin J. Evans
<martin...@easysoft.com> wrote:
> Thanks, I did not know that.

Klortho #11907 ;-)