Date: Wed, 29 May 1996 18:52:09 +0200 (MET DST) From: grog@lemis.de (Greg Lehey) To: scrappy@ki.net (Marc G. Fournier) Cc: hackers@freebsd.org (FreeBSD Hackers) Subject: Re: I HATE OPTIMISING COMPILERS Message-ID: <199605291652.SAA24396@allegro.lemis.de> In-Reply-To: <Pine.NEB.3.93.960529121137.7224C-100000@ki.net> from "Marc G. Fournier" at May 29, 96 12:13:13 pm
next in thread | previous in thread | raw e-mail | index | archive | help
Marc G. Fournier writes:
>
> On Wed, 29 May 1996, Greg Lehey wrote:
>> Gary Palmer writes:
>>>
>>> Well, I certainly do at the minute.
>>>
>>> Take the following innocuous piece of code:
>>>
>>> if (pdu->version == SNMP_DEFAULT_VERSION)
>>> pdu->version = session->version;
>>>
>>> if (pdu->version == SNMP_DEFAULT_VERSION){
>>> fprintf(stderr, "No version specified\n");
>>> snmp_errno = SNMPERR_BAD_ADDRESS;
>>> return 0;
>>>}
>>>
>>> (etc).
>>
>> Well, it looks like what you hate is a broken compiler. That's not
>> optimizing, that's just broken.
>>
>> How come you didn't use gdb to follow up the problem?
>>
> How do you follow up a problem like this with gdb? I hadn't
> even thought about an optimizing problem when I started to try to
> debug this with gdb, but I couldn't find a thing that seemed to
> indicate whree the problem lay ...
Basically you use the debugger's commands to do your printfs for you.
You can single step through the code (that's what I'd do when it's
boiled down to such a small fragement), or you can set breakpoints.
You can issue explicit print commands, or you can use the display
command to show it to you every time you stop.
Let's look at Gary's original message. To make it easier to
visualize, I packed this in a minimal shell:
> #include <stdio.h>
> #define SNMP_DEFAULT_VERSION -1
> #define SNMP_FOO_VERSION 2
> #define SNMPERR_BAD_ADDRESS 0xdeadbeef
>
> struct _pdu
> {
> int version;
> char foo;
>}
> pdu;
> struct _pdu session;
>
> int snmp_errno;
>
> foo (struct _pdu *pdu, struct _pdu *session)
> {
> if (pdu->version == SNMP_DEFAULT_VERSION)
> pdu->version = session->version;
>
> if (pdu->version == SNMP_DEFAULT_VERSION)
> {
> fprintf(stderr, "No version specified\n");
> snmp_errno = SNMPERR_BAD_ADDRESS;
> return 0;
>}
>}
>
> main ()
> {
> pdu.version = SNMP_DEFAULT_VERSION;
> session.version = SNMP_FOO_VERSION;
> foo (&pdu, &session);
>}
Back to Gary's message:
> So I go and stick in a nice printf just above the first if statement
> to check that it's not doing something funny. The code now becomes:
>
> printf("pdu->version = %d\nsession->version = %d\nSNMP_DEFAULT_VERSION = %d\n",
> pdu->version, session->version, SNMP_DEFAULT_VERSION);
>
> if (pdu->version == SNMP_DEFAULT_VERSION)
> pdu->version = session->version;
>
> if (pdu->version == SNMP_DEFAULT_VERSION){
> fprintf(stderr, "No version specified\n");
> snmp_errno = SNMPERR_BAD_ADDRESS;
> return 0;
>}
>
> Which produces:
>
> pdu->version = -1
> session->version = 2
> SNMP_DEFAULT_VERSION = -1
> No version specified
OK, here you'd set a breakpoint on the first line of the original
example (the first 'if (pdu->version == SNMP_DEFAULT_VERSION)'):
+ === grog@freebie (/dev/ttyp4) ~ 16 -> gdb junk
+ GDB is free software and you are welcome to distribute copies of it
+ under certain conditions; type "show copying" to see the conditions.
+ There is absolutely no warranty for GDB; type "show warranty" for details.
+ GDB 4.13 (i386-unknown-freebsd), Copyright 1994 Free Software Foundation, Inc...
+ (gdb) b foo
+ Breakpoint 1 at 0x159b: file junk.c, line 18.
+ (gdb) r
+ Starting program: /usr/home/grog/junk
+ During symbol reading...unknown symbol type 0x1e...
+
+ Breakpoint 1, foo (pdu=0x21e4, session=0x21d8) at junk.c:18
+ 18 if (pdu->version == SNMP_DEFAULT_VERSION)
+ (gdb) r
+ Starting program: /usr/home/grog/junk
+ During symbol reading...unknown symbol type 0x1e...
+
+ Breakpoint 1, foo (pdu=0x21e4, session=0x21d8) at junk.c:18
+ 18 if (pdu->version == SNMP_DEFAULT_VERSION)
+ (gdb) p pdu->version
+ $1 = -1
+ (gdb) p SNMP_DEFAULT_VERSION
+ No symbol "SNMP_DEFAULT_VERSION" in current context.
Lose, lose. You lose preprocessor variables in the preprocessor. To
keep them into the debugger, use enums instead.
+ (gdb) p session->version
+ $2 = 2
+ (gdb)
OK, so far we've seen the same stuff as in Gary's first example. I
can't make this fail here, so we don't get the same final result.
Let's look at the second time:
> So, okay, session->version != SNMP_DEFAULT_VERSION, but the problem is
> the aggregate of those two if statements disagrees. Hmm. Okay. Put in
> a second printf:
>
> printf("pdu->version = %d\nsession->version = %d\nSNMP_DEFAULT_VERSION = %d\n",
> pdu->version, session->version, SNMP_DEFAULT_VERSION);
>
> if (pdu->version == SNMP_DEFAULT_VERSION)
> pdu->version = session->version;
>
> printf("pdu->version = %d\nsession->version = %d\nSNMP_DEFAULT_VERSION = %d\n",
> pdu->version, session->version, SNMP_DEFAULT_VERSION);
>
> if (pdu->version == SNMP_DEFAULT_VERSION){
> fprintf(stderr, "No version specified\n");
> snmp_errno = SNMPERR_BAD_ADDRESS;
> return 0;
>}
At this point, you just need to set a second breakpoint. on the second
if statement. Since in this example you've finished, we need to start
again:
+
+ (gdb) l 16
I wish gdb would do what I say, and not what it thinks I mean...
+ 11 pdu;
+ 12 struct _pdu session;
+ 13
+ 14 int snmp_errno;
+ 15
+ 16 foo (struct _pdu *pdu, struct _pdu *session)
+ 17 {
+ 18 if (pdu->version == SNMP_DEFAULT_VERSION)
+ 19 pdu->version = session->version;
+ 20
+ 21 if (pdu->version == SNMP_DEFAULT_VERSION)
+ 22 {
+ 23 fprintf(stderr, "No version specified\n");
+ 24 snmp_errno = SNMPERR_BAD_ADDRESS;
+ 25 return 0;
+ 26 }
+ 27 }
+ (gdb) b 21
+ Breakpoint 2 at 0x15ad: file junk.c, line 21.
+ (gdb) r
+ Starting program: /usr/home/grog/junk
+
+ Breakpoint 1, foo (pdu=0x21e4, session=0x21d8) at junk.c:18
+ 18 if (pdu->version == SNMP_DEFAULT_VERSION)
As before, just for the fun of it
+ (gdb) p session->version
+ $1 = 2
+ (gdb) p pdu->version
+ $2 = -1
+ (gdb) c
+ Continuing.
+
+ Breakpoint 2, foo (pdu=0x21e4, session=0x21d8) at junk.c:21
+ 21 if (pdu->version == SNMP_DEFAULT_VERSION)
+ (gdb) p pdu->version
+ $3 = 2
At this point, of course, our mileage varies. Here, you would get the
result:
+ (gdb) p pdu->version
+ $3 = -1
Gary continues:
> And all of a sudden IT STARTS WORKING PROPERLY. Take out the two
> printfs, a nasty idea forming in my mind. The code now becomes:
>
> if (pdu->version == SNMP_DEFAULT_VERSION)
> pdu->version = session->version;
>
> session->version = pdu->version;
>
> if (pdu->version == SNMP_DEFAULT_VERSION){
> fprintf(stderr, "No version specified\n");
> snmp_errno = SNMPERR_BAD_ADDRESS;
> return 0;
>}
>
> Which looks kinda daft, but again, the code worked as expected. Take
> out the ``session->version = pdu->version;'' statement, and it starts
> failing again. Now I'm really suspicious. Go to the Makefile. Change
> ``CFLAGS=-O -g -Dfreebsd2'' to be just ``CFLAGS=-Dfreebsd2''. make
> clean all. Go and rebuild the client apps that depend on the
> library.
>
> BINGO. Much cheering and celebrating. I go to the fridge and get
> another coke.
>
> Anyone want to help me take GCC out the back kicking and screaming to
> face the shooting squad? Admittedly that code isn't the most elegant
> way of doing that assignment/test, but GCC should NOT produce such
> bogus code from it :-( And here we (or at least I) thought that `-O'
> was safe to use :-(
I'd do this differently in gdb. Personally, I'd look at the code that
was generated. Your code won't look like this, of course, because
this code works. But you can see a few things:
+ Breakpoint 1, foo (pdu=0x21e4, session=0x21d8) at junk.c:18
+ 18 if (pdu->version == SNMP_DEFAULT_VERSION)
+ (gdb) x/20i $eip
+ 0x159b <foo+3>: movl 0x8(%ebp),%eax get foo->version
+ 0x159e <foo+6>: cmpl $0xffffffff,(%eax) compare with -1 (SNMP_DEFAULT_VERSION)
+ 0x15a1 <foo+9>: jne 0x15ad <foo+21> on if not equal
+ 0x15a3 <foo+11>: movl 0x8(%ebp),%eax get foo->version again (you can tell
that this wasn't optimized :-)
+ 0x15a6 <foo+14>: movl 0xc(%ebp),%edx get the address of session in edx
+ 0x15a9 <foo+17>: movl (%edx),%ecx get the first 4 bytes of session in ecx
+ 0x15ab <foo+19>: movl %ecx,(%eax) and store them in the first 4 bytes of pdu
+ 0x15ad <foo+21>: movl 0x8(%ebp),%eax (2nd if statement)
+ 0x15b0 <foo+24>: cmpl $0xffffffff,(%eax)
+ 0x15b3 <foo+27>: jne 0x15d8 <foo+64>
+ 0x15b5 <foo+29>: pushl $0x1580
+ 0x15ba <foo+34>: pushl $0x2180
+ 0x15bf <foo+39>: call 0x207c <_DYNAMIC+124>
+ 0x15c4 <foo+44>: addl $0x8,%esp
+ 0x15c7 <foo+47>: movl $0xdeadbeef,0x20cc
+ 0x15d1 <foo+57>: xorl %eax,%eax
+ 0x15d3 <foo+59>: jmp 0x15d8 <foo+64>
Reading assembler is tough, of course, but there are other ways. In
any case, you save a lot by doing it this way rather than going and
editing and recompiling possibly dozens of times.
Greg
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199605291652.SAA24396>