Re: separated debuginfo patch

Re: separated debuginfo patch

[Nick Clifton]

Hi Philippe,

> cc'ing binutils list

 [And adding gdb since this affects how debuginfo files are
 processed].
 
> hi, we was discussing about the separate debug info files.
> I've some doubt than crc is the right way to check for debug
> file match.

> Changed my mind, now I think a timestamp is better.
> This sort of problem is generally solved by a timestamp
> (64 bits) not a crc, both file contain the same timestamp and
> the timestamp must be equal, imho crc is usefull to check file
> corruption not file matching

Although by having a crc you can do both at the same time.

> crc *can*(1) be here if someone want to check if the debug file
> is corrupted but this must be a client decision to check the
> crc not a bfd one. With the current scheme I'm unusure than an
> operport -l will work enough well since we can get easily a few
> GB of debug info leading to an opreport slower by a few magnitude
> order. It can be usefull for gdb itself, what will occur if I try
> to get a bt for OpenOffice and it's 1.2 GB of debug info in separate
> debug file ?
>
> doing it in this way seems feasible:
>
> objcopy --add-gnu-link will add a timestamp section in both
> debug file and binary which can be used to check file match
> by bfd lib.
>
> BFD maintainers is this scheme sensible?

The problem binutils has is that the debuginfo file and .gnu-debuginfo
section in the stripped file are created by two different steps (1).
Thus adding the same timestamp to both files is tricky and would
require that the debuginfo file be first created with a blank
timestamp section and then later, when the .gnu-debuginfo section is
added, the timestamp is initialised.

The other problem with timestamps is that it means that bootstrap
sequences that involve the creation of debuginfo file will no longer
work.  ie the debuginfo files and stripped executables from two
different bootstrap stages will no longer compare as identical because
of the timestamps.

This problem could be avoided by using a unique-binary-identifier that
is specified on the command line, rather than a timestamp that is
generated by the objcopy/strip program.  ie objcopy could have a new
switch such as:

  --set-debuglink-timestamp=<number>

If this switch is not specified when --add-gnu-debuglink is used, then
a timestamp is added, otherwise <number> is added.  Generating a
non-trivial value for <number> however might prove difficult...


Still, I am not totally opposed to the idea, and if the gdb guys agree
I would be happy to consider a patch adding these features.  (I am a
bit swamped at the moment, so I do not have time to create the patch
myself).  It should not be too hard to do.

We just need to agree on the format and name of the timestamp section
in the debuginfo file, and how to distinguish old .gnu-debuglink
sections (only containing a crc) from new ones (containing a crc and a
timestamp).

