PR27725, better objcopy -p times

Message ID 20210415084201.GQ5425@bubble.grove.modra.org
State New
Headers show
Series
  • PR27725, better objcopy -p times
Related show

Commit Message

Claudiu Zissulescu via Binutils April 15, 2021, 8:42 a.m.
Nanosecond rather than second resolution.

There's a good chance this will run foul of some system oddities so I
won't be backporting this to the branch.

	PR 27725
	* configure.ac: Check for sys/time.h and utimensat.  Use standard
	checks for mkstemp and mkdtemp.  Whitespace.  Check for nanosecond
	members of struct stat.
	* rename.c: Prefer sys/time.h for utimes over utime.h for utime.
	(STAT_TIMESPEC, STAT_TIMESPEC_NS): Define
	(get_stat_atime_ns, get_stat_mtime_ns): New inline functions.
	(get_stat_atime, get_stat_mtime): Likewise.
	(set_times): Choose first available of utimensat, utimes, utime.
	Use above inline functions to set timespec and timeval values.
	* configure: Regenerate.
	* config.in: Regenerate.
	* testsuite/binutils-all/objcopy.exp (objcopy_test): Add test of
	file timestamp when --preserve-dates is used.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

Claudiu Zissulescu via Binutils April 15, 2021, 6:41 p.m. | #1
On 2021-04-15 4:42 a.m., Alan Modra wrote:
> Nanosecond rather than second resolution.

> 

> There's a good chance this will run foul of some system oddities so I

> won't be backporting this to the branch.

> 

> 	PR 27725

> 	* configure.ac: Check for sys/time.h and utimensat.  Use standard

> 	checks for mkstemp and mkdtemp.  Whitespace.  Check for nanosecond

> 	members of struct stat.

> 	* rename.c: Prefer sys/time.h for utimes over utime.h for utime.

> 	(STAT_TIMESPEC, STAT_TIMESPEC_NS): Define

> 	(get_stat_atime_ns, get_stat_mtime_ns): New inline functions.

> 	(get_stat_atime, get_stat_mtime): Likewise.

> 	(set_times): Choose first available of utimensat, utimes, utime.

> 	Use above inline functions to set timespec and timeval values.

> 	* configure: Regenerate.

> 	* config.in: Regenerate.

> 	* testsuite/binutils-all/objcopy.exp (objcopy_test): Add test of

> 	file timestamp when --preserve-dates is used.


Hi Alan,

Starting with this patch, I get:

libtool: link: ccache gcc -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -Werror -I/home/simark/src/binutils-gdb/binutils/../zlib -g3 -O0 -fsanitize=address -fmax-errors=1 -fuse-ld=gold -fsanitize=address -o ar arparse.o arlex.o ar.o not-ranlib.o arsup.o rename.o binemul.o emul_vanilla.o bucomm.o version.o filemode.o  ../bfd/.libs/libbfd.a -L/home/simark/build/binutils-gdb/zlib -lz ../libiberty/libiberty.a -lfl -ldl
/home/simark/src/binutils-gdb/binutils/rename.c:169: error: undefined reference to 'get_stat_atime'
/home/simark/src/binutils-gdb/binutils/rename.c:170: error: undefined reference to 'get_stat_mtime'

Simon
Claudiu Zissulescu via Binutils April 16, 2021, 12:16 a.m. | #2
On Thu, Apr 15, 2021 at 02:41:04PM -0400, Simon Marchi wrote:
> On 2021-04-15 4:42 a.m., Alan Modra wrote:

> > Nanosecond rather than second resolution.

> Starting with this patch, I get:

> 

> libtool: link: ccache gcc -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -Werror -I/home/simark/src/binutils-gdb/binutils/../zlib -g3 -O0 -fsanitize=address -fmax-errors=1 -fuse-ld=gold -fsanitize=address -o ar arparse.o arlex.o ar.o not-ranlib.o arsup.o rename.o binemul.o emul_vanilla.o bucomm.o version.o filemode.o  ../bfd/.libs/libbfd.a -L/home/simark/build/binutils-gdb/zlib -lz ../libiberty/libiberty.a -lfl -ldl

> /home/simark/src/binutils-gdb/binutils/rename.c:169: error: undefined reference to 'get_stat_atime'

> /home/simark/src/binutils-gdb/binutils/rename.c:170: error: undefined reference to 'get_stat_mtime'


ccache weirdness?  There are definition of those two functions a few
lines above the references.

