From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2524 invoked by alias); 16 Apr 2013 15:44:04 -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 2490 invoked by uid 89); 16 Apr 2013 15:44:04 -0000 X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_CS autolearn=ham version=3.3.1 Received: from na3sys009aog110.obsmtp.com (HELO na3sys009aog110.obsmtp.com) (74.125.149.203) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 16 Apr 2013 15:44:02 +0000 Received: from mx10.qnx.com ([209.226.137.110]) (using TLSv1) by na3sys009aob110.postini.com ([74.125.148.12]) with SMTP ID DSNKUW1xwDHaMR7UC1L4HQY8HWI2Yh2MFjsw@postini.com; Tue, 16 Apr 2013 08:44:02 PDT Received: by mx10.qnx.com (Postfix, from userid 500) id 36CA52115E; Tue, 16 Apr 2013 11:43:59 -0400 (EDT) Received: from exhts.ott.qnx.com (exch2 [10.222.2.136]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mx10.qnx.com (Postfix) with ESMTPS id B2CF11FD6E; Tue, 16 Apr 2013 11:43:58 -0400 (EDT) Received: from [10.222.96.215] (10.222.2.5) by EXCH2.ott.qnx.com (10.222.2.136) with Microsoft SMTP Server (TLS) id 14.2.318.4; Tue, 16 Apr 2013 11:43:58 -0400 Message-ID: <516D6AE7.8030700@qnx.com> Date: Tue, 16 Apr 2013 16:40:00 -0000 From: Aleksandar Ristovski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Jan Kratochvil CC: Subject: Re: [PATCH 6/8] gdbserver build-id attribute generator References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1365521265-28870-7-git-send-email-ARistovski@qnx.com> <20130414141658.GD23227@host2.jankratochvil.net> In-Reply-To: <20130414141658.GD23227@host2.jankratochvil.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00482.txt.bz2 On 13-04-14 10:16 AM, Jan Kratochvil wrote: > On Tue, 09 Apr 2013 17:27:43 +0200, Aleksandar Ristovski wrote: > [...] >> + >> +#define ELFXX_FLD(hdr, fld) ((is_elf64) ? (hdr)._64.fld : (hdr)._32.fld) >> +#define ELFXX_SIZEOF(hdr) ((is_elf64) ? sizeof ((hdr)._64) \ >> + : sizeof ((hdr)._32)) > > Macros should not depend on external variables, that makes them tricky, it was > acceptable when the variable was present in the scope where the macro was > defined. But that is no longer true when the macro is global. > > is_elf64 should be a parameter of each of these macros. > Ok. > >> +#define ELFXX_ROUNDUP(what) ((is_elf64) ? (((what) + sizeof (Elf64_Word) - 1) \ >> + & ~(sizeof (Elf64_Word) - 1)) \ >> + : (((what) + sizeof (Elf32_Word) - 1) \ >> + & ~(sizeof (Elf32_Word) - 1))) > > This is overcomplicated. The ELF standard defines it as "4-byte alignment". > While both sizeof (Elf64_Word) == 4 and sizeof (Elf32_Word) == 4 I find that > incorrect, the standard talks about 4 bytes (for both elf32 and elf64), not > about sizeof of anything. Ok. I re-read it and indeed it says 4 bytes, and not how I mis-remembered, word boundary. > > ... >> >> +/* Linearly traverse pheaders given in PHDR until supplied >> + predicate function returns 1. If supplied predicate function >> + did return 1, stop traversal and return that PHDR. */ > > There is no predicate function anymore. Ok. > > >> + >> ... > >> + if (phdr == NULL) >> + return NULL; > > I do not see a reason for this check, callers never pass it NULL. It should > be rather gdb_assert in such case if anything. Ok. removed. > > ... >> + { >> + if (is_elf64) >> + { >> + const Elf64_Phdr *const p = phdr; > > When the 32-vs-64 ELF framework is provided here I expected this existing code > could be simplified on top of it. Provided such patch at the end of this > mail. Yes, indeed. Incorporated. > > ... >> +static void >> +free_mapping_entry (VEC (mapping_entry_s) *lst) > > It does not free mapping_entry. It frees the vector of them. Therefore it > should be name for example free_mapping_entry_vec. And the function is > missing comment. I consider this nitpicking - argument type does convey this very clearly. But added _vec as per your suggestion. > > ... >> + const ULONGEST key = *(CORE_ADDR*) k; > > GNU Coding Standards: > const ULONGEST key = *(CORE_ADDR *) k; > And when it is all const it should not be temporarily de-const-ed: > const ULONGEST key = *(const CORE_ADDR *) k; > Ok. ... >> + VEC (mapping_entry_s) *list; >> +}; >> + >> +static linux_find_memory_region_ftype find_memory_region_callback; > > Why is it here? It should be before the find_memory_region_callback > definition. > Ok. > >> + >> +/* Read build-id from PT_NOTE. */ > > Describe function parameters. It is essential to see the LOAD_ADDR vs. L_ADDR > difference. Ok. > > ... >> + gdb_assert (ELFXX_FLD (ehdr, e_phnum) < 100); /* Basic sanity check. */ > > I am aware a similar gdb_assert is in get_dynamic but that is a bug. > gdbserver should not crash on weird inferior data. > There should be a warning + return. > > Also comments should be on their own line: > /* Basic sanity check. */ > gdb_assert (ELFXX_FLD (ehdr, e_phnum) < 100); > > >> + gdb_assert (e_phentsize == ELFXX_SIZEOF (*phdr)); > > Likewise. Ok, replaced with a check + warning. > > ... >> + pt_note = xmalloc (ELFXX_FLD (*phdr, p_memsz)); >> + pt_end = (gdb_byte*) pt_note + ELFXX_FLD (*phdr, p_memsz); > > GNU Coding Standards: > pt_end = (gdb_byte *) pt_note + ELFXX_FLD (*phdr, p_memsz); > But pt_note is already gdb_byte * so that cast is useless, therefore: > pt_end = pt_note + ELFXX_FLD (*phdr, p_memsz); > Ok. > >> + >> + if (linux_read_memory (ELFXX_FLD (*phdr, p_vaddr) + l_addr, pt_note, >> + ELFXX_FLD (*phdr, p_memsz)) != 0) >> + { >> + xfree (pt_note); >> + warning ("Could not read note."); > > Print also the note address when there is a warning at all. Ok. > > >> + break; >> + } >> + >> + nhdr = (void *) pt_note; >> + while ((void *) nhdr < (void *) pt_end) > > When it is all const there should be (const void *). But in fact it is easier > to use const gdb_byte * when pt_end is already such type: > while ((const gdb_byte *) nhdr < pt_end) gdb_byte * is o.k., but there is no really a need for const - when casting is for the purpose of pointer comparison, IMO const only clutters the reading. > > >> + { >> + const size_t namesz >> + = ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_namesz)); >> + const size_t descsz >> + = ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_descsz)); >> + const size_t note_sz = ELFXX_SIZEOF (*nhdr) + namesz + descsz; >> + >> + if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0 > > It should be (const gdb_byte *) because nhdr is already const *. I like using const, as you may have noticed, unless it only clutters, like is the case when casting is used for the purpose of pointer comparison like here. > > >> + || descsz == 0) >> + { >> + warning ("Malformed PT_NOTE\n"); > > Print also the note address when there is a warning at all. Ok. > > >> + break; >> + } >> + if (ELFXX_FLD (*nhdr, n_type) == NT_GNU_BUILD_ID >> + && ELFXX_FLD (*nhdr, n_namesz) == 4) >> + { >> + const char gnu[4] = "GNU\0"; >> + const char *const pname >> + = (char *) nhdr + ELFXX_SIZEOF (*nhdr); > > It should be (const char *) because nhdr is already const *. When assigning to const, then const in the cast for pointer arithmetic does not matter and IMO only clutters. > > >> + >> + if (memcmp (pname, gnu, 4) == 0) >> + { >> + const size_t n_descsz = ELFXX_FLD (*nhdr, n_descsz); >> + >> + bil->hex_build_id = xmalloc (n_descsz * 2 + 1); >> + bin2hex ((gdb_byte*) pname + namesz, bil->hex_build_id, > > It should be (const gdb_byte *) because pname is already const *. Ok. > > Additionally according to the GNU Coding Standards spacing it should be: > bin2hex ((const gdb_byte *) pname + namesz, bil->hex_build_id, > > >> + n_descsz); >> + xfree (pt_note); >> + return; >> + } >> + } >> + nhdr = (void*) ((gdb_byte *) nhdr + note_sz); >> + } >> + xfree (pt_note); >> + } >> + } >> +} >> + >> +/* Add mapping_entry. */ > > When the line > static linux_find_memory_region_ftype find_memory_region_callback; > will be here the parameters would be described. But still one could write: > /* Add mapping_entry. See linux_find_memory_region_ftype for the > parameters description. */ Ok. > > ... >> + >> +/* Get build-id for the given L_LD. DATA must point to > > Maybe one could describe more what is "L_LD". > L_LD is the link_map.l_ld field from libc shared library list. > > And that L_ADDR parameter is also: > L_ADDR is the link_map.l_addr field from libc shared library list. > Ok. > >> + already filled list of mapping_entry elements. >> + >> + 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; >> + >> + if (VEC_address (mapping_entry_s, data->list) == NULL) >> + return NULL; > > I do not think this check should be / needs to be here. bsearch with NMEMB == > 0 should return NULL. You are probably right. Removed. > > ... >> + { >> + /* Do not try to find hex_build_id again. */ >> + bil->hex_build_id = xstrdup (BUILD_ID_INVALID); >> + warning ("Could not determine load address; " >> + "build-id can not be used."); > > You should print some identification for troubleshooting when the warning is > there at all, probably L_LD here is the best one. Ok. > > ... >> + } >> { >> /* Expand to guarantee sufficient storage. */ >> - uintptr_t document_len = p - document; >> + const ptrdiff_t document_len = p - document; >> >> - document = xrealloc (document, 2 * allocated); >> allocated *= 2; >> + document = xrealloc (document, allocated); >> p = document + document_len; >> } > > This "code cleanup" change is unrelated to this patch. But it is IMO not > worth checking in separately, it does not improve it anyhow IMO. It uses correct type for pointer difference and does not repeat arithmetic. I removed it, it will probably reduce clash with Gary's patch. > > >> >> - name = xml_escape_text ((char *) libname); > > Why did you move this line to several lines above? It is a needless change. Ok. > > >> p += sprintf (p, "> - "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>", >> + "l_addr=\"0x%lx\" l_ld=\"0x%lx\"", >> name, (unsigned long) lm_addr, >> (unsigned long) l_addr, (unsigned long) l_ld); >> + if (hex_enc_build_id != NULL >> + && strcmp (hex_enc_build_id, BUILD_ID_INVALID) != 0) >> + p += sprintf (p, " build-id=\"%s\"", hex_enc_build_id); > >> + p += sprintf(p, "/>"); > > GNU Coding Standards: > p += sprintf (p, "/>"); > Ok. --- Aleksandar