This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Do without union of variable length arrays.


Mark,

I appreciate your efforts to make these and other changes about nested
functions. We can see soon elfutils will compile with clang/llvm on Android.
Please take a look of the new attached 0007*patch.
Thanks.


On Tue, Oct 6, 2015 at 2:25 PM, Mark Wielaard <mjw@redhat.com> wrote:

> On Mon, 2015-10-05 at 14:15 -0700, Chih-hung Hsieh wrote:
> > Please review attached 0006*patch with the new changed I mentioned
> > previously. Thanks.
>
> Very nice. Now we get to keep the array bounds.
> You forgot them in one place though:
>
> > diff --git a/libelf/elf_getarsym.c b/libelf/elf_getarsym.c
> > index 1ab94ca..f5ee3cb 100644
> > --- a/libelf/elf_getarsym.c
> > +++ b/libelf/elf_getarsym.c
> > @@ -201,11 +201,7 @@ elf_getarsym (Elf *elf, size_t *ptr)
> >        elf->state.ar.ar_sym = (Elf_Arsym *) malloc (ar_sym_len);
> >        if (elf->state.ar.ar_sym != NULL)
> >         {
> > -         union
> > -         {
> > -           uint32_t u32[n];
> > -           uint64_t u64[n];
> > -         } *file_data;
> > +         void *file_data; /* unit32_t[n] or uint64_t[n] */
> >           char *str_data;
> >           size_t sz = n * w;
> >
> > @@ -272,7 +268,7 @@ elf_getarsym (Elf *elf, size_t *ptr)
> >               arsym[cnt].as_name = str_data;
> >               if (index64_p)
> >                 {
> > -                 uint64_t tmp = file_data->u64[cnt];
> > +                 uint64_t tmp = ((uint64_t *) file_data)[cnt];
> >                   if (__BYTE_ORDER == __LITTLE_ENDIAN)
> >                     tmp = bswap_64 (tmp);
> >
> > @@ -294,9 +290,9 @@ elf_getarsym (Elf *elf, size_t *ptr)
> >                     }
> >                 }
> >               else if (__BYTE_ORDER == __LITTLE_ENDIAN)
> > -               arsym[cnt].as_off = bswap_32 (file_data->u32[cnt]);
> > +               arsym[cnt].as_off = bswap_32 (((uint32_t *)
> file_data)[cnt]);
> >               else
> > -               arsym[cnt].as_off = file_data->u32[cnt];
> > +               arsym[cnt].as_off = ((uint32_t *) file_data)[cnt];
> >
> >               arsym[cnt].as_hash = _dl_elf_hash (str_data);
> >               str_data = rawmemchr (str_data, '\0') + 1;
>
> Can you use the same pattern here so we keep the [n] bound?
>
> And one nitpick (sorry):
>
> > @@ -1013,13 +1017,17 @@ find_alloc_sections_prelink (Elf *debug,
> Elf_Data *debug_shstrtab,
> > [...]
> > +      Elf32_Shdr (*s32)[shnum - 1] = shdr;
> > +      Elf64_Shdr (*s64) [shnum - 1]= shdr;
>
>                          ^-----------^
> Move that space please.
>
> We are really there now, sorry for the somewhat long review/please
> rewrite (yet again) cycle.
>
> Thanks,
>
> Mark
>
Mark,

I appreciate your efforts to make these and other changes about nested functions. We can see soon elfutils will compile with clang/llvm on Android.
Please take a look of the new attached 0007*patch.
Thanks.


On Tue, Oct 6, 2015 at 2:25 PM, Mark Wielaard <mjw@redhat.com> wrote:
On Mon, 2015-10-05 at 14:15 -0700, Chih-hung Hsieh wrote:
> Please review attached 0006*patch with the new changed I mentioned
> previously. Thanks.

Very nice. Now we get to keep the array bounds.
You forgot them in one place though:

> diff --git a/libelf/elf_getarsym.c b/libelf/elf_getarsym.c
> index 1ab94ca..f5ee3cb 100644
> --- a/libelf/elf_getarsym.c
> +++ b/libelf/elf_getarsym.c
> @@ -201,11 +201,7 @@ elf_getarsym (Elf *elf, size_t *ptr)
>        elf->state.ar.ar_sym = (Elf_Arsym *) malloc (ar_sym_len);
>        if (elf->state.ar.ar_sym != NULL)
>         {
> -         union
> -         {
> -           uint32_t u32[n];
> -           uint64_t u64[n];
> -         } *file_data;
> +         void *file_data; /* unit32_t[n] or uint64_t[n] */
>           char *str_data;
>           size_t sz = n * w;
>
> @@ -272,7 +268,7 @@ elf_getarsym (Elf *elf, size_t *ptr)
>               arsym[cnt].as_name = str_data;
>               if (index64_p)
>                 {
> -                 uint64_t tmp = file_data->u64[cnt];
> +                 uint64_t tmp = ((uint64_t *) file_data)[cnt];
>                   if (__BYTE_ORDER == __LITTLE_ENDIAN)
>                     tmp = bswap_64 (tmp);
>
> @@ -294,9 +290,9 @@ elf_getarsym (Elf *elf, size_t *ptr)
>                     }
>                 }
>               else if (__BYTE_ORDER == __LITTLE_ENDIAN)
> -               arsym[cnt].as_off = bswap_32 (file_data->u32[cnt]);
> +               arsym[cnt].as_off = bswap_32 (((uint32_t *) file_data)[cnt]);
>               else
> -               arsym[cnt].as_off = file_data->u32[cnt];
> +               arsym[cnt].as_off = ((uint32_t *) file_data)[cnt];
>
>               arsym[cnt].as_hash = _dl_elf_hash (str_data);
>               str_data = rawmemchr (str_data, '\0') + 1;

Can you use the same pattern here so we keep the [n] bound?

And one nitpick (sorry):

> @@ -1013,13 +1017,17 @@ find_alloc_sections_prelink (Elf *debug, Elf_Data *debug_shstrtab,
> [...]
> +      Elf32_Shdr (*s32)[shnum - 1] = shdr;
> +      Elf64_Shdr (*s64) [shnum - 1]= shdr;

                         ^-----------^
Move that space please.

We are really there now, sorry for the somewhat long review/please
rewrite (yet again) cycle.

Thanks,

Mark

Attachment: 0007-Do-without-union-of-variable-length-arrays.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]