Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

On Fri, 2015-10-09 at 17:40 -0700, Josh Stone wrote:
> Some systems don't have loff_t, like FreeBSD where off_t always supports
> large files.  We need a standardized 64-bit signed type for the public
> header, without depending on configuration... OK, just use int64_t.

I note that this is a deliberate difference between the elfutils libelf
interface and other libelf implementations, which use off_t in the
public interface and don't have LFS support on 32bit systems. We were
lucky enough to use off64_t, then loff_t, from the start, so we always
support LFS as far as possible.

I do think that the type should kept signed to not accidentally change
any assumptions of programs using the current loff_t elements. So
int64_t is a good choice.

I don't really like that changing it to int64_t makes it less clear we
are talking about offsets here. But I don't have a good suggestion what
to do different. Maybe introduce a typedef int64_t elf_off_t? Or maybe
detect we don't have loff_t and define it ourselves somehow? But that is
not easy to do with the preprocessor.

Keeping the sys/types.h include seems correct since we need some other
types for the Elf_Arhdr struct.

Cheers,

Mark

Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Josh Stone]

[-- Attachment #1: Type: text/plain, Size: 3331 bytes --]

On 10/09/2015 05:40 PM, Josh Stone wrote:
> Some systems don't have loff_t, like FreeBSD where off_t always supports
> large files.  We need a standardized 64-bit signed type for the public
> header, without depending on configuration... OK, just use int64_t.
> 
> Signed-off-by: Josh Stone <jistone@redhat.com>

Pushed to master.

> ---
>  libelf/ChangeLog |  4 ++++
>  libelf/libelf.h  | 13 +++++++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/libelf/ChangeLog b/libelf/ChangeLog
> index b1d1ecdf11e5..4517da16c904 100644
> --- a/libelf/ChangeLog
> +++ b/libelf/ChangeLog
> @@ -1,5 +1,9 @@
>  2015-10-09  Josh Stone  <jistone@redhat.com>
>  
> +	* libelf.h: Replace loff_t with int64_t throughout.
> +
> +2015-10-09  Josh Stone  <jistone@redhat.com>
> +
>  	* libelfP.h (struct Elf): Replace off64_t with off_t.
>  	* elf_getdata_rawchunk.c (elf_getdata_rawchunk): Likewise.
>  
> diff --git a/libelf/libelf.h b/libelf/libelf.h
> index 5a2b3af856e0..54f7c29b2488 100644
> --- a/libelf/libelf.h
> +++ b/libelf/libelf.h
> @@ -29,6 +29,7 @@
>  #ifndef _LIBELF_H
>  #define _LIBELF_H 1
>  
> +#include <stdint.h>
>  #include <sys/types.h>
>  
>  /* Get the ELF types.  */
> @@ -74,7 +75,7 @@ typedef struct
>    Elf_Type d_type;		/* Type of this piece of data.  */
>    unsigned int d_version;	/* ELF version.  */
>    size_t d_size;		/* Size in bytes.  */
> -  loff_t d_off;			/* Offset into section.  */
> +  int64_t d_off;		/* Offset into section.  */
>    size_t d_align;		/* Alignment in section.  */
>  } Elf_Data;
>  
> @@ -136,7 +137,7 @@ typedef struct
>    uid_t ar_uid;			/* User ID.  */
>    gid_t ar_gid;			/* Group ID.  */
>    mode_t ar_mode;		/* File mode.  */
> -  loff_t ar_size;		/* File size.  */
> +  int64_t ar_size;		/* File size.  */
>    char *ar_rawname;		/* Original name of archive member.  */
>  } Elf_Arhdr;
>  
> @@ -177,13 +178,13 @@ extern Elf_Cmd elf_next (Elf *__elf);
>  extern int elf_end (Elf *__elf);
>  
>  /* Update ELF descriptor and write file to disk.  */
> -extern loff_t elf_update (Elf *__elf, Elf_Cmd __cmd);
> +extern int64_t elf_update (Elf *__elf, Elf_Cmd __cmd);
>  
>  /* Determine what kind of file is associated with ELF.  */
>  extern Elf_Kind elf_kind (Elf *__elf) __attribute__ ((__pure__));
>  
>  /* Get the base offset for an object file.  */
> -extern loff_t elf_getbase (Elf *__elf);
> +extern int64_t elf_getbase (Elf *__elf);
>  
>  
>  /* Retrieve file identification data.  */
> @@ -301,7 +302,7 @@ extern Elf_Data *elf_newdata (Elf_Scn *__scn);
>     would be for TYPE.  The resulting Elf_Data pointer is valid until
>     elf_end (ELF) is called.  */
>  extern Elf_Data *elf_getdata_rawchunk (Elf *__elf,
> -				       loff_t __offset, size_t __size,
> +				       int64_t __offset, size_t __size,
>  				       Elf_Type __type);
>  
>  
> @@ -313,7 +314,7 @@ extern char *elf_strptr (Elf *__elf, size_t __index, size_t __offset);
>  extern Elf_Arhdr *elf_getarhdr (Elf *__elf);
>  
>  /* Return offset in archive for current file ELF.  */
> -extern loff_t elf_getaroff (Elf *__elf);
> +extern int64_t elf_getaroff (Elf *__elf);
>  
>  /* Select archive element at OFFSET.  */
>  extern size_t elf_rand (Elf *__elf, size_t __offset);
> 

Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On Mon, 2015-10-12 at 10:44 -0700, Roland McGrath wrote:
> > I note that this is a deliberate difference between the elfutils libelf
> > interface and other libelf implementations, which use off_t in the
> > public interface and don't have LFS support on 32bit systems. We were
> > lucky enough to use off64_t, then loff_t, from the start, so we always
> > support LFS as far as possible.
> 
> Actually for early releases we used off_t in the public header but compiled
> the library with _FILE_OFFSET_BITS=64, so the ABI was broken for vanilla
> 32-bit clients of the library.

