From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12538 invoked by alias); 21 Jan 2004 22:06:45 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 12531 invoked from network); 21 Jan 2004 22:06:44 -0000 Received: from unknown (HELO gateway.sf.frob.com) (64.81.54.130) by sources.redhat.com with SMTP; 21 Jan 2004 22:06:44 -0000 Received: from magilla.sf.frob.com (magilla.sf.frob.com [198.49.250.228]) by gateway.sf.frob.com (Postfix) with ESMTP id 4CE88357B; Wed, 21 Jan 2004 14:06:43 -0800 (PST) Received: from magilla.sf.frob.com (localhost.localdomain [127.0.0.1]) by magilla.sf.frob.com (8.12.9/8.12.9) with ESMTP id i0LM6hOi025952; Wed, 21 Jan 2004 14:06:43 -0800 Received: (from roland@localhost) by magilla.sf.frob.com (8.12.9/8.12.9/Submit) id i0LM6gP1025948; Wed, 21 Jan 2004 14:06:42 -0800 Date: Wed, 21 Jan 2004 22:06:00 -0000 Message-Id: <200401212206.i0LM6gP1025948@magilla.sf.frob.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit From: Roland McGrath To: Andrew Cagney Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH] auxv support via xfer_partial, for core files and /proc In-Reply-To: Andrew Cagney's message of Wednesday, 21 January 2004 10:48:00 -0500 <400E9F30.1060106@gnu.org> Emacs: it's not slow --- it's stately. X-SW-Source: 2004-01/txt/msg00595.txt.bz2 > Roland, FYI, there is already this patch: > http://sources.redhat.com/ml/gdb-patches/2003-11/msg00204.html Thanks for the pointer. I have some comments below comparing the two patches. > What you should definitly do is add yourself to the write-after-approval > list (don't forget to the ChangeLog and to post the committed change). Done. Here are the notable differences between the two patches. 1. My patch includes the changes affecting gcore: * procfs.c (procfs_make_note_section): If we can read TARGET_OBJECT_AUXV data, add an NT_AUXV note containing it. * linux-proc.c (linux_make_note_section): Likewise. This is not something your patch tries to address. I think we want those changes regardless of the implementation details for TARGET_OBJECT_AUXV. 2. Your patch defines the "info auxv" command for pretty-printing. That is a nice thing to have. However, I would redo your implementation. As you've written it, `read_auxv_entry' will be called for each element of the vector and read the whole thing fresh each time. On my 2.6 kernel, that is 17 times open+pread+close instead of one. 3. My patch includes the reading support for core files: * corelow.c (core_xfer_partial): New function. (init_core_ops): Use it for core_ops.to_xfer_partial. This is something that could not be tested using my patch alone, but your "info auxv" command provides the way to test it. (I did test that code, but by using other patches not yet submitted.) 4. You have a single implementation of native_xfer_auxv that presumes that /proc/PID/auxv is the thing to use on every potential native system. The systems we know of supporting auxv access (Linux and Solaris) do all use the same method. But assuming this does not seem proper or consistent with how things are done in gdb. My patch implements /proc/PID/auxv reading in two places. This is a bit of unsightly duplication, but consistent with the existing duplication of code between procfs.c and linux-proc.c. For Linux, the implementation is via defining a macro in nm-linux.h that inftarg.c's generic xfer_partial function will use; that is consistent with the existing NATIVE_XFER_UNWIND_TABLE macro, and the status quo wherein Linux support does not have its own struct target_ops. The procfs.c addition seems like the cleaner approach, defining to_xfer_partial for that target. (That way it also works when using "target procfs", which seems proper.) For Solaris that required the sol-thread.c addition so that it would pass down the call to procfs_ops in the same way it does for to_xfer_memory (i.e. twiddling inferior_ptid). 5. Your implementation uses pread unconditionally. Mine doesn't bother to use pread even when it's available, and just uses lseek + read. Full portability, including things like older glibcs running on newer kernels, requires using configure tests for pread. I did not bother with that because in the real-world case, there will just be one read from offset 0 and so lseek won't even be called, just open+read+close. 6. My implementation does not try to support writing. It would be trivial to do so if you think it's worthwhile. I did not bother because no system actually supports changing this data, and I can't see a situation in which gdb might ever want to even if it were possible. My inclination is to add the "info auxv" implementation to my patch, and leave the other details as I did them. Comments? Thanks, Roland