From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Larmour To: gdb-patches@sourceware.cygnus.com Cc: Alex Schuilenburg Subject: ser-unix.c:do_unix_readchar() Don't timeout when told not to Date: Sun, 19 Mar 2000 23:42:00 -0000 Message-ID: <38D5D64F.CB756366@redhat.co.uk> X-SW-Source: 2000-03/msg00364.html Message-ID: <20000319234200.VO-mBG-Bxz7l8zvF_1Dv4Mu4Hxu4GtGFVoftetJVJTI@z> A while back, folks in the eCos team reported that you could get "Watchdog has expired. Target detached." messages even when the watchdog wasn't set. The GDB team reacted in disbelief :-). After getting irritated by it this evening, I looked for and found the problem: do_unix_readchar() could still return SERIAL_TIMEOUT, even when the requested timeout was -1 (i.e. infinite). The attached patch fixes the problem, and I used the opportunity to make the code slightly more robust, and more similar to e.g. do_hardwire_readchar in the same file. I don't have write permission, so please check this in. Thanks, Jifl 2000-03-20 Jonathan Larmour * ser-unix.c (do_unix_readchar): Reorganise to be more robust, particularly ensuring it can't return SERIAL_TIMEOUT when told not to time out. -- Red Hat, 35 Cambridge Place, Cambridge, UK. CB2 1NS Tel: +44 (1223) 728762 "Plan to be spontaneous tomorrow." || These opinions are all my own fault Index: ser-unix.c =================================================================== RCS file: /cvs/src/src/gdb/ser-unix.c,v retrieving revision 1.2 diff -u -5 -p -c -r1.2 ser-unix.c *** ser-unix.c 2000/02/09 08:52:47 1.2 --- ser-unix.c 2000/03/20 07:30:48 *************** do_unix_readchar (serial_t scb, int time *** 908,920 **** /* We have to be able to keep the GUI alive here, so we break the original timeout into steps of 1 second, running the "keep the GUI alive" hook each time through the loop. Also, timeout = 0 means to poll, so we just set the delta to 0, so we ! will only go through the loop once. */ ! delta = (timeout == 0 ? 0 : 1); while (1) { /* N.B. The UI may destroy our world (for instance by calling remote_stop,) in which case we want to get out of here as --- 908,920 ---- /* We have to be able to keep the GUI alive here, so we break the original timeout into steps of 1 second, running the "keep the GUI alive" hook each time through the loop. Also, timeout = 0 means to poll, so we just set the delta to 0, so we ! will only go through the loop once. timeout < 0 means to wait forever. */ ! delta = (timeout <= 0 ? 0 : 1); while (1) { /* N.B. The UI may destroy our world (for instance by calling remote_stop,) in which case we want to get out of here as *************** do_unix_readchar (serial_t scb, int time *** 926,980 **** { if (ui_loop_hook (0)) return SERIAL_TIMEOUT; } ! status = ser_unix_wait_for (scb, delta); timeout -= delta; ! /* If we got a character or an error back from wait_for, then we can ! break from the loop before the timeout is completed. */ ! if (status != SERIAL_TIMEOUT) ! { ! break; ! } ! /* If we have exhausted the original timeout, then generate ! a SERIAL_TIMEOUT, and pass it out of the loop. */ ! else if (timeout == 0) ! { ! status = SERIAL_TIMEOUT; ! break; } - } - - if (status < 0) - return status; ! while (1) ! { ! status = read (scb->fd, scb->buf, BUFSIZ); ! if (status != -1 || errno != EINTR) ! break; ! } ! ! if (status <= 0) ! { ! if (status == 0) ! return SERIAL_TIMEOUT; /* 0 chars means timeout [may need to ! distinguish between EOF & timeouts ! someday] */ ! else ! return SERIAL_ERROR; /* Got an error from read */ } - - scb->bufcnt = status; - scb->bufcnt--; - scb->bufp = scb->buf; - return *scb->bufp++; } /* Perform operations common to both old and new readchar. */ /* Return the next character from the input FIFO. If the FIFO is --- 926,967 ---- { if (ui_loop_hook (0)) return SERIAL_TIMEOUT; } ! status = ser_unix_wait_for (scb, timeout < 0 ? timeout : delta); timeout -= delta; ! /* If we got an error back from wait_for, then we can return */ ! if (status == SERIAL_ERROR) ! return status; ! status = read (scb->fd, scb->buf, BUFSIZ); ! if (status <= 0) ! { ! if (status == 0) ! { ! if (timeout != 0) ! continue; ! else ! return SERIAL_TIMEOUT; /* 0 chars means timeout [may need to ! distinguish between EOF & timeouts ! someday] */ ! } ! else if (errno == EINTR) ! continue; ! else ! return SERIAL_ERROR; /* Got an error from read */ } ! scb->bufcnt = status; ! scb->bufcnt--; ! scb->bufp = scb->buf; ! return *scb->bufp++; } } /* Perform operations common to both old and new readchar. */ /* Return the next character from the input FIFO. If the FIFO is >From ac131313@cygnus.com Mon Mar 20 00:20:00 2000 From: Andrew Cagney To: Philippe GIACINTI Cc: gdb-patches@sourceware.cygnus.com Subject: Re: [PATCH] update: minor fixes in remote.c Date: Mon, 20 Mar 2000 00:20:00 -0000 Message-id: <38D5DEF3.B5482CF4@cygnus.com> References: <14549.54618.39052.823724@gargle.gargle.HOWL> X-SW-Source: 2000-03/msg00365.html Content-length: 1458 Philippe GIACINTI wrote: > @@ -3439,7 +3439,8 @@ > int todo; > int i; > > - todo = min (len, max_buf_size / 2); /* num bytes that will fit */ > + todo = min (len, max_buf_size / 2 - 2); /* num bytes that will fit */ > + /* -2 for leading $ and trailing # + checksum (2 hex chars) */ > > /* construct "m"","" */ > /* sprintf (buf, "m%lx,%x", (unsigned long) memaddr, todo); */ > > If the host side needs to read for example 500 bytes and assuming that the > maximum buffer size is 400 chars, then without the fix, the remote side will > be asked to send min (500, 400/2)=200 bytes, its answer will then be: > $ ... 200 hex encoded bytes = 400 chars #CS = 400 + 4 chars which is greater > than the max buffer size and will lead to an error from the host side. > So we need to subtract 4 chars (i.e.: 2 hex encoded bytes) from requested > number of bytes. I'm still not sure that there is a problem here (I'm not saying that there isn't though). Several changes late last year and the testsuite addition remote.exp addressed/tested many of the buffer overflow problems that were lingering in that code. Do you have a debug session or test case that demonstrates the problem? One thing to keep in mind is that, when receiving packets, the ``$'' and ``#NN'' are not included in the size of incomming packets as the remote code does not use the packet buffer for those characters. Andrew >From ac131313@cygnus.com Mon Mar 20 01:03:00 2000 From: Andrew Cagney To: Tom Tromey Cc: gdb-patches@sourceware.cygnus.com Subject: Re: Patch: make "!" a command unconditionally Date: Mon, 20 Mar 2000 01:03:00 -0000 Message-id: <38D5E957.7604102@cygnus.com> References: <87itz9cabi.fsf@cygnus.com> <38BB5D9F.1A9BF1B3@cygnus.com> <200002290701.XAA15591@ferrule.cygnus.com> X-SW-Source: 2000-03/msg00366.html Content-length: 2223 Tom Tromey wrote: > > >>>>> "Andrew" == Andrew Cagney writes: > > Andrew> From memory this has been suggested before. The problem pointed out > Andrew> last time was that you need to specify: > > Andrew> ``!'' > > I suggested it last time but didn't supply a patch. > I agree it would be nice to eliminate the space. > That's probably more than a 20 second hack though :-( > > Andrew> I think there also may have been a debate over ``!'' as a > Andrew> shell escape vs ``!'' for history. (personally it didn't > Andrew> worry me). Fernando? > > This doesn't bother me either. For these kinds of tools "!" is more > typically a shell escape. FYI, I've checked in the attatched. > Speaking of history, I wish gdb would pick up bash's "C-o" > (operate-and-get-next) binding. Hmm, operate-and-get-next (C-o) Accept the current line for execution and fetch the next line relative to the current line from the history for editing. Any argument is ignored. learn something new every day. Probably involves tweeks to the readline interface. Andrew Mon Mar 20 19:58:45 2000 Andrew Cagney * command.c (_initialize_command): Document requirements for ``!'' command. Index: command.c =================================================================== RCS file: /cvs/src/src/gdb/command.c,v retrieving revision 1.2 diff -p -r1.2 command.c *** command.c 2000/02/09 08:52:45 1.2 --- command.c 2000/03/20 08:59:25 *************** _initialize_command () *** 1678,1683 **** --- 1678,1689 ---- "Execute the rest of the line as a shell command. \n\ With no arguments, run an inferior shell."); + /* NOTE: cagney/2000-03-20: Being able to enter ``(gdb) !ls'' would + be a really useful feature. Unfortunatly, the below wont do + this. Instead it adds support for the form ``(gdb) ! ls'' + (i.e. the space is required). If the ``!'' command below is + added the complains about no ``!'' command would be replaced by + complains about how the ``!'' command is broken :-) */ if (xdb_commands) add_com_alias ("!", "shell", class_support, 0); >From Philippe.Giacinti@sxb.bsf.alcatel.fr Mon Mar 20 01:10:00 2000 From: Philippe GIACINTI To: Andrew Cagney Cc: Philippe GIACINTI , gdb-patches@sourceware.cygnus.com Subject: Re: [PATCH] update: minor fixes in remote.c Date: Mon, 20 Mar 2000 01:10:00 -0000 Message-id: <14549.60108.37967.719735@gargle.gargle.HOWL> References: <14549.54618.39052.823724@gargle.gargle.HOWL> <38D5DEF3.B5482CF4@cygnus.com> X-SW-Source: 2000-03/msg00367.html Content-length: 2140 Andrew Cagney wrote: > Philippe GIACINTI wrote: > > > @@ -3439,7 +3439,8 @@ > > int todo; > > int i; > > > > - todo = min (len, max_buf_size / 2); /* num bytes that will fit */ > > + todo = min (len, max_buf_size / 2 - 2); /* num bytes that will fit */ > > + /* -2 for leading $ and trailing # + checksum (2 hex chars) */ > > > > /* construct "m"","" */ > > /* sprintf (buf, "m%lx,%x", (unsigned long) memaddr, todo); */ > > > > If the host side needs to read for example 500 bytes and assuming that the > > maximum buffer size is 400 chars, then without the fix, the remote side will > > be asked to send min (500, 400/2)=200 bytes, its answer will then be: > > $ ... 200 hex encoded bytes = 400 chars #CS = 400 + 4 chars which is greater > > than the max buffer size and will lead to an error from the host side. > > So we need to subtract 4 chars (i.e.: 2 hex encoded bytes) from requested > > number of bytes. > > I'm still not sure that there is a problem here (I'm not saying that > there isn't though). Several changes late last year and the testsuite > addition remote.exp addressed/tested many of the buffer overflow > problems that were lingering in that code. > > Do you have a debug session or test case that demonstrates the problem? > > One thing to keep in mind is that, when receiving packets, the ``$'' and > ``#NN'' are not included in the size of incomming packets as the remote > code does not use the packet buffer for those characters. > > Andrew > OK, you're right. I've checked with the last snapshot, and the problem doesn't happen anymore. I had this problem with GDB-4.18 (see attached log) and fixed it with the previous patch. Sorry for the inconvenience ... Anyway, the first part of the patch (using set_thread) should be OK. Regards. -- Philippe Giacinti - Alcatel Business Systems - PCD/SMB/SWPCC - CC S0645 1, route du Dr Albert Schweitzer - 67408 Illkirch Cedex - FRANCE Tel: +33 (0)3.90.67.62.10 - Fax: +33 (0)3.90.67.77.93 mailto:Philippe.Giacinti@sxb.bsf.alcatel.fr