Possibilities for the new section in the debuginfo file include:

  .gnu_debuglink  (ie reuse the section name that is found in the
                   stripped executable.  The version in the debuginfo
                   file could be exactly the same format, or it could
                   be a shortened version with only the timestamp and
                   not the filename or crc).
                   
  .gnu_debuglink_timestamp  (a bit wordy, but self documenting)

  .note.gnu.debuglink.timestamp (even longer, but who's counting ? :-)

 
Cheers
        Nick
        
(1) This was a deliberate choice on my part since the strip and
    objcopy tools historically only ever have one output file.  I could
    have changed their internals to handle multiple outputs, but this
    was more work than I wanted to do, and I believe in keeping things
    simple unless forced to add complications.
    

Re: separated debuginfo patch

[Philippe Elie]

Nick Clifton wrote:
> Hi Philippe,

[trying to avoid crc'ing the separate debug file]

I need to know how GDB guys want I deal with the gdb part, for now
gdb.diff just remove (#if 0) all duplicated code from bfd and use
bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it
ok to remove this code or must I update the duplicated code according
to the change in bfd ?


> The problem binutils has is that the debuginfo file and .gnu-debuginfo
> section in the stripped file are created by two different steps (1).
> Thus adding the same timestamp to both files is tricky and would
> require that the debuginfo file be first created with a blank
> timestamp section and then later, when the .gnu-debuginfo section is
> added, the timestamp is initialised.

I avoided it by forcing user to specify the timestamp in both step

> 
> The other problem with timestamps is that it means that bootstrap
> sequences that involve the creation of debuginfo file will no longer
> work.  ie the debuginfo files and stripped executables from two
> different bootstrap stages will no longer compare as identical because
> of the timestamps.
> 
> This problem could be avoided by using a unique-binary-identifier that
> is specified on the command line, rather than a timestamp that is
> generated by the objcopy/strip program.  ie objcopy could have a new
> switch such as:
> 
>   --set-debuglink-timestamp=<number>
> 
> If this switch is not specified when --add-gnu-debuglink is used, then
> a timestamp is added, otherwise <number> is added.  Generating a
> non-trivial value for <number> however might prove difficult...

I like it, such number is packager responsiblility, for gnu
based system `date +%s` should be sufficient. I changed slightly
it, number is not optionnal and must be provided by caller, is
it a real problem ?


> We just need to agree on the format and name of the timestamp section
> in the debuginfo file, and how to distinguish old .gnu-debuglink
> sections (only containing a crc) from new ones (containing a crc and a
> timestamp).

I disambiguite the two case by looking the section size. See
bfd.diff, it's a bit ugly but I don't see how I can deal cleanly
with this problem.

> 
> Possibilities for the new section in the debuginfo file include:
> 
>   .gnu_debuglink  (ie reuse the section name that is found in the
>                    stripped executable.  The version in the debuginfo
>                    file could be exactly the same format, or it could
>                    be a shortened version with only the timestamp and
>                    not the filename or crc).
>                    
>   .gnu_debuglink_timestamp  (a bit wordy, but self documenting)

I tried to re-use exactly the same format but putting a valid
filename in the .gnu_debuglink in debug info file conduct gdb
into an infinite loop so on I preferred to play safe and use
a separate section.

The patch is a RFC rather than a submission, it needs some cleanup,
updating bfd documentation etc. btw bfd documentation gives two
alternative to use this feature but the first doesn't works with or
w/o the attached patch:

   @item Run @code{objcopy --strip-debug foo} to create a
   stripped executable.
   @item Run @code{objcopy --add-gnu-debuglink=foo.dbg foo}
   to add a link to the debugging info into the stripped executable.

gdb doesn't like file produced in this way and gdb document
only the second way which works

   @item Copy @code{foo} to  @code{foo.full}
   @item Run @code{objcopy --strip-debug foo}
   @item Run @code{objcopy --add-gnu-debuglink=foo.full foo}

The patch is tested with gdb code with and w/o gdeb.diff applied,
it works transparentely for gdb except obviously the old code
always does the crc.

patch can be tested through this two ways:

gcc -g -O2 test.cpp
cp a.out a.out.dbg
strip --strip-unneeded a.out
objcopy --set-debuglink-timestamp=12 a.out.dbg
objcopy --set-debuglink-timestamp=12 --add-gnu-debuglink=a.out.dbg a.out

gcc -g -O2 test.cpp
cp a.out a.out.dbg
strip --strip-unneeded a.out
objcopy --add-gnu-debuglink=a.out.dbg a.out

the first is now the preferred way. I don't like at all
than gdb need the full file it makes separate debug info
less usefull, can a GDB wizard gives some clues if it's
feasible to use the file stripped from it's code/data
section and if it'll be hard to implement ?

regards,
Philippe Elie

Re: separated debuginfo patch

[Philippe Elie]

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

Philippe Elie wrote:
> Nick Clifton wrote:
> 
>> Hi Philippe,
> 


sorry, the missing patch ...

regards,
Phil

[-- Attachment #2: bfd.diff --]
[-- Type: text/plain, Size: 14208 bytes --]

Index: bfd-in2.h
===================================================================
RCS file: /cvs/src/src/bfd/bfd-in2.h,v
retrieving revision 1.226
diff -c -3 -p -r1.226 bfd-in2.h
*** bfd-in2.h	1 Jul 2003 14:44:59 -0000	1.226
--- bfd-in2.h	4 Jul 2003 21:18:00 -0000
*************** bfd_boolean bfd_make_readable (bfd *abfd
*** 853,865 ****
  unsigned long bfd_calc_gnu_debuglink_crc32
     (unsigned long crc, const unsigned char *buf, bfd_size_type len);
  
  char *bfd_follow_gnu_debuglink (bfd *abfd, const char *dir);
  
  struct sec *bfd_create_gnu_debuglink_section
     (bfd *abfd, const char *filename);
  
  bfd_boolean bfd_fill_in_gnu_debuglink_section
!    (bfd *abfd, struct sec *sect, const char *filename);
  
  /* Extracted from libbfd.c.  */
  
--- 853,874 ----
  unsigned long bfd_calc_gnu_debuglink_crc32
     (unsigned long crc, const unsigned char *buf, bfd_size_type len);
  
+ char *bfd_get_debug_link_info 
+    (bfd *abfd, unsigned long *crc32_out, unsigned long *timestamp);
+ 
  char *bfd_follow_gnu_debuglink (bfd *abfd, const char *dir);
  
  struct sec *bfd_create_gnu_debuglink_section
     (bfd *abfd, const char *filename);
  
  bfd_boolean bfd_fill_in_gnu_debuglink_section
!    (bfd *abfd, struct sec *sect, const char *filename,
!     unsigned long timestamp);
! 
! struct sec *bfd_create_gnu_debuglink_timestamp_section (bfd *abfd);
! 
! bfd_boolean bfd_fill_in_gnu_debuglink_timestamp_section
!    (bfd *abfd, struct sec *sect, unsigned long timestamp);
  
  /* Extracted from libbfd.c.  */
  
Index: opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.18
diff -c -3 -p -r1.18 opncls.c
*** opncls.c	29 Jun 2003 10:06:39 -0000	1.18
--- opncls.c	4 Jul 2003 21:18:02 -0000
*************** bfd_release (bfd *abfd, void *block)
*** 692,697 ****
--- 692,698 ----
  */
  
  #define GNU_DEBUGLINK	".gnu_debuglink"
+ #define GNU_DEBUGLINK_TIMESTAMP ".gnu_debuglink_timestamp"
  /*
  FUNCTION
  	bfd_calc_gnu_debuglink_crc32
*************** bfd_calc_gnu_debuglink_crc32 (unsigned l
*** 779,798 ****
  
  
  /*
! INTERNAL_FUNCTION
! 	get_debug_link_info
  
  SYNOPSIS
! 	char *get_debug_link_info (bfd *abfd, unsigned long *crc32_out);
  
  DESCRIPTION
! 	fetch the filename and CRC32 value for any separate debuginfo
! 	associated with @var{abfd}. Return NULL if no such info found,
! 	otherwise return filename and update @var{crc32_out}.
  */
  
! static char *
! get_debug_link_info (bfd *abfd, unsigned long *crc32_out)
  {
    asection * sect;
    bfd_size_type debuglink_size;
--- 780,803 ----
  
  
  /*
! FUNCTION
! 	bfd_get_debug_link_info
  
  SYNOPSIS
! 	char *bfd_get_debug_link_info 
! 	  (bfd *abfd, unsigned long *crc32_out, unsigned long *timestamp);
  
  DESCRIPTION
! 	fetch the filename, CRC32 value and 32 bits timestamp for any
! 	separate debuginfo associated with @var{abfd}. Return NULL if no such
! 	info found, otherwise return filename, update @var{crc32_out} and
! 	@var{timestamp}. If timestamp is not available return 0 in
! 	@var{timestamp}
  */
  
! char *
! bfd_get_debug_link_info (bfd *abfd, unsigned long *crc32_out, 
! 			 unsigned long *timestamp)
  {
    asection * sect;
    bfd_size_type debuglink_size;
*************** get_debug_link_info (bfd *abfd, unsigned
*** 803,808 ****
--- 808,814 ----
  
    BFD_ASSERT (abfd);
    BFD_ASSERT (crc32_out);
+   BFD_ASSERT (timestamp);
  
    sect = bfd_get_section_by_name (abfd, GNU_DEBUGLINK);
  
*************** get_debug_link_info (bfd *abfd, unsigned
*** 828,852 ****
  
    crc32 = bfd_get_32 (abfd, contents + crc_offset);
  
    *crc32_out = crc32;
    return contents;
  }
  
  /*
  INTERNAL_FUNCTION
  	separate_debug_file_exists
  
  SYNOPSIS
  	bfd_boolean separate_debug_file_exists
! 	  (char *name, unsigned long crc32);
  
  DESCRIPTION
  	Checks to see if @var{name} is a file and if its contents
! 	match @var{crc32}.
  */
  
  static bfd_boolean
! separate_debug_file_exists (const char *name, const unsigned long crc)
  {
    static char buffer [8 * 1024];
    unsigned long file_crc = 0;
--- 834,920 ----
  
    crc32 = bfd_get_32 (abfd, contents + crc_offset);
  
+   /* .gnu_debuglink format changed by adding a timestamp value after the
+    * the crc, we must support the old format */
+   *timestamp = 0;
+   if (debuglink_size - (crc_offset + 4) >= 4)
+     *timestamp = bfd_get_32 (abfd, contents + crc_offset + 4);
+ 
    *crc32_out = crc32;
    return contents;
  }
  
  /*
  INTERNAL_FUNCTION
+ 	bfd_get_debug_timestamp_info
+ 
+ SYNOPSIS
+ 	bfd_boolean bfd_get_debug_timestamp_info
+ 	  (bfd *abfd, unsigned long *timestamp);
+ 
+ DESCRIPTION
+ 	fetch the timestamp for any separate debuginfo associated with
+ 	@var{abfd}.
+ 
+ RETURNS
+ 	<<TRUE>> is returned if all is ok, otherwise <<FALSE>>.
+ */
+ 
+ static bfd_boolean
+ bfd_get_debug_timestamp_info (bfd *abfd,  unsigned long *timestamp)
+ {
+   asection * sect;
+   bfd_size_type timestamp_size;
+   bfd_boolean ret;
+   char * contents;
+ 
+   BFD_ASSERT (abfd);
+   BFD_ASSERT (timestamp);
+ 
+   sect = bfd_get_section_by_name (abfd, GNU_DEBUGLINK_TIMESTAMP);
+ 
+   if (sect == NULL)
+     return FALSE;
+ 
+   timestamp_size = bfd_section_size (abfd, sect);  
+ 
+   if (timestamp_size != 4)
+     return FALSE;
+ 
+   contents = malloc (timestamp_size);
+   if (contents == NULL)
+     return FALSE;
+ 
+   ret = bfd_get_section_contents (abfd, sect, contents, 0, timestamp_size);
+   if (! ret)
+     {
+       free (contents);
+       return FALSE;
+     }
+ 
+   *timestamp = bfd_get_32 (abfd, contents);
+ 
+   free (contents);
+ 
+   return TRUE;
+ }
+ 
+ /*
+ INTERNAL_FUNCTION
  	separate_debug_file_exists
  
  SYNOPSIS
  	bfd_boolean separate_debug_file_exists
! 	  (char *name, unsigned long crc32, unsigned long timestamp);
  
  DESCRIPTION
  	Checks to see if @var{name} is a file and if its contents
! 	match @var{timestamp} or if the @var{crc32} match.
  */
  
  static bfd_boolean
! separate_debug_file_exists (const char *name, const unsigned long crc,
! 			    unsigned long timestamp)
  {
    static char buffer [8 * 1024];
    unsigned long file_crc = 0;
*************** separate_debug_file_exists (const char *
*** 855,860 ****
--- 923,954 ----
  
    BFD_ASSERT (name);
  
+   printf("try %s %ld\n", name, timestamp);
+ 
+   if (timestamp)
+     {
+       bfd *abfd = bfd_openr(name, NULL);
+ 
+       if (abfd)
+ 	{
+ 	  char ** matching;
+ 
+ 	  if (bfd_check_format_matches(abfd, bfd_object, &matching))
+ 	    {
+ 	      unsigned long temp_timestamp;
+ 
+ 	      if (bfd_get_debug_timestamp_info (abfd, &temp_timestamp))
+ 		{
+ 		  bfd_close(abfd);
+ 		  return temp_timestamp == timestamp;
+ 		}
+ 	      bfd_close (abfd);
+ 	    }
+ 	}
+     }
+ 
+   printf("doing full crc\n");
+ 
    fd = open (name, O_RDONLY);
    if (fd < 0)
      return FALSE;
*************** find_separate_debug_file (bfd *abfd, con
*** 891,896 ****
--- 985,991 ----
    char *dir;
    char *debugfile;
    unsigned long crc32;
+   unsigned long timestamp;
    int i;
  
    BFD_ASSERT (abfd);
*************** find_separate_debug_file (bfd *abfd, con
*** 901,907 ****
    if (! abfd->filename)
      return NULL;
  
!   basename = get_debug_link_info (abfd, & crc32);
    if (basename == NULL)
      return NULL;
  
--- 996,1002 ----
    if (! abfd->filename)
      return NULL;
  
!   basename = bfd_get_debug_link_info (abfd, &crc32, &timestamp);
    if (basename == NULL)
      return NULL;
  
*************** find_separate_debug_file (bfd *abfd, con
*** 943,949 ****
    strcpy (debugfile, dir);
    strcat (debugfile, basename);
  
!   if (separate_debug_file_exists (debugfile, crc32))
      {
        free (basename);
        free (dir);
--- 1038,1044 ----
    strcpy (debugfile, dir);
    strcat (debugfile, basename);
  
!   if (separate_debug_file_exists (debugfile, crc32, timestamp))
      {
        free (basename);
        free (dir);
*************** find_separate_debug_file (bfd *abfd, con
*** 955,961 ****
    strcat (debugfile, ".debug/");
    strcat (debugfile, basename);
  
!   if (separate_debug_file_exists (debugfile, crc32))
      {
        free (basename);
        free (dir);
--- 1050,1056 ----
    strcat (debugfile, ".debug/");
    strcat (debugfile, basename);
  
!   if (separate_debug_file_exists (debugfile, crc32, timestamp))
      {
        free (basename);
        free (dir);
*************** find_separate_debug_file (bfd *abfd, con
*** 972,978 ****
    strcat (debugfile, dir);
    strcat (debugfile, basename);
  
!   if (separate_debug_file_exists (debugfile, crc32))
      {
        free (basename);
        free (dir);
--- 1067,1073 ----
    strcat (debugfile, dir);
    strcat (debugfile, basename);
  
!   if (separate_debug_file_exists (debugfile, crc32, timestamp))
      {
        free (basename);
        free (dir);
*************** bfd_create_gnu_debuglink_section (bfd *a
*** 1073,1079 ****
    debuglink_size = strlen (filename) + 1;
    debuglink_size += 3;
    debuglink_size &= ~3;
!   debuglink_size += 4;
  
    if (! bfd_set_section_size (abfd, sect, debuglink_size))
      /* XXX Should we delete the section from the bfd ?  */
--- 1168,1174 ----
    debuglink_size = strlen (filename) + 1;
    debuglink_size += 3;
    debuglink_size &= ~3;
!   debuglink_size += 8;
  
    if (! bfd_set_section_size (abfd, sect, debuglink_size))
      /* XXX Should we delete the section from the bfd ?  */
*************** FUNCTION
*** 1089,1095 ****
  
  SYNOPSIS
  	bfd_boolean bfd_fill_in_gnu_debuglink_section
! 	  (bfd *abfd, struct sec *sect, const char *filename);
  
  DESCRIPTION
  
--- 1184,1191 ----
  
  SYNOPSIS
  	bfd_boolean bfd_fill_in_gnu_debuglink_section
! 	  (bfd *abfd, struct sec *sect, const char *filename,
! 	   unsigned long timestamp);
  
  DESCRIPTION
  
*************** RETURNS
*** 1106,1112 ****
  bfd_boolean
  bfd_fill_in_gnu_debuglink_section (bfd *abfd,
  				   struct sec *sect,
! 				   const char *filename)
  {
    bfd_size_type debuglink_size;
    unsigned long crc32;
--- 1202,1209 ----
  bfd_boolean
  bfd_fill_in_gnu_debuglink_section (bfd *abfd,
  				   struct sec *sect,
! 				   const char *filename,
! 				   unsigned long timestamp)
  {
    bfd_size_type debuglink_size;
    unsigned long crc32;
*************** bfd_fill_in_gnu_debuglink_section (bfd *
*** 1147,1153 ****
    debuglink_size = strlen (filename) + 1;
    debuglink_size += 3;
    debuglink_size &= ~3;
!   debuglink_size += 4;
  
    contents = malloc (debuglink_size);
    if (contents == NULL)
--- 1244,1250 ----
    debuglink_size = strlen (filename) + 1;
    debuglink_size += 3;
    debuglink_size &= ~3;
!   debuglink_size += 8;
  
    contents = malloc (debuglink_size);
    if (contents == NULL)
*************** bfd_fill_in_gnu_debuglink_section (bfd *
*** 1158,1168 ****
      }
  
    strcpy (contents, filename);
!   crc_offset = debuglink_size - 4;
  
    bfd_put_32 (abfd, crc32, contents + crc_offset);
  
    if (! bfd_set_section_contents (abfd, sect, contents, 0, debuglink_size))
      {
        /* XXX Should we delete the section from the bfd ?  */
        free (contents);
--- 1255,1373 ----
      }
  
    strcpy (contents, filename);
!   crc_offset = debuglink_size - 8;
  
    bfd_put_32 (abfd, crc32, contents + crc_offset);
+   bfd_put_32 (abfd, timestamp, contents + crc_offset + 4);
  
    if (! bfd_set_section_contents (abfd, sect, contents, 0, debuglink_size))
+     {
+       /* XXX Should we delete the section from the bfd ?  */
+       free (contents);
+       return FALSE;
+     }
+ 
+   return TRUE;
+ }
+ 
+ /*
+ FUNCTION
+ 	bfd_create_gnu_debuglink_timestamp_section
+ 
+ SYNOPSIS
+ 	struct sec *bfd_create_gnu_debuglink_timestamp_section (bfd *abfd);
+ 
+ DESCRIPTION
+ 
+ 	Takes a @var{BFD} and adds a .gnu_debuglink_time_stamp section to it.
+ 	The section is sized to be big enough to contain a 32 bits timestamp
+ 	value.
+ 
+ RETURNS
+ 	A pointer to the new section is returned if all is ok.  Otherwise <<NULL>> is
+ 	returned and bfd_error is set.  
+ */
+ 
+ asection *
+ bfd_create_gnu_debuglink_timestamp_section (bfd *abfd)
+ {
+   asection *sect;
+ 
+   if (abfd == NULL)
+     {
+       bfd_set_error (bfd_error_invalid_operation);
+       return NULL;
+     }
+ 
+   sect = bfd_get_section_by_name (abfd, GNU_DEBUGLINK_TIMESTAMP);
+   if (sect)
+     {
+       /* Section already exists.  */
+       bfd_set_error (bfd_error_invalid_operation);
+       return NULL;
+     }
+ 
+   sect = bfd_make_section (abfd, GNU_DEBUGLINK_TIMESTAMP);
+   if (sect == NULL)
+     return NULL;
+ 
+   if (! bfd_set_section_flags (abfd, sect,
+ 			       SEC_HAS_CONTENTS | SEC_READONLY | SEC_DEBUGGING))
+     /* XXX Should we delete the section from the bfd ?  */
+     return NULL;
+ 
+   
+   if (! bfd_set_section_size (abfd, sect, 4))
+     /* XXX Should we delete the section from the bfd ?  */
+     return NULL;
+   
+   return sect;
+ }
+ 
+ 
+ /*
+ FUNCTION
+ 	bfd_fill_in_gnu_debuglink_timestamp_section
+ 
+ SYNOPSIS
+ 	bfd_boolean bfd_fill_in_gnu_debuglink_timestamp_section
+ 	  (bfd *abfd, struct sec *sect, unsigned long timestamp);
+ 
+ DESCRIPTION
+ 
+ 	Takes a @var{BFD} and containing a .gnu_debuglink section @var{SECT}
+ 	and fills in the contents of the section to contain a link to the
+ 	specified @var{filename}.  The filename should be relative to the
+ 	current directory.
+ 
+ RETURNS
+ 	<<TRUE>> is returned if all is ok.  Otherwise <<FALSE>> is returned
+ 	and bfd_error is set.  
+ */
+ 
+ bfd_boolean
+ bfd_fill_in_gnu_debuglink_timestamp_section (bfd *abfd, struct sec *sect,
+ 					     unsigned long timestamp)
+ {
+   char * contents;
+ 
+   if (abfd == NULL || sect == NULL)
+     {
+       bfd_set_error (bfd_error_invalid_operation);
+       return FALSE;
+     }
+ 
+   contents = malloc (4);
+   if (contents == NULL)
+     {
+       /* XXX Should we delete the section from the bfd ?  */
+       bfd_set_error (bfd_error_no_memory);
+       return FALSE;
+     }
+ 
+   bfd_put_32 (abfd, timestamp, contents);
+ 
+   if (! bfd_set_section_contents (abfd, sect, contents, 0, 4))
      {
        /* XXX Should we delete the section from the bfd ?  */
        free (contents);

[-- Attachment #3: binutils.diff --]
[-- Type: text/plain, Size: 3871 bytes --]

Index: objcopy.c
===================================================================
RCS file: /cvs/src/src/binutils/objcopy.c,v
retrieving revision 1.51
diff -c -3 -p -r1.51 objcopy.c
*** objcopy.c	27 Jun 2003 08:01:28 -0000	1.51
--- objcopy.c	4 Jul 2003 21:18:19 -0000
*************** static struct section_add *add_sections;
*** 224,229 ****
--- 224,233 ----
     This should be the filename to store in the .gnu_debuglink section.  */
  static const char * gnu_debuglink_filename = NULL;
  
+ /* If non zero the argument to --set-debuglink-timestamp.
+    This should be the value to store in the .gnu_debuglink_timestamp */
+ static unsigned long int gnu_debuglink_timestamp;
+ 
  /* Whether to convert debugging information.  */
  static bfd_boolean convert_debugging = FALSE;
  
*************** static char *prefix_alloc_sections_strin
*** 286,291 ****
--- 290,296 ----
  #define OPTION_FORMATS_INFO (OPTION_PREFIX_ALLOC_SECTIONS + 1)
  #define OPTION_ADD_GNU_DEBUGLINK (OPTION_FORMATS_INFO + 1)
  #define OPTION_ONLY_KEEP_DEBUG (OPTION_ADD_GNU_DEBUGLINK + 1)
+ #define OPTION_SET_DEBUGLINK_TIMESTAMP  (OPTION_ONLY_KEEP_DEBUG + 1)
  
  /* Options to handle if running as "strip".  */
  
*************** static struct option copy_options[] =
*** 371,376 ****
--- 376,382 ----
    {"set-start", required_argument, 0, OPTION_SET_START},
    {"srec-len", required_argument, 0, OPTION_SREC_LEN},
    {"srec-forceS3", no_argument, 0, OPTION_SREC_FORCES3},
+   {"set-debuglink-timestamp", required_argument, 0, OPTION_SET_DEBUGLINK_TIMESTAMP},
    {"strip-all", no_argument, 0, 'S'},
    {"strip-debug", no_argument, 0, 'g'},
    {"strip-unneeded", no_argument, 0, OPTION_STRIP_UNNEEDED},
*************** copy_object (ibfd, obfd)
*** 1133,1138 ****
--- 1139,1145 ----
    long symcount;
    asection **osections = NULL;
    asection * gnu_debuglink_section = NULL;
+   asection * gnu_debuglink_timestamp_section = NULL;
    bfd_size_type *gaps = NULL;
    bfd_size_type max_gap = 0;
    long symsize;
*************** copy_object (ibfd, obfd)
*** 1258,1263 ****
--- 1265,1280 ----
  	RETURN_NONFATAL (gnu_debuglink_filename);
  	}
      }
+   else if (gnu_debuglink_timestamp) {
+       gnu_debuglink_timestamp_section =
+ 	bfd_create_gnu_debuglink_timestamp_section (obfd);
+ 
+       if (gnu_debuglink_timestamp_section == NULL)
+ 	{
+ 	  fprintf (stderr, "UGG A\n");
+ 	  RETURN_NONFATAL ("Unable to create gnu_debuglink_timestamp section");
+ 	}
+   }
  
    if (gap_fill_set || pad_to_set)
      {
*************** copy_object (ibfd, obfd)
*** 1421,1434 ****
  
    if (gnu_debuglink_filename != NULL)
      {
!       if (! bfd_fill_in_gnu_debuglink_section
! 	  (obfd, gnu_debuglink_section, gnu_debuglink_filename))
  	{
  	  fprintf (stderr, "UGG 2\n");
  	  RETURN_NONFATAL (gnu_debuglink_filename);
  	}
      }
  
    if (gap_fill_set || pad_to_set)
      {
        bfd_byte *buf;
--- 1438,1462 ----
  
    if (gnu_debuglink_filename != NULL)
      {
!       if (! bfd_fill_in_gnu_debuglink_section (obfd, gnu_debuglink_section,
! 					       gnu_debuglink_filename,
! 					       gnu_debuglink_timestamp))
  	{
  	  fprintf (stderr, "UGG 2\n");
  	  RETURN_NONFATAL (gnu_debuglink_filename);
  	}
      }
  
+   if (gnu_debuglink_timestamp_section)
+     {
+       if (! bfd_fill_in_gnu_debuglink_timestamp_section
+ 	  (obfd, gnu_debuglink_timestamp_section, gnu_debuglink_timestamp))
+ 	{
+ 	  fprintf (stderr, "UGG 3\n");
+ 	  RETURN_NONFATAL ("Unable to fill gnu_debuglink_timestamp section");
+ 	}
+     }
+ 
    if (gap_fill_set || pad_to_set)
      {
        bfd_byte *buf;
*************** copy_main (argc, argv)
*** 2458,2463 ****
--- 2486,2495 ----
  
  	case OPTION_ADD_GNU_DEBUGLINK:
  	  gnu_debuglink_filename = optarg;
+ 	  break;
+ 
+ 	case OPTION_SET_DEBUGLINK_TIMESTAMP:
+ 	  gnu_debuglink_timestamp = atoi(optarg);
  	  break;
  
  	case 'K':

[-- Attachment #4: gdb.diff --]
[-- Type: text/plain, Size: 665 bytes --]

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.101
diff -u -r1.101 symfile.c
--- symfile.c	27 Jun 2003 13:11:17 -0000	1.101
+++ symfile.c	3 Jul 2003 13:54:51 -0000
@@ -1103,6 +1103,7 @@
 static char *
 find_separate_debug_file (struct objfile *objfile)
 {
+#if 0
   asection *sect;
   char *basename;
   char *dir;
@@ -1178,6 +1179,12 @@
   xfree (basename);
   xfree (dir);
   return NULL;
+#else
+  char *debug_file = NULL;
+  if (objfile->obfd)
+    debug_file = bfd_follow_gnu_debuglink(objfile->obfd, debug_file_directory);
+  return debug_file;
+#endif
 }
 
 

Re: separated debuginfo patch

[Nick Clifton]

Hi Philippe,

  Sorry for the long delay in replying - I have been very busy over
  the last few weeks.


> I need to know how GDB guys want I deal with the gdb part, for now
> gdb.diff just remove (#if 0) all duplicated code from bfd and use
> bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it
> ok to remove this code or must I update the duplicated code according
> to the change in bfd ?

  Well this is up to the gdb maintainers to decide, but it certainly
  seems like a good idea to avoid the code duplication.


> > We just need to agree on the format and name of the timestamp
> > section in the debuginfo file, and how to distinguish old
> > .gnu-debuglink sections (only containing a crc) from new ones
> > (containing a crc and a timestamp).
> 
> I disambiguate the two case by looking the section size. See
> bfd.diff, it's a bit ugly but I don't see how I can deal cleanly
> with this problem.

Actually I rather like this solution.  Adding the timestamp as an
extra field at the end of the .gnu-debuglink section seems rather
elegant and it eliminates the need for a new section.

Some comments on the patch:
      
> + #define GNU_DEBUGLINK_TIMESTAMP ".gnu_debuglink_timestamp"

This is not needed, since the timestamp is going to be held in the
.gnu-debuglink section.


>   INTERNAL_FUNCTION
> + 	bfd_get_debug_timestamp_info
> + 
> + SYNOPSIS
> + 	bfd_boolean bfd_get_debug_timestamp_info
> + 	  (bfd *abfd, unsigned long *timestamp);
> + 
> + DESCRIPTION
> + 	fetch the timestamp for any separate debuginfo associated with
> + 	@var{abfd}.

Similarly this function should extract the timestamp from the
.gnu-debuglink section.


> +   printf("try %s %ld\n", name, timestamp);

I assume that this is left over debugging ?

> !   debuglink_size += 8;

We ought to add a comment explaining why the value 8 is used here.


Overall though I like the patch and the solution.  If we can get the
GDB maintainers to agree (or at least not object to) adding the extra
field at the end of the .gnu-debuglink section then I would be happy
to review a final version of the patch.  (Note - you will need a FSF
copyright assignment as well...)

Cheers
        Nick

Re: separated debuginfo patch

[Jim Blandy]

Nick Clifton <nickc@redhat.com> writes:

> Hi Philippe,
> 
>   Sorry for the long delay in replying - I have been very busy over
>   the last few weeks.
> 
> 
> > I need to know how GDB guys want I deal with the gdb part, for now
> > gdb.diff just remove (#if 0) all duplicated code from bfd and use
> > bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it
> > ok to remove this code or must I update the duplicated code according
> > to the change in bfd ?
> 
>   Well this is up to the gdb maintainers to decide, but it certainly
>   seems like a good idea to avoid the code duplication.

Sure, the plan has long been for GDB to just use the function in BFD.
The code was added to GDB before BFD; that's the only reason it's
there at all.

Just to be sure --- under this arrangement, the old-style debug links
will continue to work, right?

One could use something like '(date; ps auxww; vmstat) | md5sum | cut
-b 1-33' to generate nice unique ID strings.

Re: separated debuginfo patch

[Nick Clifton]

Hi Jim,

>> > I need to know how GDB guys want I deal with the gdb part, for now
>> > gdb.diff just remove (#if 0) all duplicated code from bfd and use
>> > bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it
>> > ok to remove this code or must I update the duplicated code according
>> > to the change in bfd ?
>> 
>>   Well this is up to the gdb maintainers to decide, but it certainly
>>   seems like a good idea to avoid the code duplication.
>
> Sure, the plan has long been for GDB to just use the function in BFD.
> The code was added to GDB before BFD; that's the only reason it's
> there at all.
>
> Just to be sure --- under this arrangement, the old-style debug links
> will continue to work, right?

Yes.

> One could use something like '(date; ps auxww; vmstat) | md5sum | cut
> -b 1-33' to generate nice unique ID strings.

yikes!  Well that ought to be a reasonably randon number.  The only
problem would be making sure that the same number was added to both
the debuginfo file and the stripped binary.  I guess you would need to
make sure your makefile only computed the value once, before it
creates either file.

Cheers
        Nick
        

Re: separated debuginfo patch

[Andrew Cagney]

Philippe,
> 
> [trying to avoid crc'ing the separate debug file]
> 
> I need to know how GDB guys want I deal with the gdb part, for now
> gdb.diff just remove (#if 0) all duplicated code from bfd and use
> bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it
> ok to remove this code or must I update the duplicated code according
> to the change in bfd ? 

I just wonder if it should eventually be made more transparent?
    bfd_openr (file, FOLLOW_DEBUG_LINK).
Doing things like:
	objdump --follow-debug-link
would then become possible.  Regardless, it makes sense to put the 
algorighm in BFD.

Nick wrote:
> Overall though I like the patch and the solution.  If we can get the
> GDB maintainers to agree (or at least not object to) adding the extra
> field at the end of the .gnu-debuglink section then I would be happy
> to review a final version of the patch.  (Note - you will need a FSF
> copyright assignment as well...)

Well, the so called GNU debuglink mechanism was never actually discussed 
on a GNU list, re-visiting it now sounds like a good idea.  Looks to me 
like you've come up with something actually useful.

Perhaps someone should post a revised description and have it added to 
the BFD doco.  Here's the current description from GDB:
http://sources.redhat.com/gdb/current/onlinedocs/gdb_16.html#SEC134

Andrew

Re: separated debuginfo patch

[Daniel Jacobowitz]

On Fri, Jul 18, 2003 at 09:42:18AM -0400, Andrew Cagney wrote:
> Philippe,
> >
> >[trying to avoid crc'ing the separate debug file]
> >
> >I need to know how GDB guys want I deal with the gdb part, for now
> >gdb.diff just remove (#if 0) all duplicated code from bfd and use
> >bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it
> >ok to remove this code or must I update the duplicated code according
> >to the change in bfd ? 
> 
> I just wonder if it should eventually be made more transparent?
>    bfd_openr (file, FOLLOW_DEBUG_LINK).
> Doing things like:
> 	objdump --follow-debug-link
> would then become possible.  Regardless, it makes sense to put the 
> algorighm in BFD.

It should.  I talked Graydon into trying this at the time he submitted
the BFD part - it turned out to be a world of trouble given BFDs
current data structure, and we bailed out.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: separated debuginfo patch

[graydon hoare]

Daniel Jacobowitz <drow@mvista.com> writes:

> On Fri, Jul 18, 2003 at 09:42:18AM -0400, Andrew Cagney wrote:
> > Philippe,
> > >
> > >[trying to avoid crc'ing the separate debug file]
> > >
> > >I need to know how GDB guys want I deal with the gdb part, for now
> > >gdb.diff just remove (#if 0) all duplicated code from bfd and use
> > >bfd_follow_gnu_debuglink() to retrieve the debug info file. Is it
> > >ok to remove this code or must I update the duplicated code according
> > >to the change in bfd ? 
> > 
> > I just wonder if it should eventually be made more transparent?
> >    bfd_openr (file, FOLLOW_DEBUG_LINK).
> > Doing things like:
> > 	objdump --follow-debug-link
> > would then become possible.  Regardless, it makes sense to put the 
> > algorighm in BFD.
> 
> It should.  I talked Graydon into trying this at the time he submitted
> the BFD part - it turned out to be a world of trouble given BFDs
> current data structure, and we bailed out.

yeah, we gave up on "transparent" access in bfd_openr due to the
ugliness of merging symbols from separate bfds; nonetheless the
requisite (distinct) debuglink-following function was added upstream.

http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/opncls.c.diff?r1=1.13&r2=1.14&cvsroot=src&f=h

-graydon

Re: separated debuginfo patch

[Michael Elizabeth Chastain]

Jim Blandy writes:
> One could use something like '(date; ps auxww; vmstat) | md5sum | cut
> -b 1-33' to generate nice unique ID strings.

If you are on Linux, and you can be platform-specific,
a useful command is:

  uuidgen -r

Michael C