[PATCH 1/3] libc_elf: improve auxiliary vector handling

[Carmelo AMOROSO]

On 5/3/2011 1:39 AM, Kevin Cernekee wrote:
> On Mon, May 2, 2011 at 9:44 AM, Carmelo AMOROSO <carmelo.amoroso at st.com> wrote:
>> This patch is aimed to improve the aux vect handling to gather some useful
>> information from the system through the aux vect. It is achieved by:
> 
> Thanks for working on this - it looks like we're on the right track.
> A few cosmetic nitpicks:
> 
>> +/* Minimum number of AT entries from auxiliar vector */
> 
> "auxiliary"
> 
>> +# error "AT_EXTRA_UPTO has to be defined (0 if not extra AT entried are needed)"
> 
> "no extra AT entries"
> 
>> +/* For the ld.so we are interested to the minimum entries from the auxvect */
> 
> "interested in"
> 

I should use spell check :)

> It's a personal preference, but it might be a tiny bit more
> straightforward to define libc AT_NUM in terms of AT_EXTRA_UPTO
> directly.  e.g.
> 
> #define AT_BASE_NUM		(AT_EGID + 1)
> 
> # if AT_EXTRA_UPTO == 0
> #  define AT_LIBC_NUM           AT_BASE_NUM
> # else
> #  define AT_LIBC_NUM           (AT_EXTRA_UPTO + 1)
> # endif
> 

yes, it looks simpler.

>> +extern size_t __pagesize;
> 
> I see this getting declared as an extern in: dl-support.c, malloc.h,
> getpagesize.c, and linuxthreads.old/internals.h .
> 
> Do you think it would be a good idea to move the declaration into
> <auxvect.h> or another common header file?
> 

I don't like auxvect.h, but the idea to move into a common header is fine.

>> -   /* Get the number of program headers from the aux vect */
>> -   _dl_phnum = (size_t) av[AT_PHNUM].a_un.a_val;
>> +	for (; av->a_type < AT_NUM; ++av) {
>> +		switch (av->a_type) {
> 
> FWIW, that does result in mixed indentation styles in dl-support.c .
> 
>> +	for (; av->a_type < AT_NUM; ++av) {
> 
> ...
> 
>> +			__pagesize = (size_t) (av[AT_PAGESZ].a_un.a_val) ?
> 
> All of these cases should probably look more like "av->a_un.a_val",
> since av gets incremented on each iteration of the loop.  Without this
> fix, the patch set does not work at all for me.
> 

yes, my code is definitely wrong. I'll fix it and re-send.

> There is another problem with setting __pagesize up in _dl_aux_init()
> - it causes __uClibc_init() to skip initialization:
> 
> void __uClibc_init(void)
> {
>     /* Don't recurse */
>     if (__pagesize)
> 	return;
> 
> The most obvious symptom that I noticed was that statically linked TLS
> binaries crashed here:

> 
>     if (likely(not_null_ptr(__errno_location)))
> 	*(__errno_location()) = 0;
> 

ok, I have your patch for this and I'll merge it.

>> +#ifndef SHARED
>> +			case AT_PHDR:
>> +			   /* Get the program headers base address from the aux vect */
> 
> Maybe wouldn't hurt anything to leave these cases/variables enabled in
> the SHARED builds?
> 

for SHARED case, phdr stuff are retrieved in a different way. I'll check
it in any case.

Thanks,
Carmelo