From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12943 invoked by alias); 31 Oct 2011 03:17:17 -0000 Received: (qmail 12934 invoked by uid 22791); 31 Oct 2011 03:17:14 -0000 X-SWARE-Spam-Status: No, hits=-7.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 31 Oct 2011 03:16:56 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p9V3Guik025341 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 30 Oct 2011 23:16:56 -0400 Received: from psique ([10.3.112.13]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p9V3Gq6T029422; Sun, 30 Oct 2011 23:16:54 -0400 From: Sergio Durigan Junior To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Implement new `info core mappings' command References: <20111031001117.GA11608@host1.jankratochvil.net> Date: Mon, 31 Oct 2011 07:00:00 -0000 In-Reply-To: <20111031001117.GA11608@host1.jankratochvil.net> (Jan Kratochvil's message of "Mon, 31 Oct 2011 01:11:18 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes 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 X-SW-Source: 2011-10/txt/msg00815.txt.bz2 Hello Jan, Thanks for the review. Comments below. Jan Kratochvil writes: > On Wed, 26 Oct 2011 22:49:53 +0200, Sergio Durigan Junior wrote: >> --- a/gdb/corefile.c >> +++ b/gdb/corefile.c > [...] >> @@ -83,6 +89,186 @@ core_file_command (char *filename, int from_tty) >> } >> >> >> +/* Helper function for `print_core_map'. It is used to iterate >> + over the corefile's sections and print proper information about >> + memory-mappings. >> + >> + BFD is the bfd used to get the sections. >> + SECT is the current section being "visited". >> + OBJ is not used. */ >> + >> +static void >> +print_proc_map_iter (bfd *bfd, asection *sect, void *obj) >> +{ >> + /* We're interested in matching sections' names beginning with >> + `load', because they are the sections containing information >> + about the process' memory regions. */ >> + static const char *proc_map_match = "load"; > > I think they are pretty useful, for Linux kernel dumped core files with > MMF_DUMP_ELF_HEADERS > /usr/share/doc/kernel-doc-*/Documentation/filesystems/proc.txt > - (bit 4) ELF header pages in file-backed private memory areas (it is > effective only if the bit 2 is cleared) > > Program Headers: > Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align > NOTE 0x001508 0x0000000000000000 0x0000000000000000 0x0008d0 0x000000 0 > LOAD 0x002000 0x0000000000400000 0x0000000000000000 0x001000 0x008000 R E 0x1000 > LOAD 0x003000 0x0000000000607000 0x0000000000000000 0x002000 0x002000 RW 0x1000 > > (gdb) info files > Local core dump file: > 0x0000000000400000 - 0x0000000000401000 is load1a > 0x0000000000401000 - 0x0000000000401000 is load1b > 0x0000000000607000 - 0x0000000000609000 is load2 > > But one does not see the ending address 0x408000 anywhere, one IMO has to use > readelf/objdump now to get the full information. > > > I think this function should not be based on sections at all, it should just > read the segments. Linux kernel does not dump any sections. bfd creates some > some sections from those segments (_bfd_elf_make_section_from_phdr) but they > cannot / do not contain any additional info, those are there IMO only for > better compatibility with sections-only consuming code. Just to be clear, you're saying that I should actually forget about the part of the code which checks inside (possible non-empty) sections in the corefile, and just check immediately for segments? >> + int proc_map_match_size = strlen (proc_map_match); >> + /* Flag to indicate whether we have found something. */ >> + int found = 0; >> + /* The section's size. */ >> + bfd_size_type secsize; >> + /* We have to know the bitness of this architecture. */ >> + int bitness; >> + /* We'll use these later. They are basically used for iterating >> + over every objfile in the system so that we can find needed >> + information about the memory region being examinated. */ >> + struct obj_section *s = NULL; >> + struct objfile *objfile = NULL; >> + /* Fields to be printed for the proc map. */ >> + unsigned long start = 0, end = 0; >> + unsigned int size = 0; > > On 32bit host with --enable-64-bit-bfd: sizeof (bfd_vma) > sizeof (long) > moreover for sizeof (int) of `size'. Ok, I'll replace that by unsigned long too, thanks. >> + char *filename = NULL; >> + >> + if (strncmp (proc_map_match, sect->name, proc_map_match_size) != 0) >> + /* This section is not useful. */ >> + return; >> + >> + bitness = gdbarch_addr_bit (gdbarch_from_bfd (bfd)); >> + >> + /* Unfortunately, some sections in the corefile don't have any >> + content inside. This is bad because we need to print, among >> + other things, its final address in the memory (which is >> + impossible to know if we don't have a size). That's why we >> + first need to check if the section's got anything inside it. */ >> + secsize = bfd_section_size (bfd, sect); >> + >> + if (secsize == 0) >> + { >> + /* Ok, the section is empty. In this case, we must look inside >> + ELF's Program Header, because (at least) there we have >> + information about the section's size. That's what we're doing >> + here. */ >> + Elf_Internal_Phdr *p = elf_tdata (bfd)->phdr; >> + if (p != NULL) >> + { >> + int i; >> + unsigned int n = elf_elfheader (bfd)->e_phnum; >> + for (i = 0; i < n; i++, p++) >> + /* For each entry in the Program Header, we have to >> + check if the section's initial address is equal to >> + the entry's virtual address. If it is, then we >> + have just found the section's entry in the Program >> + Header, and can use the entry's information to >> + complete missing data from the section. */ >> + if (sect->vma == p->p_vaddr) >> + { >> + found = 1; >> + break; >> + } > > I do not understand what is a goal of this part. Isn't it related to the > pairtally omitted segments above? But those are named "load..." so they are > already skipped. I'm not sure I understood what you said. The sections named "load..." are not skipped. It's the sections *not* named "load..." which are. >> + /* Now begins a new part of the work. We still don't have complete >> + information about the memory region. For example, we still need >> + to know the filename which is represented by the region. Such >> + info can be gathered from the objfile's data structure, and for >> + that we must iterate over all the objsections and check if the >> + objsection's initial address is inside the section we have at hand. >> + If it is, then we can use this specific objsection to obtain the >> + missing data. */ >> + found = 0; >> + ALL_OBJSECTIONS (objfile, s) >> + if (obj_section_addr (s) >= start >> + && obj_section_addr (s) <= end) > > I think it should ignore S which is section_is_overlay. You mean I should check for `!section_is_overlay (s)' here? >> +static void >> +print_core_map (void) >> +{ >> + const char *exe; >> + int bitness; >> + >> + gdb_assert (core_bfd != NULL); >> + >> + bitness = gdbarch_addr_bit (gdbarch_from_bfd (core_bfd)); >> + >> + /* Getting the executable name. */ >> + exe = bfd_core_file_failing_command (core_bfd); >> + >> + printf_filtered (_("exe = '%s'\n"), exe); > > bfd_core_file_failing_command can return NULL, NULL is not compatible with %s; > also the bfd error may be printed in such case. Oh, thanks for the catch, I'll update this accordingly. >> @@ -450,6 +636,11 @@ _initialize_core (void) >> { >> struct cmd_list_element *c; >> >> + add_info ("core", info_core_cmd, _("\ >> +Show information about a corefile.\n\ >> +Specify any of the following keywords for detailed info:\n\ >> + mappings -- list of mapped memory regions.")); > > I think it should be add_prefix_cmd so that tab completion works. "mappings > / "all" should be commands, not parameters. "info proc" already has this bug. Yeah, `info proc' is buggy indeed. I'll see if I send a patch fixing it tomorrow. Thanks for the tip. Thank you, Sergio.