From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15143 invoked by alias); 26 Feb 2014 02:14:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 15127 invoked by uid 89); 26 Feb 2014 02:14:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 26 Feb 2014 02:14:50 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 28C4F11655B; Tue, 25 Feb 2014 21:14:48 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id viNQDNQhW9Us; Tue, 25 Feb 2014 21:14:48 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id EF3CE116402; Tue, 25 Feb 2014 21:14:47 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 516ABE0EFC; Tue, 25 Feb 2014 18:14:46 -0800 (PST) Date: Wed, 26 Feb 2014 02:14:00 -0000 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [commit] Adjust ia64_linux_xfer_partial following to_xfer_partial API change. Message-ID: <20140226021446.GA4348@adacore.com> References: <1393345450-20878-1-git-send-email-brobecker@adacore.com> <530CD37F.3050106@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8t9RHnE3ZwKMSgU+" Content-Disposition: inline In-Reply-To: <530CD37F.3050106@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-02/txt/msg00770.txt.bz2 --8t9RHnE3ZwKMSgU+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 3382 > > + gdb_byte *tmp_buf = alloca (offset + len); > > + ULONGEST xfered; > > + > > + xfered = syscall (__NR_getunwind, readbuf, offset + len); > > It seems this should have passed tmp_buf instead of readbuf? Yes, indeed. Hasty implementation which, as you found out, gets luck (at best). > Also, I don't think the "offset + len" size is the > right size to pass down to the syscall. > > From: > > http://lxr.free-electrons.com/source/arch/ia64/kernel/unwind.c#L2313 I thought it was OK. Basically, we were asked to get len bytes at offset. The idea was that, since the syscall doesn't handle offsets, we have to fetch from zero again, which means getting offset bytes (which were going to be thrown away) + len bytes (which we'll return). > So it looks like this code is getting lucky and the caller > is requesting enough bytes: > > x = target_read_alloc (¤t_target, TARGET_OBJECT_UNWIND_TABLE, > NULL, buf_p); Right. I noticed that also, because I couldn't explain at the beginning why the syscall was only performed with offset == 0 in the previous implementation. > But to_xfer_partial implementations should not assume that. E.g., > change target_read_alloc_1 to start by requesting a byte at a time > instead of 4096, and this will fail. > > > + if (xfered <= 0) > > Note that because xfered is ULONGEST, this won't actually > detect errors. Indeed. I had assumed that a return value of 0 indicated an error already, and wrote it that way; and then I thought, hey, negative values should never happen either, right? Let's handle that too. Duh... > > + else if (xfered <= offset) > > + return TARGET_XFER_EOF; > > Not sure I follow this one. The theory was the following: 1. First xfer request, ask ~4k at offset zero, return xxx bytes and OK 2. The caller finds an OK, but only xxx bytes < 4k. So round #2, asks for 4k - xxx bytes at offset xxx. We fetch unwind table again, found that xferd is xxx again. Since offset is xxx, it means we're done. Return EOF. > I suggest something along the lines of the below instead. > Should handle partial requests, and clearer to read, I think. > Completely untested, though. It does look better indeed. A little more elaborate, and also a little more cogniscent of what the syscall actually does. I based my implementation a lot on the previous one, with the idea in the back of my mind that I wouldn't be spending too much time on it, because we would probably want to remove that code one day - as it's based on a deprecated feature. (and for good measure, I don't believe in ia64's future either) Attached is what I checked in. There were a couple of tweaks I needed to make (TARGET_XFER_EIO -> TARGET_XFER_E_IO, and res was not assigned). I'm a little nervous about the gdb_assert, as I usually prefer to use them only when everything is under our control and we can't recover from the situation. I was almost ready to see what would happen if we returned E_IO instead of brutally interruping the debugging session. But we'll see what happens with the current code. BTW: Now that I have pushed the patch, I just realized something. Are you sure the gate_table_size is a constant? >:-o gdb/ChangeLog: * ia64-linux-nat.c (ia64_linux_xfer_partial): Reimplement handling of object == TARGET_OBJECT_UNWIND_TABLE. Thanks, -- Joel --8t9RHnE3ZwKMSgU+ Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Re-implement-ia64-linux-nat.c-ia64_linux_xfer_partia.patch" Content-length: 2742 >From d16461aeef555da47e358b0f81c75912e4ea07e2 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 25 Feb 2014 20:45:50 -0500 Subject: [PATCH] Re-implement ia64-linux-nat.c::ia64_linux_xfer_partial [description of this patch and ChangeLog entry by Joel Brobecker] The recent implementation was questionable, and if it worked, it was only by chance because the requested length is large enough that only one read was sufficient. Note that the implementation before that also made that assumption, in the form of only handling TARGET_OBJECT_UNWIND_TABLE xfer requests when offset was zero. gdb/ChangeLog: * ia64-linux-nat.c (ia64_linux_xfer_partial): Reimplement handling of object == TARGET_OBJECT_UNWIND_TABLE. --- gdb/ChangeLog | 5 +++++ gdb/ia64-linux-nat.c | 35 +++++++++++++++++++++++------------ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index e60ec1c..16f4619 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2014-02-25 Pedro Alves + + * ia64-linux-nat.c (ia64_linux_xfer_partial): Reimplement + handling of object == TARGET_OBJECT_UNWIND_TABLE. + 2014-02-25 Stan Shebs * defs.h: Annotate comments for Doxygen. diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c index c057b55..3cfbd76 100644 --- a/gdb/ia64-linux-nat.c +++ b/gdb/ia64-linux-nat.c @@ -850,20 +850,31 @@ ia64_linux_xfer_partial (struct target_ops *ops, { if (object == TARGET_OBJECT_UNWIND_TABLE && readbuf != NULL) { - gdb_byte *tmp_buf = alloca (offset + len); - ULONGEST xfered; - - xfered = syscall (__NR_getunwind, readbuf, offset + len); - if (xfered <= 0) + static long gate_table_size; + gdb_byte *tmp_buf; + long res; + + /* Probe for the table size once. */ + if (gate_table_size == 0) + gate_table_size = syscall (__NR_getunwind, NULL, 0); + if (gate_table_size < 0) return TARGET_XFER_E_IO; - else if (xfered <= offset) + + if (offset >= gate_table_size) return TARGET_XFER_EOF; - else - { - memcpy (readbuf, tmp_buf + offset, xfered - offset); - *xfered_len = xfered - offset; - return TARGET_XFER_OK; - } + + tmp_buf = alloca (gate_table_size); + res = syscall (__NR_getunwind, tmp_buf, gate_table_size); + if (res < 0) + return TARGET_XFER_E_IO; + gdb_assert (res == gate_table_size); + + if (offset + len > gate_table_size) + len = gate_table_size - offset; + + memcpy (readbuf, tmp_buf + offset, len); + *xfered_len = len; + return TARGET_XFER_OK; } return super_xfer_partial (ops, object, annex, readbuf, writebuf, -- 1.7.9.5 --8t9RHnE3ZwKMSgU+--