This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Patch for objcopy


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.  */

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]