From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/1443 Path: news.gmane.org!not-for-mail From: musl Newsgroups: gmane.linux.lib.musl.general Subject: Re: ldso : dladdr support Date: Tue, 07 Aug 2012 16:15:34 +0200 Message-ID: <50212306.6070402@gmail.com> References: <5020DA13.6080803@gmail.com> <20120807114627.GG30810@port70.net> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1344348951 10210 80.91.229.3 (7 Aug 2012 14:15:51 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Tue, 7 Aug 2012 14:15:51 +0000 (UTC) Cc: Szabolcs Nagy To: musl@lists.openwall.com Original-X-From: musl-return-1444-gllmg-musl=m.gmane.org@lists.openwall.com Tue Aug 07 16:15:51 2012 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1SykZa-0004Yo-2o for gllmg-musl@plane.gmane.org; Tue, 07 Aug 2012 16:15:50 +0200 Original-Received: (qmail 21924 invoked by uid 550); 7 Aug 2012 14:15:48 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 21916 invoked from network); 7 Aug 2012 14:15:48 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=fmKcy5POqvECzj2wyKSAVozYWIpo7tt7o9q0AQpqZFM=; b=ZAcRTdd1BGDc82ed6iGkxPTWFTzcbefRi7MMoYXMZ7oulDPtlmDi2Bq1yqYrHFpgiT LkBMFiUTaYoyibHR+/tmUAGco+nyIKVen9uD+hPwvQnsTwpi359fT+tRd/PzW/Pghjgy RnamLVxvPz+SJXMB5L7+fYNR4aWU7mnd27YdkX7fzLHqCjsx5cIHyI24+Mu3G73p7J9P +TjFvIg1UyS1ivPAQrA+5Rlz3T+1v2sOQKLddP5EbeMqY0kH+QK63KkJ/SCyej38ibEa aooZ+ncdDBXS9GhYxJlRRT94WB4xLoa3Zm6J9cl3BJgeUi8MX29+2EKouLJY4bVc3VXe qzdQ== User-Agent: Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120714 Thunderbird/14.0 In-Reply-To: <20120807114627.GG30810@port70.net> Xref: news.gmane.org gmane.linux.lib.musl.general:1443 Archived-At: Thanks for your review. On 07/08/2012 13:46, Szabolcs Nagy wrote: > * musl [2012-08-07 11:04:19 +0200]: >> This patch adds support for dladdr function. >> It is based on my previous patch (gnu hash support). >> > i havent checked the content of your patch yet > just had a quick glance > > i think before you make substantial changes > it's better to have some discussion about > the design Could you tell me more about the design issues (I guess this has something to do with function pointers and multi hash algorithms design) ? > > i'll just have some stylistic comments for now > (i think there is no official guideline and > we are not terribly anal about it but i'll > still point out a few things for future reference) > >> struct hash_algo { >> uint32_t (*hash) (const char *); >> - Sym *(*lookup) (const char *s, uint32_t h, struct dso *dso); >> + Sym *(*lookup) (const char *, uint32_t, struct dso *); >> + void (*iterate) (struct dso *, int (*) (struct dso *, Sym *, void *), void *); >> }; >> > use > foo(arg) > instead of > foo (arg) > > in case of keywords it's not clear what's better > (musl usually uses spaces after if,while,for,..) > but for function calls no space is clearer > > (in general musl code rarely uses unnecessary spaces and parentheses) I'm correcting my code to match the musl coding style. > > actually i'm not sure if the function pointer approach is > the right one here, but that's a design question > Could you tell me why (performance)? >> + uint32_t *hashtab = dso->hashtab; >> + uint32_t *buckets = hashtab + 4 + (hashtab[2] * (sizeof(size_t) / sizeof(uint32_t))); > i don't know the algorithm but the sizeof magic looks suspicious to me I took the algorithm described here : https://blogs.oracle.com/ali/entry/gnu_hash_elf_sections The maskwords are native word size (64 bits for elf64 and 32 bits for elf32). I could define these macros: #define MASKWORD_SIZE sizeof (Elf32_Word) #define MASKWORD_SIZE sizeof (Elf64_Word) > >> +static uint32_t gnu_hash (const char *s0) >> +{ >> + const unsigned char *s = (void *)s0; >> + uint_fast32_t h = 5381; >> + for (unsigned char c = *s; c != '\0'; c = *++s) >> + h = h * 33 + c; >> + return h & 0xffffffff; >> +} >> + > i think c != '\0' is not very idiomatic > and i'd avoid using c99 style declaration in for > > use > > for (; *s; s++) > h = 33*h + *s; > Corrected. >> + uint32_t shift2 = hashtab[3]; >> + uint32_t h2 = h1 >> shift2; > i'm not sure if input validation makes sense in ldso > but shifts can be tricky (hashtab[3] must be in 0..31 here) By validation do you mean mask the shift value with 0x1F ? > >> + for (h1 &= ~1; 1; sym++) { > the 1; in the middle is unnecessary > > h1 &= ~1; is problematic if signed int representation is > not two's complement > (~1 is an implementation defined negative value which is > then converted to uint32_t according to well defined rules) > > so i'd use > > for (h1 &= -2;; sym++) { > > which is probably less clear but more correct > (maybe an explicit (uint32_t)-2 cast helps with that) I'll take a closer look at the gnu_lookup algo to see if I can improve it. >> - if (h==0x6b366be && !strcmp(s, "dlopen")) rtld_used = 1; >> - if (h==0x6b3afd && !strcmp(s, "dlsym")) rtld_used = 1; >> - if (h==0x595a4cc && !strcmp(s, "__stack_chk_fail")) ssp_used = 1; >> + uint32_t computed[HASH_ALG_CNT / 32 + 1]; >> + uint32_t hashes[HASH_ALG_CNT]; >> + memset (computed, 0, sizeof (computed)); >> + if (!strcmp(s, "dlopen")) rtld_used = 1; >> + if (!strcmp(s, "dlsym")) rtld_used = 1; >> + if (!strcmp(s, "__stack_chk_fail")) ssp_used = 1; > ... >> + if (!(computed[dso->hashalg / 32] & (1 << (dso->hashalg % 32)))) { >> + h = hashalgs[dso->hashalg].hash(s); >> + hashes[dso->hashalg] = h; >> + computed[dso->hashalg / 32] |= (1 << (dso->hashalg % 32)); >> + } >> + else { >> + h = hashes[dso->hashalg]; >> + } > i think we can have more efficient code here if > only gnu and sysv hash algos are supported > (i don't think it's realistic to assume new hash algorithms > even gnu hash is a fairly useless addition to elf) This has to do with the function pointer approach. Do you prefer if/else alternative? > the if else style is > > if (cond) { > ... > } else { > ... > } > >