Re: Patch for objcopy

[Nick Clifton <nickc at redhat dot com>]

Hi Aishwarya,

> I have a patch for objcopy which addresses the following issue; There
> are two particular options of objcopy that do not work when specified
> together:
> --only-keep-debug&  --only-section<name>
>
> When both the above options are specified together , only the second
> option works. I  have made small changes to objcopy in a manner such
> that both the above options work together when specified. I have
> included the patch for this below.
>
> Would anyone have any suggestions about this ?

Thank you for raising this issue and submitting a patch.  I do have a 
few comments on the matter however.  Firstly, it is not clear to me 
exactly how these two options should interact.  For example:

  --only-keep-debug --only-section=foo

Should this keep all of the debug sections *and* a section named "foo". 
 Or should it only keep the section "foo" and then only if it is a 
debug section.

I assume that you believe the former ?  Either way the decision ought to 
be documented (in binutils/doc/binutils.texi).

Personally, I am currently of the opinion that the two options should 
not be used together.  Ie an error message should be issued if the user 
tries to keep both debug sections and specifically named sections.  This 
is because the --only-keep-debug option is intended for a specific 
purpose - created stripped debug file.  If you need to keep a whole 
range of sections, including some debugging sections, then it would be 
cleaner to use multiple --only-section options.


> --- binutil-final/binutils-2.20.1/binutils/objcopy.c    2010-12-02
> 12:24:12.000000000 -0600
> +++ original-binutil/binutils-2.20.1/binutils/objcopy.c 2010-11-29
> 15:06:03.000000000 -0600

You ran the diff the wrong way around.  Currently your patch shows how 
to remove your changes from modified code, rather than how to add them 
to unmodified code...

Also you are patching an old set of binutils sources.  It really helps 
if you use the latest development sources from the head of the mainline 
of the binutils repository.


> -  if ((strip_symbols == STRIP_DEBUG
> +  if (strip_symbols == STRIP_DEBUG
>        || strip_symbols == STRIP_ALL
>        || strip_symbols == STRIP_UNNEEDED
>        || strip_symbols == STRIP_NONDEBUG
> @@ -1806,7 +1806,7 @@ copy_object (bfd *ibfd, bfd *obfd)
>        || change_leading_char
>        || remove_leading_char
>        || redefine_sym_list
> -      || weaken)&&((strip_symbols != STRIP_NONDEBUG) || (!sections_copied)))
> +      || weaken)

This seems wrong.  If you do not want this if-statement to trigger when 
strip_symbols is STRIP_NONDEBUG or sections_copied is true then why not 
just remove them from the list of conditions to test ?

Why do you modify setup_section() and copy_section() rather than 
changing is_strip_section() ?  It seems to me that a patch like the one 
attached would achieve the same thing as your modification, but in a 
cleaner fashion.   What do you think ?


Cheers
  Nick


Index: binutils/objcopy.c
===================================================================
RCS file: /cvs/src/src/binutils/objcopy.c,v
retrieving revision 1.152
diff -u -3 -p -r1.152 objcopy.c
--- binutils/objcopy.c	6 May 2011 14:41:56 -0000	1.152
+++ binutils/objcopy.c	3 Jun 2011 14:24:49 -0000
@@ -938,8 +938,12 @@ is_strip_section (bfd *abfd ATTRIBUTE_UN
 
       if (sections_removed && p != NULL && p->remove)
 	return TRUE;
-      if (sections_copied && (p == NULL || ! p->copy))
-	return TRUE;
+      /* If sections_copied is true, but the indicated section is not
+	 found then do not return FALSE here.  There might be other
+	 reasons for keeping the section, such as --only-keep-debug
+	 being enabled.  */
+      if (sections_copied && p != NULL && p->copy)
+	return FALSE;
     }
 
   if ((bfd_get_section_flags (abfd, sec) & SEC_DEBUGGING) != 0)
@@ -974,7 +978,7 @@ is_strip_section (bfd *abfd ATTRIBUTE_UN
 	return TRUE;
     }
 
-  return FALSE;
+  return sections_copied ? TRUE : FALSE;
 }
 
 /* Return true if SYM is a hidden symbol.  */