-- 
Alan Modra
Australia Development Lab, IBM
Claudiu Zissulescu via Binutils April 16, 2021, 1:07 a.m. | #3
On 2021-04-15 8:16 p.m., Alan Modra wrote:
> On Thu, Apr 15, 2021 at 02:41:04PM -0400, Simon Marchi wrote:

>> On 2021-04-15 4:42 a.m., Alan Modra wrote:

>>> Nanosecond rather than second resolution.

>> Starting with this patch, I get:

>>

>> libtool: link: ccache gcc -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -Werror -I/home/simark/src/binutils-gdb/binutils/../zlib -g3 -O0 -fsanitize=address -fmax-errors=1 -fuse-ld=gold -fsanitize=address -o ar arparse.o arlex.o ar.o not-ranlib.o arsup.o rename.o binemul.o emul_vanilla.o bucomm.o version.o filemode.o  ../bfd/.libs/libbfd.a -L/home/simark/build/binutils-gdb/zlib -lz ../libiberty/libiberty.a -lfl -ldl

>> /home/simark/src/binutils-gdb/binutils/rename.c:169: error: undefined reference to 'get_stat_atime'

>> /home/simark/src/binutils-gdb/binutils/rename.c:170: error: undefined reference to 'get_stat_mtime'

> 

> ccache weirdness?  There are definition of those two functions a few

> lines above the references.


Hmm nope, I tried a build from scratch with ccache completely out of the
way, it still does it:

    $ readelf --syms rename.o | grep get_stat
       244: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND get_stat_atime
       245: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND get_stat_mtime