You are right. I didn't look back far enough. elfutils 0.129 and earlier
did indeed use off_t in the public headers. So not always, but only for
the last 8 years. Still a respectable time. At least a couple of years
longer than I have been involved with the project.

Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

> I note that this is a deliberate difference between the elfutils libelf
> interface and other libelf implementations, which use off_t in the
> public interface and don't have LFS support on 32bit systems. We were
> lucky enough to use off64_t, then loff_t, from the start, so we always
> support LFS as far as possible.

Actually for early releases we used off_t in the public header but compiled
the library with _FILE_OFFSET_BITS=64, so the ABI was broken for vanilla
32-bit clients of the library.

Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

> That breaks size_t all over the place, so we at least need stddef.h.

Oh, yeah.

> Then Elf_Arhdr breaks 4 more: time_t, uid_t, gid_t, and mode_t.
> I think sys/types.h is the right header for those, no?

Oh, yeah.  Forgot about that one entirely.  Yeah, <sys/types.h> is fine.
I think it's fine to assume that includes <stdint.h> too, but it's also
certainly fine to include it explicitly.

Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Josh Stone]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

On 10/09/2015 06:03 PM, Roland McGrath wrote:
> Let's drop <sys/types.h> while we're there.  Anybody broken by that (unless
> I overlooked some use of such a type in libelf.h itself) deserves to be.

That breaks size_t all over the place, so we at least need stddef.h.

Then Elf_Arhdr breaks 4 more: time_t, uid_t, gid_t, and mode_t.
I think sys/types.h is the right header for those, no?

Re: [PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 151 bytes --]

Let's drop <sys/types.h> while we're there.  Anybody broken by that (unless
I overlooked some use of such a type in libelf.h itself) deserves to be.

[PATCH v2] libelf: Use int64_t for offsets in libelf.h

[Josh Stone]

[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]

Some systems don't have loff_t, like FreeBSD where off_t always supports
large files.  We need a standardized 64-bit signed type for the public
header, without depending on configuration... OK, just use int64_t.

Signed-off-by: Josh Stone <jistone@redhat.com>
---
 libelf/ChangeLog |  4 ++++
 libelf/libelf.h  | 13 +++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index b1d1ecdf11e5..4517da16c904 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,5 +1,9 @@
 2015-10-09  Josh Stone  <jistone@redhat.com>
 
+	* libelf.h: Replace loff_t with int64_t throughout.
+
+2015-10-09  Josh Stone  <jistone@redhat.com>
+
 	* libelfP.h (struct Elf): Replace off64_t with off_t.
 	* elf_getdata_rawchunk.c (elf_getdata_rawchunk): Likewise.
 
diff --git a/libelf/libelf.h b/libelf/libelf.h
index 5a2b3af856e0..54f7c29b2488 100644
--- a/libelf/libelf.h
+++ b/libelf/libelf.h
@@ -29,6 +29,7 @@
 #ifndef _LIBELF_H
 #define _LIBELF_H 1
 
+#include <stdint.h>
 #include <sys/types.h>
 
 /* Get the ELF types.  */
@@ -74,7 +75,7 @@ typedef struct
   Elf_Type d_type;		/* Type of this piece of data.  */
   unsigned int d_version;	/* ELF version.  */
   size_t d_size;		/* Size in bytes.  */
-  loff_t d_off;			/* Offset into section.  */
+  int64_t d_off;		/* Offset into section.  */
   size_t d_align;		/* Alignment in section.  */
 } Elf_Data;
 
@@ -136,7 +137,7 @@ typedef struct
   uid_t ar_uid;			/* User ID.  */
   gid_t ar_gid;			/* Group ID.  */
   mode_t ar_mode;		/* File mode.  */
-  loff_t ar_size;		/* File size.  */
+  int64_t ar_size;		/* File size.  */
   char *ar_rawname;		/* Original name of archive member.  */
 } Elf_Arhdr;
 
@@ -177,13 +178,13 @@ extern Elf_Cmd elf_next (Elf *__elf);
 extern int elf_end (Elf *__elf);
 
 /* Update ELF descriptor and write file to disk.  */
-extern loff_t elf_update (Elf *__elf, Elf_Cmd __cmd);
+extern int64_t elf_update (Elf *__elf, Elf_Cmd __cmd);
 
 /* Determine what kind of file is associated with ELF.  */
 extern Elf_Kind elf_kind (Elf *__elf) __attribute__ ((__pure__));
 
 /* Get the base offset for an object file.  */
-extern loff_t elf_getbase (Elf *__elf);
+extern int64_t elf_getbase (Elf *__elf);
 
 
 /* Retrieve file identification data.  */
@@ -301,7 +302,7 @@ extern Elf_Data *elf_newdata (Elf_Scn *__scn);
    would be for TYPE.  The resulting Elf_Data pointer is valid until
    elf_end (ELF) is called.  */
 extern Elf_Data *elf_getdata_rawchunk (Elf *__elf,
-				       loff_t __offset, size_t __size,
+				       int64_t __offset, size_t __size,
 				       Elf_Type __type);
 
 
@@ -313,7 +314,7 @@ extern char *elf_strptr (Elf *__elf, size_t __index, size_t __offset);
 extern Elf_Arhdr *elf_getarhdr (Elf *__elf);
 
 /* Return offset in archive for current file ELF.  */
-extern loff_t elf_getaroff (Elf *__elf);
+extern int64_t elf_getaroff (Elf *__elf);
 
 /* Select archive element at OFFSET.  */
 extern size_t elf_rand (Elf *__elf, size_t __offset);
-- 
2.4.3