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] |
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 >
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] |