Re: Support archives with 64-bit symbol table

[Mark Wielaard <mjw at redhat dot com>]

On Thu, Aug 02, 2012 at 01:43:10AM +0200, Petr Machata wrote:
> these two patches implement support for 64-bit symbol table in .a
> archives.  As of recently, binutils' ar produces such archives on s390x
> (regardless of actual archive size).  This appears to be the same as
> "/", except all fields are 64-bit.

Just a quick partial review, with some pedantic comments. Have to read
up on ar archives. But it looks correct to me. Are the magic entries
("/SYM64/") described somewhere in a specification document or is this
GNU specific?

> From 37118707e4974c6bc95d2e3d7fbf59ac1e1912c3 Mon Sep 17 00:00:00 2001
> From: Petr Machata <pmachata@redhat.com>
> Date: Wed, 1 Aug 2012 21:37:52 +0200
> Subject: [PATCH 1/2] Implement support for archives with 64-bit symbol table
> diff --git a/libelf/ChangeLog b/libelf/ChangeLog
> index 8c9ff8b..18ada85 100644
> --- a/libelf/ChangeLog
> +++ b/libelf/ChangeLog
> @@ -1,3 +1,11 @@
> +2012-08-01  Petr Machata  <pmachata@redhat.com>
> +
> +	* elf_getarsym (read_number_entries): New function.
> +	(elf_getarsym): Handle 64-bit symbol table, stored in special
> +	entry named "/SYM64/".
> +	* elf_begin.c (__libelf_next_arhdr_wrlock): Don't reject archive
> +	because it contains 64-bit symbol table.
> +
> [...]
> --- a/libelf/elf_getarsym.c
> +++ b/libelf/elf_getarsym.c
> @@ -1,5 +1,5 @@
>  /* Return symbol table of archive.
> -   Copyright (C) 1998, 1999, 2000, 2002, 2005 Red Hat, Inc.
> +   Copyright (C) 1998-2012 Red Hat, Inc.

This should be 1998-2000, 2002, 2005, 2012. Only the first is a full
range. Similarly in some other places. (Yes, I did ask a lawyer.)

> -      /* Now test whether this is the index.  It is denoted by the
> -	 name being "/ ".
> +      bool index64_p;
> +      /* Now test whether this is the index.  If the name is "/", this
> +	 is 32-bit index, if it's "/SYM64/", it's 64-bit index.
> +
>  	 XXX This is not entirely true.  There are some more forms.
>  	 Which of them shall we handle?  */

Is this still true? Which other forms are there?

> +		  /* Check whether 64-bit offset fits into 32-bit
> +		     size_t.  */
> +		  if (sizeof (arsym[cnt].as_off) < 8
> +		      && arsym[cnt].as_off != tmp)
> +		    {
> +		      if (elf->map_address == NULL)
> +			{
> +			  free (elf->state.ar.ar_sym);
> +			  elf->state.ar.ar_sym = NULL;
> +			}
> +
> +		      __libelf_seterrno (ELF_E_RANGE);
> +		      goto out;
> +		    }

Urgh. How messy, all these public size_t arguments and fields :{
But this is the best we can do on a 32-bit arch. nice.

> From ca57cfba6db5d7122c1bcccd3985ea4d09493df6 Mon Sep 17 00:00:00 2001
> From: Petr Machata <pmachata@redhat.com>
> Date: Wed, 1 Aug 2012 21:41:36 +0200
> Subject: [PATCH 2/2] Test case for handling archives with 64-bit symbol table
> [...]
> diff --git a/tests/archive64.a.bz2 b/tests/archive64.a.bz2
> new file mode 100644
> index 0000000000000000000000000000000000000000..a8dc081ceb0d17fd75de3d516b549e766f23af2c
> [...] 
> diff --git a/tests/test-archive64.sh b/tests/test-archive64.sh
> new file mode 100755
> index 0000000..f9d1cc6
> --- /dev/null
> +++ b/tests/test-archive64.sh

It would be more consistent to cal this run-test-archive64.sh.

> +
> +testfiles archive64.a

Please document here how archive64.a was created.

Thanks,

Mark