This fixes it though:

    diff --git a/binutils/rename.c b/binutils/rename.c
    index fe6019b26317..d63ab1681406 100644
    --- a/binutils/rename.c
    +++ b/binutils/rename.c
    @@ -129,7 +129,7 @@ get_stat_mtime_ns (struct stat const *st ATTRIBUTE_UNUSED)
     }

     /* Return *ST's access time.  */
    -inline struct timespec
    +static struct timespec
     get_stat_atime (struct stat const *st)
     {
     #ifdef STAT_TIMESPEC
    @@ -143,7 +143,7 @@ get_stat_atime (struct stat const *st)
     }

     /* Return *ST's data modification time.  */
    -inline struct timespec
    +static struct timespec
     get_stat_mtime (struct stat const *st)
     {
     #ifdef STAT_TIMESPEC

This is with gcc 10.2.0, btw.

Simon
Claudiu Zissulescu via Binutils April 16, 2021, 2:48 a.m. | #4
On Thu, Apr 15, 2021 at 09:07:40PM -0400, Simon Marchi wrote:
> On 2021-04-15 8:16 p.m., Alan Modra wrote:

> > ccache weirdness?  There are definition of those two functions a few

> > lines above the references.

> 

> Hmm nope, I tried a build from scratch with ccache completely out of the

> way, it still does it:

> 

> This fixes it though:

> 

>     -inline struct timespec

>     +static struct timespec


Oh, it's funny how I can read over something and autocorrect.  In this
case I meant to write "static inline".  (If anyone is wondering why,
see ISO C 6.7.4 in particular note 8.)

	PR 27725
	* rename.c (get_stat_atime, get_stat_mtime): Make static.
	(get_stat_atime_ns, get_stat_mtime_ns): Likewise.

diff --git a/binutils/rename.c b/binutils/rename.c
index fe6019b2631..544225d73f7 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -103,7 +103,7 @@ simple_copy (int fromfd, const char *to,
 #endif
 
 /* Return the nanosecond component of *ST's access time.  */
-inline long int
+static inline long int
 get_stat_atime_ns (struct stat const *st ATTRIBUTE_UNUSED)
 {
 # if defined STAT_TIMESPEC
@@ -116,7 +116,7 @@ get_stat_atime_ns (struct stat const *st ATTRIBUTE_UNUSED)
 }
 
 /* Return the nanosecond component of *ST's data modification time.  */
-inline long int
+static inline long int
 get_stat_mtime_ns (struct stat const *st ATTRIBUTE_UNUSED)
 {
 # if defined STAT_TIMESPEC
@@ -129,7 +129,7 @@ get_stat_mtime_ns (struct stat const *st ATTRIBUTE_UNUSED)
 }
 
 /* Return *ST's access time.  */
-inline struct timespec
+static inline struct timespec
 get_stat_atime (struct stat const *st)
 {
 #ifdef STAT_TIMESPEC
@@ -143,7 +143,7 @@ get_stat_atime (struct stat const *st)
 }
 
 /* Return *ST's data modification time.  */
-inline struct timespec
+static inline struct timespec
 get_stat_mtime (struct stat const *st)
 {
 #ifdef STAT_TIMESPEC

-- 
Alan Modra
Australia Development Lab, IBM
Claudiu Zissulescu via Binutils April 16, 2021, 4:47 a.m. | #5
On 2021-04-15 10:48 p.m., Alan Modra wrote:
> On Thu, Apr 15, 2021 at 09:07:40PM -0400, Simon Marchi wrote:

>> On 2021-04-15 8:16 p.m., Alan Modra wrote:

>>> ccache weirdness?  There are definition of those two functions a few

>>> lines above the references.

>>

>> Hmm nope, I tried a build from scratch with ccache completely out of the

>> way, it still does it:

>>

>> This fixes it though:

>>

>>     -inline struct timespec

>>     +static struct timespec

> 

> Oh, it's funny how I can read over something and autocorrect.  In this

> case I meant to write "static inline".  (If anyone is wondering why,

> see ISO C 6.7.4 in particular note 8.)

> 

> 	PR 27725

> 	* rename.c (get_stat_atime, get_stat_mtime): Make static.

> 	(get_stat_atime_ns, get_stat_mtime_ns): Likewise.

> 

> diff --git a/binutils/rename.c b/binutils/rename.c

> index fe6019b2631..544225d73f7 100644

> --- a/binutils/rename.c

> +++ b/binutils/rename.c

> @@ -103,7 +103,7 @@ simple_copy (int fromfd, const char *to,

>  #endif

>  

>  /* Return the nanosecond component of *ST's access time.  */

> -inline long int

> +static inline long int

>  get_stat_atime_ns (struct stat const *st ATTRIBUTE_UNUSED)

>  {

>  # if defined STAT_TIMESPEC

> @@ -116,7 +116,7 @@ get_stat_atime_ns (struct stat const *st ATTRIBUTE_UNUSED)

>  }

>  

>  /* Return the nanosecond component of *ST's data modification time.  */

> -inline long int

> +static inline long int

>  get_stat_mtime_ns (struct stat const *st ATTRIBUTE_UNUSED)

>  {

>  # if defined STAT_TIMESPEC

> @@ -129,7 +129,7 @@ get_stat_mtime_ns (struct stat const *st ATTRIBUTE_UNUSED)

>  }

>  

>  /* Return *ST's access time.  */

> -inline struct timespec

> +static inline struct timespec

>  get_stat_atime (struct stat const *st)

>  {

>  #ifdef STAT_TIMESPEC

> @@ -143,7 +143,7 @@ get_stat_atime (struct stat const *st)

>  }

>  

>  /* Return *ST's data modification time.  */

> -inline struct timespec

> +static inline struct timespec

>  get_stat_mtime (struct stat const *st)

>  {

>  #ifdef STAT_TIMESPEC

>


That works, thanks!

Simon

Patch

diff --git a/binutils/configure.ac b/binutils/configure.ac
index 3c5a8e13da3..b8ab642e068 100644
--- a/binutils/configure.ac
+++ b/binutils/configure.ac
@@ -182,24 +182,57 @@  AC_CHECK_SIZEOF([long long])
 # plugin-api.h tests HAVE_STDINT_H and HAVE_INTTYPES_H
 # Besides those, we need to check anything used in binutils/ not in C99.
 AC_CHECK_HEADERS(fcntl.h inttypes.h stdint.h sys/file.h \
-		 sys/stat.h sys/types.h unistd.h)
+		 sys/stat.h sys/time.h sys/types.h unistd.h)
 AC_HEADER_SYS_WAIT
 AC_FUNC_MMAP
-AC_CHECK_FUNCS(getc_unlocked sbrk utimes)
-AC_CHECK_FUNC([mkstemp],
-	      AC_DEFINE([HAVE_MKSTEMP], 1,
-	      [Define to 1 if you have the `mkstemp' function.]))
-AC_CHECK_FUNC([mkdtemp],
-              AC_DEFINE([HAVE_MKDTEMP], 1,
-              [Define to 1 if you have the `mkdtemp' function.]))
-  AC_MSG_CHECKING([for mbstate_t])
-  AC_TRY_COMPILE([#include <wchar.h>],
-  [mbstate_t teststate;],
-  have_mbstate_t=yes, have_mbstate_t=no)
-  AC_MSG_RESULT($have_mbstate_t)
-  if test x"$have_mbstate_t" = xyes; then
-    AC_DEFINE(HAVE_MBSTATE_T,1,[Define if mbstate_t exists in wchar.h.])
-  fi
+AC_CHECK_FUNCS(getc_unlocked mkdtemp mkstemp sbrk utimensat utimes)
+
+AC_MSG_CHECKING([for mbstate_t])
+AC_TRY_COMPILE([#include <wchar.h>],
+[mbstate_t teststate;],
+have_mbstate_t=yes, have_mbstate_t=no)
+AC_MSG_RESULT($have_mbstate_t)
+if test x"$have_mbstate_t" = xyes; then
+  AC_DEFINE(HAVE_MBSTATE_T,1,[Define if mbstate_t exists in wchar.h.])
+fi
+
+# Copied from gnulib stat-time.m4.
+# We should just switch over to using gnulib.
+AC_CHECK_MEMBERS([struct stat.st_atim.tv_nsec],
+  [AC_CACHE_CHECK([whether struct stat.st_atim is of type struct timespec],
+     [ac_cv_typeof_struct_stat_st_atim_is_struct_timespec],
+     [AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
+        [[
+          #include <sys/types.h>
+          #include <sys/stat.h>
+          #if HAVE_SYS_TIME_H
+          # include <sys/time.h>
+          #endif
+          #include <time.h>
+          struct timespec ts;
+          struct stat st;
+        ]],
+        [[
+          st.st_atim = ts;
+        ]])],
+        [ac_cv_typeof_struct_stat_st_atim_is_struct_timespec=yes],
+        [ac_cv_typeof_struct_stat_st_atim_is_struct_timespec=no])])
+   if test $ac_cv_typeof_struct_stat_st_atim_is_struct_timespec = yes; then
+     AC_DEFINE([TYPEOF_STRUCT_STAT_ST_ATIM_IS_STRUCT_TIMESPEC], [1],
+       [Define to 1 if the type of the st_atim member of a struct stat is
+        struct timespec.])
+   fi],
+  [AC_CHECK_MEMBERS([struct stat.st_atimespec.tv_nsec], [],
+     [AC_CHECK_MEMBERS([struct stat.st_atimensec], [],
+        [AC_CHECK_MEMBERS([struct stat.st_atim.st__tim.tv_nsec], [], [],
+           [#include <sys/types.h>
+            #include <sys/stat.h>])],
+        [#include <sys/types.h>
+         #include <sys/stat.h>])],
+     [#include <sys/types.h>
+      #include <sys/stat.h>])],
+  [#include <sys/types.h>
+   #include <sys/stat.h>])
 
 # Some systems have frexp only in -lm, not in -lc.
 AC_SEARCH_LIBS(frexp, m)
diff --git a/binutils/rename.c b/binutils/rename.c
index 969acc12b30..2cbe46497ab 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -22,10 +22,10 @@ 
 #include "bfd.h"
 #include "bucomm.h"
 
-#ifdef HAVE_GOOD_UTIME_H
-#include <utime.h>
-#elif defined HAVE_UTIMES
+#if defined HAVE_UTIMES
 #include <sys/time.h>
+#elif defined HAVE_GOOD_UTIME_H
+#include <utime.h>
 #endif
 
 /* The number of bytes to copy at once.  */
@@ -86,6 +86,77 @@  simple_copy (int fromfd, const char *to,
   return 0;
 }
 
+/* The following defines and inline functions are copied from gnulib.
+   FIXME: Use a gnulib import and stat-time.h instead.  */
+#if defined HAVE_STRUCT_STAT_ST_ATIM_TV_NSEC
+# if defined TYPEOF_STRUCT_STAT_ST_ATIM_IS_STRUCT_TIMESPEC
+#  define STAT_TIMESPEC(st, st_xtim) ((st)->st_xtim)
+# else
+#  define STAT_TIMESPEC_NS(st, st_xtim) ((st)->st_xtim.tv_nsec)
+# endif
+#elif defined HAVE_STRUCT_STAT_ST_ATIMESPEC_TV_NSEC
+# define STAT_TIMESPEC(st, st_xtim) ((st)->st_xtim##espec)
+#elif defined HAVE_STRUCT_STAT_ST_ATIMENSEC
+# define STAT_TIMESPEC_NS(st, st_xtim) ((st)->st_xtim##ensec)
+#elif defined HAVE_STRUCT_STAT_ST_ATIM_ST__TIM_TV_NSEC
+# define STAT_TIMESPEC_NS(st, st_xtim) ((st)->st_xtim.st__tim.tv_nsec)
+#endif
+
+/* Return the nanosecond component of *ST's access time.  */
+inline long int
+get_stat_atime_ns (struct stat const *st)
+{
+# if defined STAT_TIMESPEC
+  return STAT_TIMESPEC (st, st_atim).tv_nsec;
+# elif defined STAT_TIMESPEC_NS
+  return STAT_TIMESPEC_NS (st, st_atim);
+# else
+  return 0;
+# endif
+}
+
+/* Return the nanosecond component of *ST's data modification time.  */
+inline long int
+get_stat_mtime_ns (struct stat const *st)
+{
+# if defined STAT_TIMESPEC
+  return STAT_TIMESPEC (st, st_mtim).tv_nsec;
+# elif defined STAT_TIMESPEC_NS
+  return STAT_TIMESPEC_NS (st, st_mtim);
+# else
+  return 0;
+# endif
+}
+
+/* Return *ST's access time.  */
+inline struct timespec
+get_stat_atime (struct stat const *st)
+{
+#ifdef STAT_TIMESPEC
+  return STAT_TIMESPEC (st, st_atim);
+#else
+  struct timespec t;
+  t.tv_sec = st->st_atime;
+  t.tv_nsec = get_stat_atime_ns (st);
+  return t;
+#endif
+}
+
+/* Return *ST's data modification time.  */
+inline struct timespec
+get_stat_mtime (struct stat const *st)
+{
+#ifdef STAT_TIMESPEC
+  return STAT_TIMESPEC (st, st_mtim);
+#else
+  struct timespec t;
+  t.tv_sec = st->st_mtime;
+  t.tv_nsec = get_stat_mtime_ns (st);
+  return t;
+#endif
+}
+/* End FIXME.  */
+
 /* Set the times of the file DESTINATION to be the same as those in
    STATBUF.  */
 
@@ -93,20 +164,25 @@  void
 set_times (const char *destination, const struct stat *statbuf)
 {
   int result;
-#ifdef HAVE_GOOD_UTIME_H
-  struct utimbuf tb;
-
-  tb.actime = statbuf->st_atime;
-  tb.modtime = statbuf->st_mtime;
-  result = utime (destination, &tb);
+#if defined HAVE_UTIMENSAT
+  struct timespec times[2];
+  times[0] = get_stat_atime (statbuf);
+  times[1] = get_stat_mtime (statbuf);
+  result = utimensat (AT_FDCWD, destination, times, 0);
 #elif defined HAVE_UTIMES
   struct timeval tv[2];
 
   tv[0].tv_sec = statbuf->st_atime;
-  tv[0].tv_usec = 0;
+  tv[0].tv_usec = get_stat_atime_ns (statbuf) / 1000;
   tv[1].tv_sec = statbuf->st_mtime;
-  tv[1].tv_usec = 0;
+  tv[1].tv_usec = get_stat_mtime_ns (statbuf) / 1000;
   result = utimes (destination, tv);
+#elif defined HAVE_GOOD_UTIME_H
+  struct utimbuf tb;
+
+  tb.actime = statbuf->st_atime;
+  tb.modtime = statbuf->st_mtime;
+  result = utime (destination, &tb);
 #else
   long tb[2];
 
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index ef251876a02..e1df9ff748d 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -77,6 +77,7 @@  proc objcopy_test {testname srcfile type asflags ldflags} {
 	    return
 	}
 	set xflags "--preserve-dates"
+	sleep 1
     }
 
     set got [binutils_run $OBJCOPY "$OBJCOPYFLAGS $xflags $t_tempfile $t_copyfile"]
@@ -102,6 +103,17 @@  proc objcopy_test {testname srcfile type asflags ldflags} {
 
 	if [string equal "" $exec_output] then {
 	    pass "objcopy $type ($testname)"
+	    if { $type == "executable" } {
+		set dir [file dirname $t_copyfile]
+		set f2 [file tail $t_copyfile]
+		set status [remote_exec host find "$dir -name $f2 -newer $t_tempfile -print"]
+		set exec_output [lindex $status 1]
+		if [string equal "" $exec_output] then {
+		    pass "objcopy $type ($testname) timestamp"
+		} else {
+		    fail "objcopy $type ($testname) timestamp"
+		}
+	    }
 	} else {
 	    send_log "$exec_output\n"
 	    verbose "$exec_output" 1