Re: ldso : dladdr support

[Szabolcs Nagy <nsz@port70.net>]

* musl <b.brezillon.musl@gmail.com> [2012-08-08 11:55:45 +0200]:
> Here is the new patch for dladdr + gnu hash support.
> 
> On 08/08/2012 01:09, Rich Felker wrote:
> > don't remember which) where I described some optimizations that should
> > be present if gnu hash is to be supported. Basically, the dynamic
> > linker should keep track of whether there's one of the two hashes
> > that's supported by ALL loaded libs, and in that case, only ever use
> > that hash, to avoid computing two different hashes.
> The hash for given algo is only computed once (if needed).
> That's the reason for the computed table.
> If all libs uses the same hash algo, the other will never be computed.

a lib can support both hash tables
(so it's non-trivial to use the one that all libs support,
eg instead of a single alg index a bit mask should be used)

> +#define SYSV_HASH_ALG_IDX   0
> +#define GNU_HASH_ALG_IDX    1
> +#define HASH_ALG_CNT		2

if we go with this approach then use shorter names
(thes are only used locally)

eg. SYSV_HASH, GNU_HASH, NHASH

> +	for (h1 &= (uint32_t)-2;; sym++) {
> +		h2 = *hashval++;
> +		if ((h1 == (h2 & ~1)) && !strcmp(s, strings + sym->st_name))

these is still a ~1

looking at it now probably writing out & 0xfffffffe
is the cleanest

> +	static uint32_t precomp[HASH_ALG_CNT][3] = {
> +	  {0x6b366be, 0x6b3afd, 0x595a4cc},
> +	  {0xf9040207, 0xf4dc4ae, 0x1f4039c9},
> +	};
> +	uint32_t h[HASH_ALG_CNT];
> +	uint8_t computed[HASH_ALG_CNT] = {0, 0};
> +	uint8_t alg = dso->hashalg;
> +	if (alg == SYSV_HASH_ALG_IDX)
> +		h[alg] = sysv_hash(s);
> +	else
> +		h[alg] = gnu_hash(s);
> +
> +	computed[alg] = 1;
> +
> +	if (h[alg] == precomp[alg][0] && !strcmp(s, "dlopen")) rtld_used = 1;
> +	if (h[alg] == precomp[alg][1] && !strcmp(s, "dlsym")) rtld_used = 1;
> +	if (h[alg] == precomp[alg][2] && !strcmp(s, "__stack_chk_fail")) ssp_used = 1;
> +
>  	for (; dso; dso=dso->next) {
>  		Sym *sym;
> +
>  		if (!dso->global) continue;
> -		sym = lookup(s, h, dso);
> +
> +		alg = dso->hashalg;
> +		if (!computed[alg]) {
> +			if (alg == SYSV_HASH_ALG_IDX) {
> +				h[alg] = sysv_hash(s);
> +				sym = sysv_lookup(s, h[alg], dso);
> +			}
> +			else {
> +				h[alg] = gnu_hash(s);
> +				sym = gnu_lookup(s, h[alg], dso);
> +			}
> +			computed[alg] = 1;
> +		} else {
> +			if (alg == SYSV_HASH_ALG_IDX)
> +				sym = sysv_lookup(s, h[alg], dso);
> +			else
> +				sym = gnu_lookup(s, h[alg], dso);
> +		}

instead of arrays i'd write

alg = dso->hashalg;
if (alg == SYSV_HASH) {
	if (sysv_ok) {
		...
		sysv_ok = 1;
	}
	sym = sysv_lookup(s, sysv_h, dso);
} else {
}

since there are many ifs anyway

the table approach is nicer when all data and functions
are in a table:

if (!ok[alg]) {
	h[alg] = hash[alg](s);
	ok[alg] = 1;
}
sym = lookup[alg](s, h[alg], dso);

> +	p->hashalg = SYSV_HASH_ALG_IDX;
>  	p->strings = (void *)(p->base + dyn[DT_STRTAB]);
> +	for (; v[0]; v+=2) if (v[0] == DT_GNU_HASH) {
> +		p->hashtab = (void *)(p->base + v[1]);
> +		p->hashalg = GNU_HASH_ALG_IDX;
> +	}

so it seems gnu hash is used whenever it's present
i'm not sure if that's the right default..

another possibility is to have a plain find_sym function
which is simple and only supports sysv hash
and whenever it encounters a lib that has no sysv hash in
it a find_sym_gnu is called that does the hard work
(so using gnu only libs is penalized, i don't know how
common that case is though)