From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19682 invoked by alias); 17 Apr 2013 20:33:11 -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 19651 invoked by uid 89); 17 Apr 2013 20:33:11 -0000 X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 17 Apr 2013 20:33:10 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3HKX7nU008452 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 17 Apr 2013 16:33:07 -0400 Received: from host2.jankratochvil.net (ovpn-116-84.ams2.redhat.com [10.36.116.84]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3HKX0OF006605 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 17 Apr 2013 16:33:03 -0400 Date: Thu, 18 Apr 2013 07:40:00 -0000 From: Jan Kratochvil To: Aleksandar Ristovski Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 6/8] gdbserver build-id attribute generator Message-ID: <20130417203300.GB2090@host2.jankratochvil.net> References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1366127096-5744-1-git-send-email-ARistovski@qnx.com> <1366127096-5744-7-git-send-email-ARistovski@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1366127096-5744-7-git-send-email-ARistovski@qnx.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-04/txt/msg00549.txt.bz2 On Tue, 16 Apr 2013 17:44:54 +0200, Aleksandar Ristovski wrote: [...] > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c [...] > +#define ELFXX_FLD(elf64, hdr, fld) ((elf64) ? (hdr)._64.fld : (hdr)._32.fld) > +#define ELFXX_SIZEOF(elf64, hdr) ((elf64) ? sizeof ((hdr)._64) \ > + : sizeof ((hdr)._32)) > +/* Round up to next 4 byte boundary. */ > +#define ELFXX_ROUNDUP_4(elf64, what) ((elf64) ? (((what) + 3) \ > + & ~((Elf64_Word)3U)) \ GNU Coding Standards: & ~((Elf64_Word) 3U)) \ > + : (((what) + 3) \ > + & ~((Elf32_Word) 3U))) This is still overcomplicated, why the ELF64 parameter at all? There should not be any. And all the casts there with Elf64_Word and Elf32_Word: * both Elf64_Word and Elf32_Word are the same, they are 32-bit. * despite all the casts the macro is not safe for 64-bit WHAT as the mask will be just 0xffffffff. Put there just: #define ELFXX_ROUNDUP_4(what) (((what) + 3) & ~(ULONGEST) 3) > +#define BUILD_ID_INVALID "?" > + > /* ``all_threads'' is keyed by the LWP ID, which we use as the GDB protocol > representation of the thread ID. > [...] > +/* Used for finding a mapping containing the given > + l_ld passed in K. */ > + > +static int > +compare_mapping_entry_range (const void *const k, const void *const b) > +{ > + const ULONGEST key = *(CORE_ADDR *) k; More commented in another mail. I already wrote when k is const * there should be (const CORE_ADDR *). It is here even unrelated to the const values unusual in GDB. 'k' is pointing to const data so it is incorrect to cast it to non-const-target pointer. In such case 'k' should be void * and not const void *. > + const mapping_entry_s *const p = b; > + > + if (key < p->vaddr) > + return -1; > + > + if (key < p->vaddr + p->size) > + return 0; > + > + return 1; > +} [...] > +/* Read build-id from PT_NOTE. > + Argument LOAD_ADDR pepresents run time virtual address corresponding to > + the beginning of the first loadable segment. L_ADDR is displacement > + as supplied by the dynamic linker. */ > + > +static void > +read_build_id (struct find_memory_region_callback_data *const p, > + mapping_entry_s *const bil, const CORE_ADDR load_addr, > + const CORE_ADDR l_addr) > +{ > + const int is_elf64 = p->is_elf64; > + ElfXX_Ehdr ehdr; > + > + if (linux_read_memory (load_addr, (unsigned char *) &ehdr, > + ELFXX_SIZEOF (is_elf64, ehdr)) == 0 > + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG0]) == ELFMAG0 > + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG1]) == ELFMAG1 > + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG2]) == ELFMAG2 > + && ELFXX_FLD (is_elf64, ehdr, e_ident[EI_MAG3]) == ELFMAG3) > + { > + const ElfXX_Phdr *phdr; > + void *phdr_buf; > + const unsigned e_phentsize = ELFXX_FLD (is_elf64, ehdr, e_phentsize); > + > + if (ELFXX_FLD (is_elf64, ehdr, e_phnum) >= 100 > + || e_phentsize != ELFXX_SIZEOF (is_elf64, *phdr)) > + { > + /* Basic sanity check failed. */ > + warning ("Could not read program header."); Print LOAD_ADDR, otherwise one has to start debugging gdbserver to find out why the message was printed. The message could be different than the message for failed linux_read_memory below. > + return; > + } > + > + phdr_buf = alloca (ELFXX_FLD (is_elf64, ehdr, e_phnum) * e_phentsize); > + > + if (linux_read_memory (load_addr + ELFXX_FLD (is_elf64, ehdr, e_phoff), > + phdr_buf, > + ELFXX_FLD (is_elf64, ehdr, e_phnum) * e_phentsize) > + != 0) > + { > + warning ("Could not read program header."); Print LOAD_ADDR, otherwise one has to start debugging gdbserver to find out why the message was printed. > + return; > + } > + > + phdr = phdr_buf; > + > + for (;;) > + { > + gdb_byte *pt_note; > + const gdb_byte *pt_end; > + const ElfXX_Nhdr *nhdr; > + CORE_ADDR note_addr; > + > + phdr = find_phdr (p->is_elf64, phdr, (gdb_byte *) phdr_buf > + + ELFXX_FLD (is_elf64, ehdr, e_phnum) * e_phentsize, > + PT_NOTE); > + if (phdr == NULL) > + break; > + pt_note = xmalloc (ELFXX_FLD (is_elf64, *phdr, p_memsz)); > + note_addr = ELFXX_FLD (is_elf64, *phdr, p_vaddr) + l_addr; > + if (linux_read_memory (note_addr, pt_note, > + ELFXX_FLD (is_elf64, *phdr, p_memsz)) != 0) > + { > + xfree (pt_note); > + warning ("Could not read note at address 0x%s", > + paddress (note_addr)); > + break; > + } > + > + pt_end = pt_note + ELFXX_FLD (is_elf64, *phdr, p_memsz); > + nhdr = (void *) pt_note; > + while ((gdb_byte *) nhdr < pt_end) This should be (const gdb_byte *) as nhdr is const-target pointer. > + { > + const size_t namesz > + = ELFXX_ROUNDUP_4 (is_elf64, ELFXX_FLD (is_elf64, *nhdr, > + n_namesz)); > + const size_t descsz > + = ELFXX_ROUNDUP_4 (is_elf64, ELFXX_FLD (is_elf64, *nhdr, > + n_descsz)); > + const size_t note_sz = ELFXX_SIZEOF (is_elf64, *nhdr) + namesz > + + descsz; GNU Coding Standards: const size_t note_sz = (ELFXX_SIZEOF (is_elf64, *nhdr) + namesz + descsz); > + > + if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0 This should be (const gdb_byte *) as nhdr is const-target pointer. > + || descsz == 0) > + { > + warning ("Malformed PT_NOTE at address 0x%s\n", > + paddress ((CORE_ADDR) nhdr)); NHDR is in gdbserver's memory, that has no useful value for the user. You should print: paddress (note_addr + nhdr - pt_note)); > + break; > + } [...] > +/* Get build-id for the given L_LD, where L_LD corresponds to > + link_map.l_ld as specified by the dynamic linker. > + DATA must point to already filled list of mapping_entry elements. > + > + If build-id had not been read, read it and cache in corresponding > + list element. > + > + Return build_id as stored in the list element corresponding > + to L_LD. > + > + NULL may be returned if build-id could not be fetched. > + > + Returned string must not be freed explicitly. */ > + > +static const char * > +get_hex_build_id (const CORE_ADDR l_addr, const CORE_ADDR l_ld, > + struct find_memory_region_callback_data *const data) > +{ > + mapping_entry_s *bil; > + > + bil = bsearch (&l_ld, VEC_address (mapping_entry_s, data->list), > + VEC_length (mapping_entry_s, data->list), > + sizeof (mapping_entry_s), compare_mapping_entry_range); > + > + if (bil == NULL) > + return NULL; > + > + if (bil->hex_build_id == NULL) > + { > + mapping_entry_s *bil_min; > + > + bil_min = lrfind_mapping_entry (bil, VEC_address (mapping_entry_s, > + data->list)); > + if (bil_min != NULL) > + read_build_id (data, bil, bil_min->vaddr, l_addr); > + else > + { > + /* Do not try to find hex_build_id again. */ > + bil->hex_build_id = xstrdup (BUILD_ID_INVALID); > + warning ("Could not determine load address; mapping entry with" > + " offset 0 corresponding to l_ld=0x%s could not be found; " > + "build-id can not be used.", paddress (l_ld)); GNU Coding Standards s/=/ = / - IMO it applies also to the output. Also GDB uses continuation space at the end of previous line. There should be localization. warning (_("Could not determine load address; mapping entry with " "offset 0 corresponding to l_ld = 0x%s could not be " "found; build-id can not be used."), paddress (l_ld)); > + } > + } > + > + return bil->hex_build_id; > +} > + > /* Construct qXfer:libraries-svr4:read reply. */ > > static int [...] Thanks, Jan