On 5/3/2011 1:39 AM, Kevin Cernekee wrote:
> On Mon, May 2, 2011 at 9:44 AM, Carmelo AMOROSO <carmelo.amor...@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
_______________________________________________
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
