Discussion:
Fix for PR-23611
kamlesh kumar
2018-09-07 08:55:23 UTC
Permalink
Hi Everyone,

Attached patch(23611.patch) has a fix for the subjected issue and here
is the ChangeLog

2018-09-07 Kamlesh Kumar ***@gmail.com

PR binutils/23611
* objcopy.c (is_strip_section_1) :modified to consider
the .rela.plt and rela.dyn section for stripping.
* testsuite/binutils-all/objcopy.exp :Updated to add
the pr23611.c file.
* testsuite/binutils-all/pr23611.c: New file.

Please share your comments on the fix and did format is ok to commit ?

Thank you
Kamlesh
Andrew Burgess
2018-09-07 11:15:07 UTC
Permalink
I'm not a maintainer so can't approve your patch, but here's my
feedback as an interested party :)

Thanks,
Andrew
Post by kamlesh kumar
Hi Everyone,
Attached patch(23611.patch) has a fix for the subjected issue and here
is the ChangeLog
Two whitespace between date/name/email, and the email is in '<'...'>'.
Post by kamlesh kumar
PR binutils/23611
* objcopy.c (is_strip_section_1) :modified to consider
the .rela.plt and rela.dyn section for stripping.
The continuation lines should start in the same column as the '*', and
there's no whitespace before the ':', but there should be one after.
Post by kamlesh kumar
* testsuite/binutils-all/objcopy.exp :Updated to add
the pr23611.c file.
* testsuite/binutils-all/pr23611.c: New file.
Please share your comments on the fix and did format is ok to commit ?
I would prefer to see an extended commit message that explains what
the problem is and what the solution does, as well as referencing the
bug - we'll likely always have the commit message, but the bug
database could disappear one day.
Post by kamlesh kumar
Thank you
Kamlesh
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 6bd9339..5dd9b29 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -1284,11 +1284,19 @@ is_strip_section_1 (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
{
struct section_list *p;
struct section_list *q;
+ const char *secname = bfd_get_section_name (abfd, sec);
- p = find_section_list (bfd_get_section_name (abfd, sec), FALSE,
- SECTION_CONTEXT_REMOVE);
- q = find_section_list (bfd_get_section_name (abfd, sec), FALSE,
- SECTION_CONTEXT_COPY);
+ if ((strncmp (secname,".rela.",6) == 0) ||
+ (strncmp (secname,".rel.",5) == 0))
You need whitespace after each ',', and move the '||' to the start of
the next line.
Post by kamlesh kumar
+ {
+ secname=(strncmp (secname,".rela.",6) == 0 ? secname+5:secname+4);
+ p=find_section_list (secname, FALSE,SECTION_CONTEXT_REMOVE_RELOCS);
Again, whitespace needs fixing up inline with the rest of the file,
around '=', ',', '+', ':'. This will probably require some of the
lines to be split so as to not get too long.
Post by kamlesh kumar
+ }
+ else {
+ p=find_section_list (secname,FALSE,SECTION_CONTEXT_REMOVE);
+ }
+
+ q = find_section_list (secname, FALSE,SECTION_CONTEXT_COPY);
if (p && q)
fatal (_("error: section %s matches both remove and copy options"),
@@ -3926,6 +3934,7 @@ static void
handle_remove_relocations_option (const char *section_pattern)
{
find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE_RELOCS);
+ sections_removed = TRUE;
}
/* Return TRUE if ISECTION from IBFD should have its relocations removed,
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index d979648..a31a52e 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1218,6 +1218,68 @@ proc objcopy_test_without_global_symbol { } {
pass $test
}
+# objcopy remove relocation from executable test
+proc objcopy_remove_rela_plt { } {
+ global OBJCOPY
+ global READELF
+ set out1 tmpdir/test1
+ set objfile tmpdir/pr23611
+ set exec_output1 [binutils_run $OBJCOPY "--remove-relocations=.plt $objfile $out1"]
+ set exec_output2 [binutils_run $READELF "-S $out1"]
+ if { [string match "*.rela.plt*" $exec_output2]||[string match "*.rel.plt*" $exec_output2] } {
I think whitespace around the '||' is needed.
Post by kamlesh kumar
+ fail "can not remove plt relocation."
+ return
+ }
+ pass "plt relocation removed successfully"
+}
+
+proc objcopy_remove_rela_dyn { } {
+ global OBJCOPY
+ global READELF
+ set out2 tmpdir/test2
+ set objfile tmpdir/pr23611
+ set exec_output1 [binutils_run $OBJCOPY "--remove-relocations=.dyn $objfile $out2"]
+ set exec_output2 [binutils_run $READELF "-S $out2"]
+ if { [string match "*.rela.dyn*" $exec_output2]||[string match "*.rel.dyn*" $exec_output2] } {
+ fail "can not remove dyn relocation."
+ return
+ }
+ pass "dyn relocation removed successfully"
+}
+
+proc objcopy_test_remove_relocation_section_from_executable { } {
+ global OBJCOPY
+ global srcdir
+ global subdir
+ global READELF
+
+ if { [target_compile $srcdir/$subdir/pr23611.c tmpdir/pr23611 executable debug] != "" } {
+ untested "can't be tested."
+ return
+ }
+
+ if [is_remote host] {
+ set objfile [remote_download host tmpdir/pr23611]
+ } else {
+ set objfile tmpdir/pr23611
+ }
+ set out3 tmpdir/test3
+
+ set exec_output1 [binutils_run $OBJCOPY "-R .rela.plt -R .rela.dyn -R .rel.plt -R .rel.dyn $objfile $out3"]
+ set exec_output2 [binutils_run $READELF "-S $out3"]
+ if { [string match "*.rel.plt*" $exec_output2]||[string match "*.rela.plt*" $exec_output2]||[string match "*.rel.dyn*" $exec_output2]||[string match "*.rela.dyn*" $exec_output2] } {
+ fail "can't not remove plt or dyn relocation."
+ return
+ }
+ pass "removed dynamic relocations from executable"
+
+}
+
+objcopy_test_remove_relocation_section_from_executable
+objcopy_remove_rela_dyn
+objcopy_remove_rela_plt
+
+
# The AArch64 and ARM targets preserve mapping symbols
# in object files, so they will fail this test.
setup_xfail aarch64*-*-* arm*-*-*
kamlesh kumar
2018-09-07 12:15:43 UTC
Permalink
Thank you Andrew for the initial comments and attached patch addressed
the same and here is the change log entry .

2018-09-07 Kamlesh Kumar <***@gmail.com>

PR binutils/23611
* objcopy.c (is_strip_section_1): modified to consider
the .rela.plt and rela.dyn section for stripping where the
context is SECTION_CONTEXT_REMOVE_RELOCS not the
SECTION_CONTEXT_REMOVE any more,where .rela prefix is truncated
from the section name.
* testsuite/binutils-all/objcopy.exp: Updated to add
the pr23611.c file.
* testsuite/binutils-all/pr23611.c: New file.

Thank you Again
Kamlesh


On Fri, Sep 7, 2018 at 4:45 PM, Andrew Burgess
Post by Andrew Burgess
I'm not a maintainer so can't approve your patch, but here's my
feedback as an interested party :)
Thanks,
Andrew
Post by kamlesh kumar
Hi Everyone,
Attached patch(23611.patch) has a fix for the subjected issue and here
is the ChangeLog
Two whitespace between date/name/email, and the email is in '<'...'>'.
Post by kamlesh kumar
PR binutils/23611
* objcopy.c (is_strip_section_1) :modified to consider
the .rela.plt and rela.dyn section for stripping.
The continuation lines should start in the same column as the '*', and
there's no whitespace before the ':', but there should be one after.
Post by kamlesh kumar
* testsuite/binutils-all/objcopy.exp :Updated to add
the pr23611.c file.
* testsuite/binutils-all/pr23611.c: New file.
Please share your comments on the fix and did format is ok to commit ?
I would prefer to see an extended commit message that explains what
the problem is and what the solution does, as well as referencing the
bug - we'll likely always have the commit message, but the bug
database could disappear one day.
Post by kamlesh kumar
Thank you
Kamlesh
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 6bd9339..5dd9b29 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -1284,11 +1284,19 @@ is_strip_section_1 (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
{
struct section_list *p;
struct section_list *q;
+ const char *secname = bfd_get_section_name (abfd, sec);
- p = find_section_list (bfd_get_section_name (abfd, sec), FALSE,
- SECTION_CONTEXT_REMOVE);
- q = find_section_list (bfd_get_section_name (abfd, sec), FALSE,
- SECTION_CONTEXT_COPY);
+ if ((strncmp (secname,".rela.",6) == 0) ||
+ (strncmp (secname,".rel.",5) == 0))
You need whitespace after each ',', and move the '||' to the start of
the next line.
Post by kamlesh kumar
+ {
+ secname=(strncmp (secname,".rela.",6) == 0 ? secname+5:secname+4);
+ p=find_section_list (secname, FALSE,SECTION_CONTEXT_REMOVE_RELOCS);
Again, whitespace needs fixing up inline with the rest of the file,
around '=', ',', '+', ':'. This will probably require some of the
lines to be split so as to not get too long.
Post by kamlesh kumar
+ }
+ else {
+ p=find_section_list (secname,FALSE,SECTION_CONTEXT_REMOVE);
+ }
+
+ q = find_section_list (secname, FALSE,SECTION_CONTEXT_COPY);
if (p && q)
fatal (_("error: section %s matches both remove and copy options"),
@@ -3926,6 +3934,7 @@ static void
handle_remove_relocations_option (const char *section_pattern)
{
find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE_RELOCS);
+ sections_removed = TRUE;
}
/* Return TRUE if ISECTION from IBFD should have its relocations removed,
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index d979648..a31a52e 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1218,6 +1218,68 @@ proc objcopy_test_without_global_symbol { } {
pass $test
}
+# objcopy remove relocation from executable test
+proc objcopy_remove_rela_plt { } {
+ global OBJCOPY
+ global READELF
+ set out1 tmpdir/test1
+ set objfile tmpdir/pr23611
+ set exec_output1 [binutils_run $OBJCOPY "--remove-relocations=.plt $objfile $out1"]
+ set exec_output2 [binutils_run $READELF "-S $out1"]
+ if { [string match "*.rela.plt*" $exec_output2]||[string match "*.rel.plt*" $exec_output2] } {
I think whitespace around the '||' is needed.
Post by kamlesh kumar
+ fail "can not remove plt relocation."
+ return
+ }
+ pass "plt relocation removed successfully"
+}
+
+proc objcopy_remove_rela_dyn { } {
+ global OBJCOPY
+ global READELF
+ set out2 tmpdir/test2
+ set objfile tmpdir/pr23611
+ set exec_output1 [binutils_run $OBJCOPY "--remove-relocations=.dyn $objfile $out2"]
+ set exec_output2 [binutils_run $READELF "-S $out2"]
+ if { [string match "*.rela.dyn*" $exec_output2]||[string match "*.rel.dyn*" $exec_output2] } {
+ fail "can not remove dyn relocation."
+ return
+ }
+ pass "dyn relocation removed successfully"
+}
+
+proc objcopy_test_remove_relocation_section_from_executable { } {
+ global OBJCOPY
+ global srcdir
+ global subdir
+ global READELF
+
+ if { [target_compile $srcdir/$subdir/pr23611.c tmpdir/pr23611 executable debug] != "" } {
+ untested "can't be tested."
+ return
+ }
+
+ if [is_remote host] {
+ set objfile [remote_download host tmpdir/pr23611]
+ } else {
+ set objfile tmpdir/pr23611
+ }
+ set out3 tmpdir/test3
+
+ set exec_output1 [binutils_run $OBJCOPY "-R .rela.plt -R .rela.dyn -R .rel.plt -R .rel.dyn $objfile $out3"]
+ set exec_output2 [binutils_run $READELF "-S $out3"]
+ if { [string match "*.rel.plt*" $exec_output2]||[string match "*.rela.plt*" $exec_output2]||[string match "*.rel.dyn*" $exec_output2]||[string match "*.rela.dyn*" $exec_output2] } {
+ fail "can't not remove plt or dyn relocation."
+ return
+ }
+ pass "removed dynamic relocations from executable"
+
+}
+
+objcopy_test_remove_relocation_section_from_executable
+objcopy_remove_rela_dyn
+objcopy_remove_rela_plt
+
+
# The AArch64 and ARM targets preserve mapping symbols
# in object files, so they will fail this test.
setup_xfail aarch64*-*-* arm*-*-*
Alan Modra
2018-09-10 04:32:27 UTC
Permalink
BFD handles ELF relocation sections in an executable differently to
relocation sections in a relocatable object. For a relocatable
object, BFD carries the relocations as data associated with the
section to which they apply; The relocation section doesn't appear as
a separate section. For an executable, dynamic relocation sections do
appear as separate sections. This means that objcopy needs to use
different strategies when dealing with relocations.

When --remove-relocations was added to objcopy with commit
d3e5f6c8f1e, objcopy lost the ability to remove dynamic relocation
sections such as .rela.plt from executables using the option
"--remove-section=.rela.plt". This patch reinstates that
functionality.

I thought it best to keep --remove-relocations as is, rather than
extending to handle dynamic relocations as per the patch in the PR,
because executables linked with --emit-relocs may have both dynamic
and non-dynamic relocations. In that case --remove-relocataions=* is
useful to remove all the non-dynamic relocations.

PR binutils/23611
* objcopy.c (handle_remove_section_option): Consider .rela and
.rel sections for stripping directly as well as attached to the
associated section they relocate.
* doc/binutils.texi (remove-relocations): Specify that this
option removes non-dynamic relocation sections.
* testsuite/binutils-all/objcopy.exp
(objcopy_remove_relocations_from_executable): New test.

diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index e40ccb073d..76cbed05af 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -1321,17 +1321,20 @@ will remove all sections matching the pattern '.text.*', but will not
remove the section '.text.foo'.

@item --remove-relocations=@var{sectionpattern}
-Remove relocations from the output file for any section matching
-@var{sectionpattern}. This option may be given more than once. Note
-that using this option inappropriately may make the output file
-unusable. Wildcard characters are accepted in @var{sectionpattern}.
+Remove non-dynamic relocations from the output file for any section
+matching @var{sectionpattern}. This option may be given more than
+once. Note that using this option inappropriately may make the output
+file unusable, and attempting to remove a dynamic relocation section
+such as @samp{.rela.plt} from an executable or shared library with
+@option{--remove-relocations=.plt} will not work. Wildcard characters
+are accepted in @var{sectionpattern}.
For example:

@smallexample
--remove-relocations=.text.*
@end smallexample

-will remove the relocations for all sections matching the patter
+will remove the relocations for all sections matching the pattern
'.text.*'.

If the first character of @var{sectionpattern} is the exclamation
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 6bd933993b..f712ffe591 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -3943,7 +3943,7 @@ discard_relocations (bfd *ibfd ATTRIBUTE_UNUSED, asection *isection)
/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
section (one starting with ".rela." or ".rel.") then this removal must
- be done using a different technique. */
+ be done using a different technique in a relocatable object. */

static void
handle_remove_section_option (const char *section_pattern)
@@ -3952,11 +3952,9 @@ handle_remove_section_option (const char *section_pattern)
handle_remove_relocations_option (section_pattern + 5);
else if (strncmp (section_pattern, ".rel.", 5) == 0)
handle_remove_relocations_option (section_pattern + 4);
- else
- {
- find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE);
- sections_removed = TRUE;
- }
+
+ find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE);
+ sections_removed = TRUE;
}

/* Copy relocations in input section ISECTION of IBFD to an output
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index d979648758..61793d98f5 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1223,3 +1223,36 @@ proc objcopy_test_without_global_symbol { } {
setup_xfail aarch64*-*-* arm*-*-*

objcopy_test_without_global_symbol
+
+# objcopy remove relocation from executable test
+
+proc objcopy_remove_relocations_from_executable { } {
+ global OBJCOPY
+ global srcdir
+ global subdir
+ global READELF
+
+ set test "remove-section relocation sections"
+
+ if { [target_compile $srcdir/$subdir/testprog.c tmpdir/pr23611 executable debug] != "" } {
+ untested $test
+ return
+ }
+
+ if [is_remote host] {
+ set objfile [remote_download host tmpdir/pr23611]
+ } else {
+ set objfile tmpdir/pr23611
+ }
+ set out tmpdir/pr23611.out
+
+ set exec_output1 [binutils_run $OBJCOPY "-R .rela.plt -R .rela.dyn -R .rel.plt -R .rel.dyn $objfile $out"]
+ set exec_output2 [binutils_run $READELF "-S $out"]
+ if { [string match "*.rel.plt*" $exec_output2] || [string match "*.rela.plt*" $exec_output2] || [string match "*.rel.dyn*" $exec_output2] || [string match "*.rela.dyn*" $exec_output2] } {
+ fail $test
+ return
+ }
+ pass $test
+}
+
+objcopy_remove_relocations_from_executable
--
Alan Modra
Australia Development Lab, IBM
Andrew Burgess
2018-09-10 09:54:50 UTC
Permalink
Post by Alan Modra
BFD handles ELF relocation sections in an executable differently to
relocation sections in a relocatable object. For a relocatable
object, BFD carries the relocations as data associated with the
section to which they apply; The relocation section doesn't appear as
a separate section. For an executable, dynamic relocation sections do
appear as separate sections. This means that objcopy needs to use
different strategies when dealing with relocations.
When --remove-relocations was added to objcopy with commit
d3e5f6c8f1e, objcopy lost the ability to remove dynamic relocation
sections such as .rela.plt from executables using the option
"--remove-section=.rela.plt". This patch reinstates that
functionality.
I thought it best to keep --remove-relocations as is, rather than
extending to handle dynamic relocations as per the patch in the PR,
because executables linked with --emit-relocs may have both dynamic
and non-dynamic relocations. In that case --remove-relocataions=* is
useful to remove all the non-dynamic relocations.
I think having an option that removes _almost_ all relocation sections
is going to cause user confusion. Yes, we can document it, but if we
can make it work for all relocation sections I think we probably
should.

One thing I tried but couldn't find a good answer for, is, is there a
flag that appears on the input section that identifies it as a
relocation section, even though it's actually being handled as a
"normal" section? If such a thing existed then that would make
special casing `is_strip_section_1` slightly less invasive I think.

If we can't identifty relocation section and/or don't want special
cases in the `is_strip_section_1` code, could we not add more logic
into the `handle_remove_relocations_option` code? Maybe we could
generate two new section names, one prefixed with `rel.` and one with
`rela.` and add both of these to the strip section list? Or maybe add
a pattern prefix, `rel*.`?
Post by Alan Modra
PR binutils/23611
* objcopy.c (handle_remove_section_option): Consider .rela and
.rel sections for stripping directly as well as attached to the
associated section they relocate.
* doc/binutils.texi (remove-relocations): Specify that this
option removes non-dynamic relocation sections.
* testsuite/binutils-all/objcopy.exp
(objcopy_remove_relocations_from_executable): New test.
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index e40ccb073d..76cbed05af 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -1321,17 +1321,20 @@ will remove all sections matching the pattern '.text.*', but will not
remove the section '.text.foo'.
@item --remove-relocations=@var{sectionpattern}
-Remove relocations from the output file for any section matching
-that using this option inappropriately may make the output file
+Remove non-dynamic relocations from the output file for any section
+once. Note that using this option inappropriately may make the output
+file unusable, and attempting to remove a dynamic relocation section
@smallexample
--remove-relocations=.text.*
@end smallexample
-will remove the relocations for all sections matching the patter
+will remove the relocations for all sections matching the pattern
'.text.*'.
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 6bd933993b..f712ffe591 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -3943,7 +3943,7 @@ discard_relocations (bfd *ibfd ATTRIBUTE_UNUSED, asection *isection)
/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
section (one starting with ".rela." or ".rel.") then this removal must
- be done using a different technique. */
+ be done using a different technique in a relocatable object. */
I'm not sure I agree with this comment, how about:

.... then this removal must be done using a different technique,
except for dynamic relocations (for example .rela.plt and
rela.dyn) which are handled like standard sections. */

As far as I can tell through experimenting things like .rela.text are
always held as data off of the .text section no matter what the kind
of object/executable. Or is there a specific type of build where all
relocation sections are treated like normal sections?

Thanks,
Andrew
Post by Alan Modra
static void
handle_remove_section_option (const char *section_pattern)
@@ -3952,11 +3952,9 @@ handle_remove_section_option (const char *section_pattern)
handle_remove_relocations_option (section_pattern + 5);
else if (strncmp (section_pattern, ".rel.", 5) == 0)
handle_remove_relocations_option (section_pattern + 4);
- else
- {
- find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE);
- sections_removed = TRUE;
- }
+
+ find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE);
+ sections_removed = TRUE;
}
/* Copy relocations in input section ISECTION of IBFD to an output
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index d979648758..61793d98f5 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1223,3 +1223,36 @@ proc objcopy_test_without_global_symbol { } {
setup_xfail aarch64*-*-* arm*-*-*
objcopy_test_without_global_symbol
+
+# objcopy remove relocation from executable test
+
+proc objcopy_remove_relocations_from_executable { } {
+ global OBJCOPY
+ global srcdir
+ global subdir
+ global READELF
+
+ set test "remove-section relocation sections"
+
+ if { [target_compile $srcdir/$subdir/testprog.c tmpdir/pr23611 executable debug] != "" } {
+ untested $test
+ return
+ }
+
+ if [is_remote host] {
+ set objfile [remote_download host tmpdir/pr23611]
+ } else {
+ set objfile tmpdir/pr23611
+ }
+ set out tmpdir/pr23611.out
+
+ set exec_output1 [binutils_run $OBJCOPY "-R .rela.plt -R .rela.dyn -R .rel.plt -R .rel.dyn $objfile $out"]
+ set exec_output2 [binutils_run $READELF "-S $out"]
+ if { [string match "*.rel.plt*" $exec_output2] || [string match "*.rela.plt*" $exec_output2] || [string match "*.rel.dyn*" $exec_output2] || [string match "*.rela.dyn*" $exec_output2] } {
+ fail $test
+ return
+ }
+ pass $test
+}
+
+objcopy_remove_relocations_from_executable
--
Alan Modra
Australia Development Lab, IBM
Alan Modra
2018-09-10 13:24:24 UTC
Permalink
Post by Andrew Burgess
Post by Alan Modra
When --remove-relocations was added to objcopy with commit
d3e5f6c8f1e, objcopy lost the ability to remove dynamic relocation
sections such as .rela.plt from executables using the option
"--remove-section=.rela.plt". This patch reinstates that
functionality.
I thought it best to keep --remove-relocations as is, rather than
extending to handle dynamic relocations as per the patch in the PR,
because executables linked with --emit-relocs may have both dynamic
and non-dynamic relocations. In that case --remove-relocataions=* is
useful to remove all the non-dynamic relocations.
I think having an option that removes _almost_ all relocation sections
is going to cause user confusion. Yes, we can document it, but if we
can make it work for all relocation sections I think we probably
should.
Removing dynamic relocation sections from an executable is such a
weird thing to do that I'm not overly concerned. Also, they aren't
really removed. What actually happens is that the section disappears
from the section headers and the contents are overwritten with zeros.
Contrast that with what happens to relocation sections in a
relocatable object, where the section is removed and other sections
take up its file space.

You also might like to inspect the output of
gcc -o hello -Wl,-z,nocombreloc,--emit-relocs hello.c
before you discount the utility of the current --remove-relocations
behaviour. I call it a (perhaps accidental) feature rather than a bug
that the --emit-relocs created sections can be removed with
--remove-relocations.
Post by Andrew Burgess
Post by Alan Modra
/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
section (one starting with ".rela." or ".rel.") then this removal must
- be done using a different technique. */
+ be done using a different technique in a relocatable object. */
I agree that it's not 100% accurate, but it's near enough without
going into too much detail.
Post by Andrew Burgess
As far as I can tell through experimenting things like .rela.text are
always held as data off of the .text section no matter what the kind
of object/executable.
Before -z combreloc we often saw .rela.data of the dynamic reloc kind
in an executable.
--
Alan Modra
Australia Development Lab, IBM
Andrew Burgess
2018-09-10 15:21:32 UTC
Permalink
Hi Alan,

Thanks for taking the time to respond, I really appreciate your time.

I didn't quite follow all of the detail in your explanation, I'm
hoping you might be able to guide me through a little more as I really
do want to expand my understanding in this area.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
When --remove-relocations was added to objcopy with commit
d3e5f6c8f1e, objcopy lost the ability to remove dynamic relocation
sections such as .rela.plt from executables using the option
"--remove-section=.rela.plt". This patch reinstates that
functionality.
I thought it best to keep --remove-relocations as is, rather than
extending to handle dynamic relocations as per the patch in the PR,
because executables linked with --emit-relocs may have both dynamic
and non-dynamic relocations. In that case --remove-relocataions=* is
useful to remove all the non-dynamic relocations.
I think having an option that removes _almost_ all relocation sections
is going to cause user confusion. Yes, we can document it, but if we
can make it work for all relocation sections I think we probably
should.
Removing dynamic relocation sections from an executable is such a
weird thing to do that I'm not overly concerned. Also, they aren't
really removed. What actually happens is that the section disappears
from the section headers and the contents are overwritten with
zeros.
OK. That seems weird, but I feel like this is a deeper level of
knowledge, and I'm missing some of the simpler stuff, so, lets come
back to this later...
Post by Alan Modra
Contrast that with what happens to relocation sections in a
relocatable object, where the section is removed and other sections
take up its file space.
So, here you make a distinction between a 'relocatable object' and (I
assume) a 'non-relocatable object'.

So, I tried an experiment (this is all on x86-64 GNU/Linux):

$ cat test.c
#include <stdio.h>

volatile const char *message = "Hello World\n";

int
main ()
{
printf ("%s", message);
return 0;
}
$ gcc -o test.o -c test.c -g3 -O0
$ gcc --static -Wl,-emit-relocs -o test.x test.o -g3 -O0
$ readelf -WS test.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 38 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 57 4 8
[ 8] .rela.text RELA 0000000000000000 0b5d80 02c298 18 I 57 7 8
[10] .rela__libc_freeres_fn RELA 0000000000000000 0e2018 000f90 18 I 57 9 8
[12] .rela__libc_thread_freeres_fn RELA 0000000000000000 0e2fa8 000210 18 I 57 11 8
[15] .rela.rodata RELA 0000000000000000 0e31b8 011238 18 I 57 14 8
[17] .rela__libc_subfreeres RELA 0000000000000000 0f43f0 0000d8 18 I 57 16 8
[19] .rela__libc_IO_vtables RELA 0000000000000000 0f44c8 000fa8 18 I 57 18 8
[21] .rela__libc_atexit RELA 0000000000000000 0f5470 000018 18 I 57 20 8
[24] .rela__libc_thread_subfreeres RELA 0000000000000000 0f5488 000030 18 I 57 23 8
[26] .rela.eh_frame RELA 0000000000000000 0f54b8 005100 18 I 57 25 8
[29] .rela.tdata RELA 0000000000000000 0fa5b8 000060 18 I 57 28 8
[32] .rela.init_array RELA 0000000000000000 0fa618 000030 18 I 57 31 8
[34] .rela.fini_array RELA 0000000000000000 0fa648 000030 18 I 57 33 8
[36] .rela.data.rel.ro RELA 0000000000000000 0fa678 000270 18 I 57 35 8
[40] .rela.data RELA 0000000000000000 0fa8e8 000b40 18 I 57 39 8
[46] .rela.note.stapsdt RELA 0000000000000000 0fb428 0008d0 18 I 57 45 8
[48] .rela.debug_aranges RELA 0000000000000000 0fbcf8 000030 18 I 57 47 8
[50] .rela.debug_info RELA 0000000000000000 0fbd28 000630 18 I 57 49 8
[53] .rela.debug_line RELA 0000000000000000 0fc358 000018 18 I 57 52 8
[56] .rela.debug_macro RELA 0000000000000000 0fc370 0043e0 18 I 57 55 8
$ readelf -WS test2.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 37 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 56 4 8
[ 9] .rela__libc_freeres_fn RELA 0000000000000000 0b5d80 000f90 18 I 56 8 8
[11] .rela__libc_thread_freeres_fn RELA 0000000000000000 0b6d10 000210 18 I 56 10 8
[14] .rela.rodata RELA 0000000000000000 0b6f20 011238 18 I 56 13 8
[16] .rela__libc_subfreeres RELA 0000000000000000 0c8158 0000d8 18 I 56 15 8
[18] .rela__libc_IO_vtables RELA 0000000000000000 0c8230 000fa8 18 I 56 17 8
[20] .rela__libc_atexit RELA 0000000000000000 0c91d8 000018 18 I 56 19 8
[23] .rela__libc_thread_subfreeres RELA 0000000000000000 0c91f0 000030 18 I 56 22 8
[25] .rela.eh_frame RELA 0000000000000000 0c9220 005100 18 I 56 24 8
[28] .rela.tdata RELA 0000000000000000 0ce320 000060 18 I 56 27 8
[31] .rela.init_array RELA 0000000000000000 0ce380 000030 18 I 56 30 8
[33] .rela.fini_array RELA 0000000000000000 0ce3b0 000030 18 I 56 32 8
[35] .rela.data.rel.ro RELA 0000000000000000 0ce3e0 000270 18 I 56 34 8
[39] .rela.data RELA 0000000000000000 0ce650 000b40 18 I 56 38 8
[45] .rela.note.stapsdt RELA 0000000000000000 0cf190 0008d0 18 I 56 44 8
[47] .rela.debug_aranges RELA 0000000000000000 0cfa60 000030 18 I 56 46 8
[49] .rela.debug_info RELA 0000000000000000 0cfa90 000630 18 I 56 48 8
[52] .rela.debug_line RELA 0000000000000000 0d00c0 000018 18 I 56 51 8
[55] .rela.debug_macro RELA 0000000000000000 0d00d8 0043e0 18 I 56 54 8

This is a non-dynamic object (as far as I understand it) and notice
that relocation section .rela__libc_freeres_fn moved to offset 0xb5d80
to replace .rela.text once it was removed.

Using the same method I also tested building in these configurations:

$ gcc -fPIC -shared -Wl,-emit-relocs -o test.x test.c -g3 -O0
$ gcc -g3 -O0 -o test.x -Wl,-z,nocombreloc,--emit-relocs test.c
$ gcc -Wl,-emit-relocs -o test.x test.c -g3 -O0

In all cases, removing the .rela.text section caused another section
to move up to fill its place.

So, in my ignorance it appears (to me) that the distinction is between
dynamic relocs and non-dynamic relocs, not between relocatable and
non-relocatable objects.
Post by Alan Modra
You also might like to inspect the output of
gcc -o hello -Wl,-z,nocombreloc,--emit-relocs hello.c
before you discount the utility of the current --remove-relocations
behaviour.
As the person who added --remove-relocations I certainly have an
interest in its behaviour :)

As far as I can tell, --remove-relocations can remove anything except
dynamic relocations both before and after your patch.

It certainly was never my intention to introduce such an exception,
and if I had understood this limitation at the time of the original
patch I would have tried to remove the limitation.

However, as you can see above I included the 'nocombreloc' option in
one of my test cases, but without more of a clue I'm not sure what it
is that I'm supposed to be observing. As far as I can tell an object
built with that option responds to remove-relocations just like any
other.
Post by Alan Modra
I call it a (perhaps accidental) feature rather than a bug
that the --emit-relocs created sections can be removed with
--remove-relocations.
I kind of feel like that's a pretty meta can-of-worms to open. If
emit-relocs didn't emit reloc sections that could be removed with
remove-relocations then I would have implemented remove-relocations
differently in order to allow that to be the case...

The original motivation behind --remove-relocations was that I had a
client who wanted to remove a relocation section. Before this option
the only relocation sections that could be removed were dynamic
relocation sections, something I didn't realise, and so I concluded
that NO relocation sections could be removed.

Then --remove-relocations was born and all was good. Except that I
managed to break the ability to remove dynamic relocation sections.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
section (one starting with ".rela." or ".rel.") then this removal must
- be done using a different technique. */
+ be done using a different technique in a relocatable object. */
I agree that it's not 100% accurate, but it's near enough without
going into too much detail.
So, I started to write here:

".... I don't see what 'relocatable objects' have to do with this
at all, when it is 'dynamic relocations' that are the problem."

and then it struck me. I think what you're getting at is that,
without emit-relocs, the only relocations you'd expect to see in a
relocatable object are dynamic relocations, and so, in your comment,
relocatable object implies dynamic relocations.

However, wouldn't this mean that your comment should be reversed?

The original comment said that this function deals with requests to
remove a section, except that relocation sections must be handled
differently.

You've changed this to say that, .... relocation sections must be
handled differently in a relocatable object.

And if we apply the assumption that 'in a relocatable object' means
'for dynamic relocations', then this says that dynamic relocation
sections must be handled differently. Except this isn't true, dynamic
relocation sections must be handled just like any other section, it's
non-dynamic relocation sections that must be handled
differently. Right?

One further clarification, the client that I originally did the work
for uses --emit-relocs extensively, so when I wrote this code, and
when I've been thinking about it, I'm generally imagining an
object/executable with all the relocations preserved. This I guess,
is why I'm so keen to distinguish between different relocation types
requiring different handling, instead of different object types.

Thanks for your time,

Andrew
Alan Modra
2018-09-11 00:21:38 UTC
Permalink
Post by Andrew Burgess
Hi Alan,
Thanks for taking the time to respond, I really appreciate your time.
I didn't quite follow all of the detail in your explanation, I'm
hoping you might be able to guide me through a little more as I really
do want to expand my understanding in this area.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
When --remove-relocations was added to objcopy with commit
d3e5f6c8f1e, objcopy lost the ability to remove dynamic relocation
sections such as .rela.plt from executables using the option
"--remove-section=.rela.plt". This patch reinstates that
functionality.
I thought it best to keep --remove-relocations as is, rather than
extending to handle dynamic relocations as per the patch in the PR,
because executables linked with --emit-relocs may have both dynamic
and non-dynamic relocations. In that case --remove-relocataions=* is
useful to remove all the non-dynamic relocations.
I think having an option that removes _almost_ all relocation sections
is going to cause user confusion. Yes, we can document it, but if we
can make it work for all relocation sections I think we probably
should.
Removing dynamic relocation sections from an executable is such a
weird thing to do that I'm not overly concerned. Also, they aren't
really removed. What actually happens is that the section disappears
from the section headers and the contents are overwritten with zeros.
OK. That seems weird, but I feel like this is a deeper level of
knowledge, and I'm missing some of the simpler stuff, so, lets come
back to this later...
Post by Alan Modra
Contrast that with what happens to relocation sections in a
relocatable object, where the section is removed and other sections
take up its file space.
So, here you make a distinction between a 'relocatable object' and (I
assume) a 'non-relocatable object'.
By "relocatable object" I mean an object file that hasn't been through
a final linking stage, typically a .o file, eg. the output of
"gcc -c hello.c".
Post by Andrew Burgess
$ cat test.c
#include <stdio.h>
volatile const char *message = "Hello World\n";
int
main ()
{
printf ("%s", message);
return 0;
}
$ gcc -o test.o -c test.c -g3 -O0
So test.o here is a relocatable object file.
Post by Andrew Burgess
$ gcc --static -Wl,-emit-relocs -o test.x test.o -g3 -O0
test.x on the other hand is a final linked static executable, but with
relocations emitted corresponding to those in each input object
including those extracted from libraries. --emit-relocs may transform
the input file relocations if and when the linker edits code
sequences, and may emit relocations to describe linker added stub code
too.
Post by Andrew Burgess
$ readelf -WS test.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 38 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 57 4 8
[ 8] .rela.text RELA 0000000000000000 0b5d80 02c298 18 I 57 7 8
[10] .rela__libc_freeres_fn RELA 0000000000000000 0e2018 000f90 18 I 57 9 8
[12] .rela__libc_thread_freeres_fn RELA 0000000000000000 0e2fa8 000210 18 I 57 11 8
[15] .rela.rodata RELA 0000000000000000 0e31b8 011238 18 I 57 14 8
[17] .rela__libc_subfreeres RELA 0000000000000000 0f43f0 0000d8 18 I 57 16 8
[19] .rela__libc_IO_vtables RELA 0000000000000000 0f44c8 000fa8 18 I 57 18 8
[21] .rela__libc_atexit RELA 0000000000000000 0f5470 000018 18 I 57 20 8
[24] .rela__libc_thread_subfreeres RELA 0000000000000000 0f5488 000030 18 I 57 23 8
[26] .rela.eh_frame RELA 0000000000000000 0f54b8 005100 18 I 57 25 8
[29] .rela.tdata RELA 0000000000000000 0fa5b8 000060 18 I 57 28 8
[32] .rela.init_array RELA 0000000000000000 0fa618 000030 18 I 57 31 8
[34] .rela.fini_array RELA 0000000000000000 0fa648 000030 18 I 57 33 8
[36] .rela.data.rel.ro RELA 0000000000000000 0fa678 000270 18 I 57 35 8
[40] .rela.data RELA 0000000000000000 0fa8e8 000b40 18 I 57 39 8
[46] .rela.note.stapsdt RELA 0000000000000000 0fb428 0008d0 18 I 57 45 8
[48] .rela.debug_aranges RELA 0000000000000000 0fbcf8 000030 18 I 57 47 8
[50] .rela.debug_info RELA 0000000000000000 0fbd28 000630 18 I 57 49 8
[53] .rela.debug_line RELA 0000000000000000 0fc358 000018 18 I 57 52 8
[56] .rela.debug_macro RELA 0000000000000000 0fc370 0043e0 18 I 57 55 8
$ readelf -WS test2.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 37 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 56 4 8
[ 9] .rela__libc_freeres_fn RELA 0000000000000000 0b5d80 000f90 18 I 56 8 8
[11] .rela__libc_thread_freeres_fn RELA 0000000000000000 0b6d10 000210 18 I 56 10 8
[14] .rela.rodata RELA 0000000000000000 0b6f20 011238 18 I 56 13 8
[16] .rela__libc_subfreeres RELA 0000000000000000 0c8158 0000d8 18 I 56 15 8
[18] .rela__libc_IO_vtables RELA 0000000000000000 0c8230 000fa8 18 I 56 17 8
[20] .rela__libc_atexit RELA 0000000000000000 0c91d8 000018 18 I 56 19 8
[23] .rela__libc_thread_subfreeres RELA 0000000000000000 0c91f0 000030 18 I 56 22 8
[25] .rela.eh_frame RELA 0000000000000000 0c9220 005100 18 I 56 24 8
[28] .rela.tdata RELA 0000000000000000 0ce320 000060 18 I 56 27 8
[31] .rela.init_array RELA 0000000000000000 0ce380 000030 18 I 56 30 8
[33] .rela.fini_array RELA 0000000000000000 0ce3b0 000030 18 I 56 32 8
[35] .rela.data.rel.ro RELA 0000000000000000 0ce3e0 000270 18 I 56 34 8
[39] .rela.data RELA 0000000000000000 0ce650 000b40 18 I 56 38 8
[45] .rela.note.stapsdt RELA 0000000000000000 0cf190 0008d0 18 I 56 44 8
[47] .rela.debug_aranges RELA 0000000000000000 0cfa60 000030 18 I 56 46 8
[49] .rela.debug_info RELA 0000000000000000 0cfa90 000630 18 I 56 48 8
[52] .rela.debug_line RELA 0000000000000000 0d00c0 000018 18 I 56 51 8
[55] .rela.debug_macro RELA 0000000000000000 0d00d8 0043e0 18 I 56 54 8
This is a non-dynamic object (as far as I understand it) and notice
that relocation section .rela__libc_freeres_fn moved to offset 0xb5d80
to replace .rela.text once it was removed.
$ gcc -fPIC -shared -Wl,-emit-relocs -o test.x test.c -g3 -O0
$ gcc -g3 -O0 -o test.x -Wl,-z,nocombreloc,--emit-relocs test.c
$ gcc -Wl,-emit-relocs -o test.x test.c -g3 -O0
In all cases, removing the .rela.text section caused another section
to move up to fill its place.
So, in my ignorance it appears (to me) that the distinction is between
dynamic relocs and non-dynamic relocs, not between relocatable and
non-relocatable objects.
True, but typically people don't use --emit-relocs unless they are
running some post-link optimization or analysis. So the usual place
to find non-dynamic relocations is in relocatable object files.
Post by Andrew Burgess
Post by Alan Modra
You also might like to inspect the output of
gcc -o hello -Wl,-z,nocombreloc,--emit-relocs hello.c
before you discount the utility of the current --remove-relocations
behaviour.
As the person who added --remove-relocations I certainly have an
interest in its behaviour :)
Yes, I understand. :)
Post by Andrew Burgess
As far as I can tell, --remove-relocations can remove anything except
dynamic relocations both before and after your patch.
It certainly was never my intention to introduce such an exception,
and if I had understood this limitation at the time of the original
patch I would have tried to remove the limitation.
However, as you can see above I included the 'nocombreloc' option in
one of my test cases, but without more of a clue I'm not sure what it
is that I'm supposed to be observing. As far as I can tell an object
built with that option responds to remove-relocations just like any
other.
With -Wl,-z,nocombreloc,--emit-relocs" and *not* "-static" you'll see
multiple relocation sections with the same name. For example there
will likely be two .rela.data sections, one for dynamic relocations
(that utilise the .dynsym symbol table and are loaded into the process
image), and one for non-dynamic relocations output by --emit-relocs
(that use .symtab and are non-alloc).
Post by Andrew Burgess
Post by Alan Modra
I call it a (perhaps accidental) feature rather than a bug
that the --emit-relocs created sections can be removed with
--remove-relocations.
I kind of feel like that's a pretty meta can-of-worms to open.
That's true. I was going to apply a cleaned up version (attached) of
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.

Incidentally, another wart is that not all relocation sections start
with ".rela." or ".rel.". A section FOO has .relaFOO or .relFOO for
relocations. You might like to fix that, patch preapproved.
Post by Andrew Burgess
If
emit-relocs didn't emit reloc sections that could be removed with
remove-relocations then I would have implemented remove-relocations
differently in order to allow that to be the case...
Yes, but it was implemented in a way that didn't allow removal of
dynamic relocataions, and I think that is a good thing. Other people
may have discovered it allows you to reverse the action of
ld --emit-relocs and made use of the feature. If we remove that
capability we'll probably get bug reports that --remove-relocations is
broken!
Post by Andrew Burgess
The original motivation behind --remove-relocations was that I had a
client who wanted to remove a relocation section. Before this option
the only relocation sections that could be removed were dynamic
relocation sections, something I didn't realise, and so I concluded
that NO relocation sections could be removed.
Then --remove-relocations was born and all was good. Except that I
managed to break the ability to remove dynamic relocation sections.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
section (one starting with ".rela." or ".rel.") then this removal must
- be done using a different technique. */
+ be done using a different technique in a relocatable object. */
I agree that it's not 100% accurate, but it's near enough without
going into too much detail.
".... I don't see what 'relocatable objects' have to do with this
at all, when it is 'dynamic relocations' that are the problem."
and then it struck me. I think what you're getting at is that,
without emit-relocs, the only relocations you'd expect to see in a
relocatable object are dynamic relocations, and so, in your comment,
relocatable object implies dynamic relocations.
No, generally "relocatable object" means the output of "gcc -c" or
"ld -r".
Post by Andrew Burgess
However, wouldn't this mean that your comment should be reversed?
The original comment said that this function deals with requests to
remove a section, except that relocation sections must be handled
differently.
You've changed this to say that, .... relocation sections must be
handled differently in a relocatable object.
And if we apply the assumption that 'in a relocatable object' means
'for dynamic relocations', then this says that dynamic relocation
sections must be handled differently. Except this isn't true, dynamic
relocation sections must be handled just like any other section, it's
non-dynamic relocation sections that must be handled
differently. Right?
One further clarification, the client that I originally did the work
for uses --emit-relocs extensively, so when I wrote this code, and
when I've been thinking about it, I'm generally imagining an
object/executable with all the relocations preserved. This I guess,
is why I'm so keen to distinguish between different relocation types
requiring different handling, instead of different object types.
Thanks for your time,
Andrew
--
Alan Modra
Australia Development Lab, IBM
Umesh Kalappa
2018-09-11 07:50:54 UTC
Permalink
Post by Alan Modra
Post by Alan Modra
Post by Alan Modra
That's true. I was going to apply a cleaned up version (attached) of
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.

You mean Kamlesh patch is reverted back ?
Post by Alan Modra
Post by Alan Modra
Incidentally, another wart is that not all relocation sections start
with ".rela." or ".rel.". A section FOO has .relaFOO or .relFOO for
relocations. You might like to fix that, patch preapproved.

Andrew are looking at this or you want us handle the above case ?

Thank you
~Umesh
Post by Alan Modra
Post by Alan Modra
Hi Alan,
Thanks for taking the time to respond, I really appreciate your time.
I didn't quite follow all of the detail in your explanation, I'm
hoping you might be able to guide me through a little more as I really
do want to expand my understanding in this area.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
When --remove-relocations was added to objcopy with commit
d3e5f6c8f1e, objcopy lost the ability to remove dynamic relocation
sections such as .rela.plt from executables using the option
"--remove-section=.rela.plt". This patch reinstates that
functionality.
I thought it best to keep --remove-relocations as is, rather than
extending to handle dynamic relocations as per the patch in the PR,
because executables linked with --emit-relocs may have both dynamic
and non-dynamic relocations. In that case --remove-relocataions=* is
useful to remove all the non-dynamic relocations.
I think having an option that removes _almost_ all relocation sections
is going to cause user confusion. Yes, we can document it, but if we
can make it work for all relocation sections I think we probably
should.
Removing dynamic relocation sections from an executable is such a
weird thing to do that I'm not overly concerned. Also, they aren't
really removed. What actually happens is that the section disappears
from the section headers and the contents are overwritten with zeros.
OK. That seems weird, but I feel like this is a deeper level of
knowledge, and I'm missing some of the simpler stuff, so, lets come
back to this later...
Post by Alan Modra
Contrast that with what happens to relocation sections in a
relocatable object, where the section is removed and other sections
take up its file space.
So, here you make a distinction between a 'relocatable object' and (I
assume) a 'non-relocatable object'.
By "relocatable object" I mean an object file that hasn't been through
a final linking stage, typically a .o file, eg. the output of
"gcc -c hello.c".
Post by Alan Modra
$ cat test.c
#include <stdio.h>
volatile const char *message = "Hello World\n";
int
main ()
{
printf ("%s", message);
return 0;
}
$ gcc -o test.o -c test.c -g3 -O0
So test.o here is a relocatable object file.
Post by Alan Modra
$ gcc --static -Wl,-emit-relocs -o test.x test.o -g3 -O0
test.x on the other hand is a final linked static executable, but with
relocations emitted corresponding to those in each input object
including those extracted from libraries. --emit-relocs may transform
the input file relocations if and when the linker edits code
sequences, and may emit relocations to describe linker added stub code
too.
Post by Alan Modra
$ readelf -WS test.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 38 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 57 4 8
[ 8] .rela.text RELA 0000000000000000 0b5d80 02c298 18 I 57 7 8
[10] .rela__libc_freeres_fn RELA 0000000000000000 0e2018 000f90 18 I 57 9 8
[12] .rela__libc_thread_freeres_fn RELA 0000000000000000 0e2fa8 000210 18 I 57 11 8
[15] .rela.rodata RELA 0000000000000000 0e31b8 011238 18 I 57 14 8
[17] .rela__libc_subfreeres RELA 0000000000000000 0f43f0 0000d8 18 I 57 16 8
[19] .rela__libc_IO_vtables RELA 0000000000000000 0f44c8 000fa8 18 I 57 18 8
[21] .rela__libc_atexit RELA 0000000000000000 0f5470 000018 18 I 57 20 8
[24] .rela__libc_thread_subfreeres RELA 0000000000000000 0f5488 000030 18 I 57 23 8
[26] .rela.eh_frame RELA 0000000000000000 0f54b8 005100 18 I 57 25 8
[29] .rela.tdata RELA 0000000000000000 0fa5b8 000060 18 I 57 28 8
[32] .rela.init_array RELA 0000000000000000 0fa618 000030 18 I 57 31 8
[34] .rela.fini_array RELA 0000000000000000 0fa648 000030 18 I 57 33 8
[36] .rela.data.rel.ro RELA 0000000000000000 0fa678 000270 18 I 57 35 8
[40] .rela.data RELA 0000000000000000 0fa8e8 000b40 18 I 57 39 8
[46] .rela.note.stapsdt RELA 0000000000000000 0fb428 0008d0 18 I 57 45 8
[48] .rela.debug_aranges RELA 0000000000000000 0fbcf8 000030 18 I 57 47 8
[50] .rela.debug_info RELA 0000000000000000 0fbd28 000630 18 I 57 49 8
[53] .rela.debug_line RELA 0000000000000000 0fc358 000018 18 I 57 52 8
[56] .rela.debug_macro RELA 0000000000000000 0fc370 0043e0 18 I 57 55 8
$ readelf -WS test2.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 37 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 56 4 8
[ 9] .rela__libc_freeres_fn RELA 0000000000000000 0b5d80 000f90 18 I 56 8 8
[11] .rela__libc_thread_freeres_fn RELA 0000000000000000 0b6d10 000210 18 I 56 10 8
[14] .rela.rodata RELA 0000000000000000 0b6f20 011238 18 I 56 13 8
[16] .rela__libc_subfreeres RELA 0000000000000000 0c8158 0000d8 18 I 56 15 8
[18] .rela__libc_IO_vtables RELA 0000000000000000 0c8230 000fa8 18 I 56 17 8
[20] .rela__libc_atexit RELA 0000000000000000 0c91d8 000018 18 I 56 19 8
[23] .rela__libc_thread_subfreeres RELA 0000000000000000 0c91f0 000030 18 I 56 22 8
[25] .rela.eh_frame RELA 0000000000000000 0c9220 005100 18 I 56 24 8
[28] .rela.tdata RELA 0000000000000000 0ce320 000060 18 I 56 27 8
[31] .rela.init_array RELA 0000000000000000 0ce380 000030 18 I 56 30 8
[33] .rela.fini_array RELA 0000000000000000 0ce3b0 000030 18 I 56 32 8
[35] .rela.data.rel.ro RELA 0000000000000000 0ce3e0 000270 18 I 56 34 8
[39] .rela.data RELA 0000000000000000 0ce650 000b40 18 I 56 38 8
[45] .rela.note.stapsdt RELA 0000000000000000 0cf190 0008d0 18 I 56 44 8
[47] .rela.debug_aranges RELA 0000000000000000 0cfa60 000030 18 I 56 46 8
[49] .rela.debug_info RELA 0000000000000000 0cfa90 000630 18 I 56 48 8
[52] .rela.debug_line RELA 0000000000000000 0d00c0 000018 18 I 56 51 8
[55] .rela.debug_macro RELA 0000000000000000 0d00d8 0043e0 18 I 56 54 8
This is a non-dynamic object (as far as I understand it) and notice
that relocation section .rela__libc_freeres_fn moved to offset 0xb5d80
to replace .rela.text once it was removed.
$ gcc -fPIC -shared -Wl,-emit-relocs -o test.x test.c -g3 -O0
$ gcc -g3 -O0 -o test.x -Wl,-z,nocombreloc,--emit-relocs test.c
$ gcc -Wl,-emit-relocs -o test.x test.c -g3 -O0
In all cases, removing the .rela.text section caused another section
to move up to fill its place.
So, in my ignorance it appears (to me) that the distinction is between
dynamic relocs and non-dynamic relocs, not between relocatable and
non-relocatable objects.
True, but typically people don't use --emit-relocs unless they are
running some post-link optimization or analysis. So the usual place
to find non-dynamic relocations is in relocatable object files.
Post by Alan Modra
Post by Alan Modra
You also might like to inspect the output of
gcc -o hello -Wl,-z,nocombreloc,--emit-relocs hello.c
before you discount the utility of the current --remove-relocations
behaviour.
As the person who added --remove-relocations I certainly have an
interest in its behaviour :)
Yes, I understand. :)
Post by Alan Modra
As far as I can tell, --remove-relocations can remove anything except
dynamic relocations both before and after your patch.
It certainly was never my intention to introduce such an exception,
and if I had understood this limitation at the time of the original
patch I would have tried to remove the limitation.
However, as you can see above I included the 'nocombreloc' option in
one of my test cases, but without more of a clue I'm not sure what it
is that I'm supposed to be observing. As far as I can tell an object
built with that option responds to remove-relocations just like any
other.
With -Wl,-z,nocombreloc,--emit-relocs" and *not* "-static" you'll see
multiple relocation sections with the same name. For example there
will likely be two .rela.data sections, one for dynamic relocations
(that utilise the .dynsym symbol table and are loaded into the process
image), and one for non-dynamic relocations output by --emit-relocs
(that use .symtab and are non-alloc).
Post by Alan Modra
Post by Alan Modra
I call it a (perhaps accidental) feature rather than a bug
that the --emit-relocs created sections can be removed with
--remove-relocations.
I kind of feel like that's a pretty meta can-of-worms to open.
That's true. I was going to apply a cleaned up version (attached) of
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.
Incidentally, another wart is that not all relocation sections start
with ".rela." or ".rel.". A section FOO has .relaFOO or .relFOO for
relocations. You might like to fix that, patch preapproved.
Post by Alan Modra
If
emit-relocs didn't emit reloc sections that could be removed with
remove-relocations then I would have implemented remove-relocations
differently in order to allow that to be the case...
Yes, but it was implemented in a way that didn't allow removal of
dynamic relocataions, and I think that is a good thing. Other people
may have discovered it allows you to reverse the action of
ld --emit-relocs and made use of the feature. If we remove that
capability we'll probably get bug reports that --remove-relocations is
broken!
Post by Alan Modra
The original motivation behind --remove-relocations was that I had a
client who wanted to remove a relocation section. Before this option
the only relocation sections that could be removed were dynamic
relocation sections, something I didn't realise, and so I concluded
that NO relocation sections could be removed.
Then --remove-relocations was born and all was good. Except that I
managed to break the ability to remove dynamic relocation sections.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
section (one starting with ".rela." or ".rel.") then this removal must
- be done using a different technique. */
+ be done using a different technique in a relocatable object. */
I agree that it's not 100% accurate, but it's near enough without
going into too much detail.
".... I don't see what 'relocatable objects' have to do with this
at all, when it is 'dynamic relocations' that are the problem."
and then it struck me. I think what you're getting at is that,
without emit-relocs, the only relocations you'd expect to see in a
relocatable object are dynamic relocations, and so, in your comment,
relocatable object implies dynamic relocations.
No, generally "relocatable object" means the output of "gcc -c" or
"ld -r".
Post by Alan Modra
However, wouldn't this mean that your comment should be reversed?
The original comment said that this function deals with requests to
remove a section, except that relocation sections must be handled
differently.
You've changed this to say that, .... relocation sections must be
handled differently in a relocatable object.
And if we apply the assumption that 'in a relocatable object' means
'for dynamic relocations', then this says that dynamic relocation
sections must be handled differently. Except this isn't true, dynamic
relocation sections must be handled just like any other section, it's
non-dynamic relocation sections that must be handled
differently. Right?
One further clarification, the client that I originally did the work
for uses --emit-relocs extensively, so when I wrote this code, and
when I've been thinking about it, I'm generally imagining an
object/executable with all the relocations preserved. This I guess,
is why I'm so keen to distinguish between different relocation types
requiring different handling, instead of different object types.
Thanks for your time,
Andrew
--
Alan Modra
Australia Development Lab, IBM
Alan Modra
2018-09-11 10:49:29 UTC
Permalink
Post by Alan Modra
Post by Alan Modra
That's true. I was going to apply a cleaned up version (attached) of
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.
You mean Kamlesh patch is reverted back ?
I applied a different patch to fix the PR. You can again use
objcopy --remove-section=.rela.dyn to remove .rela.dyn.
--
Alan Modra
Australia Development Lab, IBM
Umesh Kalappa
2018-09-11 11:25:39 UTC
Permalink
Alan ,
Post by Alan Modra
Post by Alan Modra
Post by Alan Modra
I applied a different patch to fix the PR.
The following commit ?

commit f9853190c8d90e9f7d43fae90478a0db291f9c03
Author: Alan Modra <***@gmail.com>
Date: Mon Sep 10 11:57:08 2018 +0930

PR23611, objcopy is not removing executable relocatable sections

Thank you
~Umesh
Post by Alan Modra
Post by Alan Modra
Post by Alan Modra
Post by Alan Modra
That's true. I was going to apply a cleaned up version (attached) of
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.
You mean Kamlesh patch is reverted back ?
I applied a different patch to fix the PR. You can again use
objcopy --remove-section=.rela.dyn to remove .rela.dyn.
--
Alan Modra
Australia Development Lab, IBM
Alan Modra
2018-09-11 15:19:39 UTC
Permalink
Post by Umesh Kalappa
Post by Alan Modra
I applied a different patch to fix the PR.
The following commit ?
commit f9853190c8d90e9f7d43fae90478a0db291f9c03
Yes.
--
Alan Modra
Australia Development Lab, IBM
Michael Matz
2018-09-11 16:43:00 UTC
Permalink
Hi,
Post by Alan Modra
Incidentally, another wart is that not all relocation sections start
with ".rela." or ".rel.". A section FOO has .relaFOO or .relFOO for
relocations.
As it should be. The second '.' in ".rela.text" comes from ".text" not
from ".rela.". If $section is "FOO" the relocation section should be
named .relaFOO, and if it's "%bar" it should be named ".rela%bar"
(assuming '%' is a valid section name character of course :) ).
Everything else introduces special cases (e.g. what if there's a "FOO" and
a ".FOO" section, vs. if there's _only_ a ".FOO" section?).
Post by Alan Modra
You might like to fix that, patch preapproved.
Ugh, please don't.


Ciao,
Michael.
Alan Modra
2018-09-12 02:34:42 UTC
Permalink
Post by Michael Matz
Hi,
Post by Alan Modra
Incidentally, another wart is that not all relocation sections start
with ".rela." or ".rel.". A section FOO has .relaFOO or .relFOO for
relocations.
As it should be. The second '.' in ".rela.text" comes from ".text" not
from ".rela.". If $section is "FOO" the relocation section should be
named .relaFOO, and if it's "%bar" it should be named ".rela%bar"
(assuming '%' is a valid section name character of course :) ).
We are in agreement up to this point.
Post by Michael Matz
Everything else introduces special cases (e.g. what if there's a "FOO" and
a ".FOO" section, vs. if there's _only_ a ".FOO" section?).
Post by Alan Modra
You might like to fix that, patch preapproved.
Ugh, please don't.
At the moment "objcopy -R .relaFOO -R .relFOO" doesn't remove
relocations for section "FOO" in a relocatable object file. Why
shouldn't we make that work?
--
Alan Modra
Australia Development Lab, IBM
Michael Matz
2018-09-12 12:06:49 UTC
Permalink
Hi,
Post by Alan Modra
Post by Michael Matz
Post by Alan Modra
Incidentally, another wart is that not all relocation sections start
with ".rela." or ".rel.". A section FOO has .relaFOO or .relFOO for
relocations.
As it should be. The second '.' in ".rela.text" comes from ".text" not
from ".rela.". If $section is "FOO" the relocation section should be
named .relaFOO, and if it's "%bar" it should be named ".rela%bar"
(assuming '%' is a valid section name character of course :) ).
We are in agreement up to this point.
Post by Michael Matz
Everything else introduces special cases (e.g. what if there's a "FOO" and
a ".FOO" section, vs. if there's _only_ a ".FOO" section?).
Post by Alan Modra
You might like to fix that, patch preapproved.
Ugh, please don't.
At the moment "objcopy -R .relaFOO -R .relFOO" doesn't remove
relocations for section "FOO" in a relocatable object file. Why
shouldn't we make that work?
Oh we should, I didn't get that _that's_ the wart you meant (I thought
you meant that not all reloc sections are called .rela.$name is the wart).


Ciao,
Michael.
Andrew Burgess
2018-09-12 16:31:12 UTC
Permalink
Post by Alan Modra
Post by Andrew Burgess
Hi Alan,
Thanks for taking the time to respond, I really appreciate your time.
I didn't quite follow all of the detail in your explanation, I'm
hoping you might be able to guide me through a little more as I really
do want to expand my understanding in this area.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
When --remove-relocations was added to objcopy with commit
d3e5f6c8f1e, objcopy lost the ability to remove dynamic relocation
sections such as .rela.plt from executables using the option
"--remove-section=.rela.plt". This patch reinstates that
functionality.
I thought it best to keep --remove-relocations as is, rather than
extending to handle dynamic relocations as per the patch in the PR,
because executables linked with --emit-relocs may have both dynamic
and non-dynamic relocations. In that case --remove-relocataions=* is
useful to remove all the non-dynamic relocations.
I think having an option that removes _almost_ all relocation sections
is going to cause user confusion. Yes, we can document it, but if we
can make it work for all relocation sections I think we probably
should.
Removing dynamic relocation sections from an executable is such a
weird thing to do that I'm not overly concerned. Also, they aren't
really removed. What actually happens is that the section disappears
from the section headers and the contents are overwritten with zeros.
OK. That seems weird, but I feel like this is a deeper level of
knowledge, and I'm missing some of the simpler stuff, so, lets come
back to this later...
Post by Alan Modra
Contrast that with what happens to relocation sections in a
relocatable object, where the section is removed and other sections
take up its file space.
So, here you make a distinction between a 'relocatable object' and (I
assume) a 'non-relocatable object'.
By "relocatable object" I mean an object file that hasn't been through
a final linking stage, typically a .o file, eg. the output of
"gcc -c hello.c".
Post by Andrew Burgess
$ cat test.c
#include <stdio.h>
volatile const char *message = "Hello World\n";
int
main ()
{
printf ("%s", message);
return 0;
}
$ gcc -o test.o -c test.c -g3 -O0
So test.o here is a relocatable object file.
Post by Andrew Burgess
$ gcc --static -Wl,-emit-relocs -o test.x test.o -g3 -O0
test.x on the other hand is a final linked static executable, but with
relocations emitted corresponding to those in each input object
including those extracted from libraries. --emit-relocs may transform
the input file relocations if and when the linker edits code
sequences, and may emit relocations to describe linker added stub code
too.
Post by Andrew Burgess
$ readelf -WS test.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 38 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 57 4 8
[ 8] .rela.text RELA 0000000000000000 0b5d80 02c298 18 I 57 7 8
[10] .rela__libc_freeres_fn RELA 0000000000000000 0e2018 000f90 18 I 57 9 8
[12] .rela__libc_thread_freeres_fn RELA 0000000000000000 0e2fa8 000210 18 I 57 11 8
[15] .rela.rodata RELA 0000000000000000 0e31b8 011238 18 I 57 14 8
[17] .rela__libc_subfreeres RELA 0000000000000000 0f43f0 0000d8 18 I 57 16 8
[19] .rela__libc_IO_vtables RELA 0000000000000000 0f44c8 000fa8 18 I 57 18 8
[21] .rela__libc_atexit RELA 0000000000000000 0f5470 000018 18 I 57 20 8
[24] .rela__libc_thread_subfreeres RELA 0000000000000000 0f5488 000030 18 I 57 23 8
[26] .rela.eh_frame RELA 0000000000000000 0f54b8 005100 18 I 57 25 8
[29] .rela.tdata RELA 0000000000000000 0fa5b8 000060 18 I 57 28 8
[32] .rela.init_array RELA 0000000000000000 0fa618 000030 18 I 57 31 8
[34] .rela.fini_array RELA 0000000000000000 0fa648 000030 18 I 57 33 8
[36] .rela.data.rel.ro RELA 0000000000000000 0fa678 000270 18 I 57 35 8
[40] .rela.data RELA 0000000000000000 0fa8e8 000b40 18 I 57 39 8
[46] .rela.note.stapsdt RELA 0000000000000000 0fb428 0008d0 18 I 57 45 8
[48] .rela.debug_aranges RELA 0000000000000000 0fbcf8 000030 18 I 57 47 8
[50] .rela.debug_info RELA 0000000000000000 0fbd28 000630 18 I 57 49 8
[53] .rela.debug_line RELA 0000000000000000 0fc358 000018 18 I 57 52 8
[56] .rela.debug_macro RELA 0000000000000000 0fc370 0043e0 18 I 57 55 8
$ readelf -WS test2.x | grep rela.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 0001f8 18 AI 0 37 8
[ 5] .rela.init RELA 0000000000000000 0b5d68 000018 18 I 56 4 8
[ 9] .rela__libc_freeres_fn RELA 0000000000000000 0b5d80 000f90 18 I 56 8 8
[11] .rela__libc_thread_freeres_fn RELA 0000000000000000 0b6d10 000210 18 I 56 10 8
[14] .rela.rodata RELA 0000000000000000 0b6f20 011238 18 I 56 13 8
[16] .rela__libc_subfreeres RELA 0000000000000000 0c8158 0000d8 18 I 56 15 8
[18] .rela__libc_IO_vtables RELA 0000000000000000 0c8230 000fa8 18 I 56 17 8
[20] .rela__libc_atexit RELA 0000000000000000 0c91d8 000018 18 I 56 19 8
[23] .rela__libc_thread_subfreeres RELA 0000000000000000 0c91f0 000030 18 I 56 22 8
[25] .rela.eh_frame RELA 0000000000000000 0c9220 005100 18 I 56 24 8
[28] .rela.tdata RELA 0000000000000000 0ce320 000060 18 I 56 27 8
[31] .rela.init_array RELA 0000000000000000 0ce380 000030 18 I 56 30 8
[33] .rela.fini_array RELA 0000000000000000 0ce3b0 000030 18 I 56 32 8
[35] .rela.data.rel.ro RELA 0000000000000000 0ce3e0 000270 18 I 56 34 8
[39] .rela.data RELA 0000000000000000 0ce650 000b40 18 I 56 38 8
[45] .rela.note.stapsdt RELA 0000000000000000 0cf190 0008d0 18 I 56 44 8
[47] .rela.debug_aranges RELA 0000000000000000 0cfa60 000030 18 I 56 46 8
[49] .rela.debug_info RELA 0000000000000000 0cfa90 000630 18 I 56 48 8
[52] .rela.debug_line RELA 0000000000000000 0d00c0 000018 18 I 56 51 8
[55] .rela.debug_macro RELA 0000000000000000 0d00d8 0043e0 18 I 56 54 8
This is a non-dynamic object (as far as I understand it) and notice
that relocation section .rela__libc_freeres_fn moved to offset 0xb5d80
to replace .rela.text once it was removed.
$ gcc -fPIC -shared -Wl,-emit-relocs -o test.x test.c -g3 -O0
$ gcc -g3 -O0 -o test.x -Wl,-z,nocombreloc,--emit-relocs test.c
$ gcc -Wl,-emit-relocs -o test.x test.c -g3 -O0
In all cases, removing the .rela.text section caused another section
to move up to fill its place.
So, in my ignorance it appears (to me) that the distinction is between
dynamic relocs and non-dynamic relocs, not between relocatable and
non-relocatable objects.
True, but typically people don't use --emit-relocs unless they are
running some post-link optimization or analysis. So the usual place
to find non-dynamic relocations is in relocatable object files.
Post by Andrew Burgess
Post by Alan Modra
You also might like to inspect the output of
gcc -o hello -Wl,-z,nocombreloc,--emit-relocs hello.c
before you discount the utility of the current --remove-relocations
behaviour.
As the person who added --remove-relocations I certainly have an
interest in its behaviour :)
Yes, I understand. :)
Post by Andrew Burgess
As far as I can tell, --remove-relocations can remove anything except
dynamic relocations both before and after your patch.
It certainly was never my intention to introduce such an exception,
and if I had understood this limitation at the time of the original
patch I would have tried to remove the limitation.
However, as you can see above I included the 'nocombreloc' option in
one of my test cases, but without more of a clue I'm not sure what it
is that I'm supposed to be observing. As far as I can tell an object
built with that option responds to remove-relocations just like any
other.
With -Wl,-z,nocombreloc,--emit-relocs" and *not* "-static" you'll see
multiple relocation sections with the same name. For example there
will likely be two .rela.data sections, one for dynamic relocations
(that utilise the .dynsym symbol table and are loaded into the process
image), and one for non-dynamic relocations output by --emit-relocs
(that use .symtab and are non-alloc).
Post by Andrew Burgess
Post by Alan Modra
I call it a (perhaps accidental) feature rather than a bug
that the --emit-relocs created sections can be removed with
--remove-relocations.
I kind of feel like that's a pretty meta can-of-worms to open.
That's true. I was going to apply a cleaned up version (attached) of
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.
This is what puzzles me about this discussion. You saw value in
--remove-relocations NOT being able to remove some relocation
sections?

What value can that possibly have? All I see is a way to confuse
users. Sure, you documented it, but, it still seems like an
unexpected caveat, and one that's pretty easy to remove.
Post by Alan Modra
Incidentally, another wart is that not all relocation sections start
with ".rela." or ".rel.". A section FOO has .relaFOO or .relFOO for
relocations. You might like to fix that, patch preapproved.
Thanks! Patch attached at the end of this email. I push it in a few
days if I don't get any follow up.
Post by Alan Modra
Post by Andrew Burgess
If
emit-relocs didn't emit reloc sections that could be removed with
remove-relocations then I would have implemented remove-relocations
differently in order to allow that to be the case...
Yes, but it was implemented in a way that didn't allow removal of
dynamic relocataions, and I think that is a good thing. Other people
may have discovered it allows you to reverse the action of
ld --emit-relocs and made use of the feature. If we remove that
capability we'll probably get bug reports that --remove-relocations is
broken!
Why on earth would we remove that feature? The --remove-relocations
option was specifically put in to remove relocations left in by
--emit-relocs.

Now, I messed up, I admit; I didn't realise that dynamic relocations
were handled differently to non-dynamic relocations. If I had I would
have written --remove-relocations differently to remove all types of
relocation.
Post by Alan Modra
Post by Andrew Burgess
The original motivation behind --remove-relocations was that I had a
client who wanted to remove a relocation section. Before this option
the only relocation sections that could be removed were dynamic
relocation sections, something I didn't realise, and so I concluded
that NO relocation sections could be removed.
Then --remove-relocations was born and all was good. Except that I
managed to break the ability to remove dynamic relocation sections.
Post by Alan Modra
Post by Andrew Burgess
Post by Alan Modra
/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
section (one starting with ".rela." or ".rel.") then this removal must
- be done using a different technique. */
+ be done using a different technique in a relocatable object. */
I agree that it's not 100% accurate, but it's near enough without
going into too much detail.
".... I don't see what 'relocatable objects' have to do with this
at all, when it is 'dynamic relocations' that are the problem."
and then it struck me. I think what you're getting at is that,
without emit-relocs, the only relocations you'd expect to see in a
relocatable object are dynamic relocations, and so, in your comment,
relocatable object implies dynamic relocations.
No, generally "relocatable object" means the output of "gcc -c" or
"ld -r".
section (one starting with ".rela." or ".rel.") then this removal must
be done using a different technique in a relocatable object. */
section (one starting with ".rela." or ".rel.") then this removal must
be done using a different technique for non-dynamic relocations. */
The first is true in all cases, except when it's not true. And the
second comment is always true (right?)

Shouldn't comments make the code more understandable, and draw
attention to gotchas?

I understand you rejected my original proposed revision as being two
verbose, but the above is only 4 characters longer, but is completely
correct. Would you be willing to accept such a revision?

Thanks

Andrew

----

Below is a patch to fix the .rela. / .rel. prefix assumption you
pointed out.

---

objcopy: Don't assume section names start with DOT when removing relocs

When calling --remove-section=.rela.text to remove relocations from
the .text section we had an assuption that section names begin with a
'.'. If a section didn't, then the relocation section would be named,
for example .relaFOO (for a section named 'FOO'), in which case we
would fail to remove the relocations.

Fixed in this commit.

binutils/ChangeLog:

* objcopoy.c (handle_remove_section_option): Don't assume that
section names begin with '.'.
* testsuite/binutils-all/remove-relocs-07.d: New file.
* testsuite/binutils-all/remove-relocs-07.s: New file.
* testsuite/binutils-all/remove-relocs-08.d: New file.
---
binutils/ChangeLog | 8 ++++++++
binutils/objcopy.c | 6 ++++--
binutils/testsuite/binutils-all/remove-relocs-07.d | 10 ++++++++++
binutils/testsuite/binutils-all/remove-relocs-07.s | 9 +++++++++
binutils/testsuite/binutils-all/remove-relocs-08.d | 10 ++++++++++
5 files changed, 41 insertions(+), 2 deletions(-)
create mode 100644 binutils/testsuite/binutils-all/remove-relocs-07.d
create mode 100644 binutils/testsuite/binutils-all/remove-relocs-07.s
create mode 100644 binutils/testsuite/binutils-all/remove-relocs-08.d

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index f712ffe5917..8a50bebb5b1 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -3948,9 +3948,11 @@ discard_relocations (bfd *ibfd ATTRIBUTE_UNUSED, asection *isection)
static void
handle_remove_section_option (const char *section_pattern)
{
- if (strncmp (section_pattern, ".rela.", 6) == 0)
+ if (strncmp (section_pattern, ".rela", 5) == 0
+ && strlen (section_pattern) > 5)
handle_remove_relocations_option (section_pattern + 5);
- else if (strncmp (section_pattern, ".rel.", 5) == 0)
+ else if (strncmp (section_pattern, ".rel", 4) == 0
+ && strlen (section_pattern) > 4)
handle_remove_relocations_option (section_pattern + 4);

find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE);
diff --git a/binutils/testsuite/binutils-all/remove-relocs-07.d b/binutils/testsuite/binutils-all/remove-relocs-07.d
new file mode 100644
index 00000000000..5b74fe380bf
--- /dev/null
+++ b/binutils/testsuite/binutils-all/remove-relocs-07.d
@@ -0,0 +1,10 @@
+#PROG: objcopy
+#source: remove-relocs-07.s
+#objcopy: --remove-relocations=data.relocs.01
+#readelf: -r
+
+Relocation section '\.rela?data\.relocs\.02' at offset 0x[0-9a-f]+ contains 3 entries:
+.*
+.*
+.*
+.*
diff --git a/binutils/testsuite/binutils-all/remove-relocs-07.s b/binutils/testsuite/binutils-all/remove-relocs-07.s
new file mode 100644
index 00000000000..e791e087504
--- /dev/null
+++ b/binutils/testsuite/binutils-all/remove-relocs-07.s
@@ -0,0 +1,9 @@
+ .section "data.relocs.01", "aw"
+ .dc.a rel_01_01
+ .dc.a rel_01_02
+ .dc.a rel_01_03
+
+ .section "data.relocs.02", "aw"
+ .dc.a rel_02_01
+ .dc.a rel_02_02
+ .dc.a rel_02_03
diff --git a/binutils/testsuite/binutils-all/remove-relocs-08.d b/binutils/testsuite/binutils-all/remove-relocs-08.d
new file mode 100644
index 00000000000..ab0ea5d266d
--- /dev/null
+++ b/binutils/testsuite/binutils-all/remove-relocs-08.d
@@ -0,0 +1,10 @@
+#PROG: objcopy
+#source: remove-relocs-07.s
+#objcopy: --remove-section=.reladata.relocs.01 --remove-section=.reldata.relocs.01
+#readelf: -r
+
+Relocation section '\.rela?data\.relocs\.02' at offset 0x[0-9a-f]+ contains 3 entries:
+.*
+.*
+.*
+.*
--
2.14.4
Michael Matz
2018-09-14 12:51:12 UTC
Permalink
Hi,
Post by Andrew Burgess
Post by Alan Modra
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.
This is what puzzles me about this discussion. You saw value in
--remove-relocations NOT being able to remove some relocation
sections?
What value can that possibly have?
relocations emitted by --emit-relocs into DSOs aren't necessary at runtime
(which is why they aren't emitted by default), dynamic relocations are.
As the latter are part of the process image they aren't actually just
placed in sections but also in segments. You can't remove such pieces
easily, you could only nullify them. If you do that the result will stop
working. So I'd ask the opposite question: what value can removing
dynamic relocations possibly have?


Ciao,
Michael.
Andrew Burgess
2018-09-14 21:33:48 UTC
Permalink
Post by Michael Matz
Hi,
Post by Andrew Burgess
Post by Alan Modra
Kamlesh's patch to make --remove-relocations remove all types of reloc
section, but on reconsidering decided the existing behaviour was worth
keeping despite needing to document it.
This is what puzzles me about this discussion. You saw value in
--remove-relocations NOT being able to remove some relocation
sections?
What value can that possibly have?
relocations emitted by --emit-relocs into DSOs aren't necessary at runtime
(which is why they aren't emitted by default), dynamic relocations are.
As the latter are part of the process image they aren't actually just
placed in sections but also in segments. You can't remove such pieces
easily, you could only nullify them. If you do that the result will stop
working. So I'd ask the opposite question: what value can removing
dynamic relocations possibly have?
I don't have an answer to that question, but I don't see the value in
asking the question.

Alan, did make --remove-section=... work for dynamic relocation
sections, that was the issue that triggered this whole email chain, so
making the point that removing dynamic relocations is weird or
pointless (though probably true) has very little to do with this
discussion.

Further, many of the options in objcopy can leave an object in a state
that might be considered weird or broken, for example
'--remove-section=.text'. I see objcopy as a general purpose object
manipulation tool, I'd just like to present a consistent interface.

However, while thinking about this over the last couple of days I can
see one use case in which the restriction on not removing dynamic
relocations is helpful.

When relocations have been left in with --emit-relocs, if the user now
wants to remove them, but leave dynamic relocations in place, then,
'--remove-relocations=*' will do the job. This at least provides some
reason for making the current choice. The inconsistent interface
still irritates a little, but I'm sure that will pass with time :)

Thanks,
Andrew
Alan Modra
2018-09-17 14:09:06 UTC
Permalink
Post by Andrew Burgess
* objcopoy.c (handle_remove_section_option): Don't assume that
section names begin with '.'.
* testsuite/binutils-all/remove-relocs-07.d: New file.
* testsuite/binutils-all/remove-relocs-07.s: New file.
* testsuite/binutils-all/remove-relocs-08.d: New file.
I'm going to commit this patch instead, which I wrote after Michael
posted his objection and before you posted your patch. The main
change here is the testsuite fixes, and new tests that don't need to
exclude mips64-openbsd.

* objcopy.c (handle_remove_section_option): Don't require a dot
after .rela and .rel to handle a possible relocation section.
* testsuite/binutils-all/remove-relocs-07.s,
* testsuite/binutils-all/remove-relocs-07.d,
* testsuite/binutils-all/remove-relocs-08.d: New tests.
* testsuite/binutils-all/remove-relocs-01.d,
* testsuite/binutils-all/remove-relocs-04.d,
* testsuite/binutils-all/remove-relocs-05.d,
* testsuite/binutils-all/remove-relocs-06.d: Exclude mips64-openbsd.

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 8e06cd284f..2e40b42da3 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -3943,18 +3943,21 @@ discard_relocations (bfd *ibfd ATTRIBUTE_UNUSED, asection *isection)

/* Wrapper for dealing with --remove-section (-R) command line arguments.
A special case is detected here, if the user asks to remove a relocation
- section (one starting with ".rela." or ".rel.") then this removal must
+ section (one starting with ".rela" or ".rel") then this removal must
be done using a different technique in a relocatable object. */

static void
handle_remove_section_option (const char *section_pattern)
{
- if (strncmp (section_pattern, ".rela.", 6) == 0)
- handle_remove_relocations_option (section_pattern + 5);
- else if (strncmp (section_pattern, ".rel.", 5) == 0)
- handle_remove_relocations_option (section_pattern + 4);
-
find_section_list (section_pattern, TRUE, SECTION_CONTEXT_REMOVE);
+ if (strncmp (section_pattern, ".rel", 4) == 0)
+ {
+ section_pattern += 4;
+ if (*section_pattern == 'a')
+ section_pattern++;
+ if (*section_pattern)
+ handle_remove_relocations_option (section_pattern);
+ }
sections_removed = TRUE;
}

diff --git a/binutils/testsuite/binutils-all/remove-relocs-01.d b/binutils/testsuite/binutils-all/remove-relocs-01.d
index 9cd0bfeb63..702747bc3f 100644
--- a/binutils/testsuite/binutils-all/remove-relocs-01.d
+++ b/binutils/testsuite/binutils-all/remove-relocs-01.d
@@ -2,6 +2,7 @@
#source: remove-relocs-01.s
#objcopy: --remove-relocations=.data.relocs.01
#readelf: -r
+#notarget: "mips64*-*-openbsd*"

Relocation section '\.rela?\.data\.relocs\.02' at offset 0x[0-9a-f]+ contains 3 entries:
.*
diff --git a/binutils/testsuite/binutils-all/remove-relocs-04.d b/binutils/testsuite/binutils-all/remove-relocs-04.d
index 99f5a6146e..1b8eab39a1 100644
--- a/binutils/testsuite/binutils-all/remove-relocs-04.d
+++ b/binutils/testsuite/binutils-all/remove-relocs-04.d
@@ -2,6 +2,7 @@
#source: remove-relocs-01.s
#objcopy: --remove-relocations=.data.relocs.0\[12\]
#readelf: -r
+#notarget: "mips64*-*-openbsd*"

Relocation section '\.rela?\.data\.relocs\.03' at offset 0x[0-9a-f]+ contains 3 entries:
.*
diff --git a/binutils/testsuite/binutils-all/remove-relocs-05.d b/binutils/testsuite/binutils-all/remove-relocs-05.d
index e2166c93d9..a429182d25 100644
--- a/binutils/testsuite/binutils-all/remove-relocs-05.d
+++ b/binutils/testsuite/binutils-all/remove-relocs-05.d
@@ -2,6 +2,7 @@
#source: remove-relocs-01.s
#objcopy: --remove-section=.rela.data.relocs.01 --remove-section=.rel.data.relocs.01
#readelf: -r
+#notarget: "mips64*-*-openbsd*"

Relocation section '\.rela?\.data\.relocs\.02' at offset 0x[0-9a-f]+ contains 3 entries:
.*
diff --git a/binutils/testsuite/binutils-all/remove-relocs-06.d b/binutils/testsuite/binutils-all/remove-relocs-06.d
index 09fed58bb0..5214bc7297 100644
--- a/binutils/testsuite/binutils-all/remove-relocs-06.d
+++ b/binutils/testsuite/binutils-all/remove-relocs-06.d
@@ -2,6 +2,7 @@
#source: remove-relocs-01.s
#objcopy: --remove-relocations=.data.relocs.* --remove-relocations=!.data.relocs.02
#readelf: -r
+#notarget: "mips64*-*-openbsd*"

Relocation section '\.rela?\.data\.relocs\.02' at offset 0x[0-9a-f]+ contains 3 entries:
.*
diff --git a/binutils/testsuite/binutils-all/remove-relocs-07.d b/binutils/testsuite/binutils-all/remove-relocs-07.d
new file mode 100644
index 0000000000..c69df8d931
--- /dev/null
+++ b/binutils/testsuite/binutils-all/remove-relocs-07.d
@@ -0,0 +1,6 @@
+#PROG: objcopy
+#source: remove-relocs-07.s
+#objcopy: --remove-relocations=FOO
+#readelf: -r
+
+There are no relocations in this file\.
diff --git a/binutils/testsuite/binutils-all/remove-relocs-07.s b/binutils/testsuite/binutils-all/remove-relocs-07.s
new file mode 100644
index 0000000000..1e95375cd7
--- /dev/null
+++ b/binutils/testsuite/binutils-all/remove-relocs-07.s
@@ -0,0 +1,2 @@
+ .section "FOO","aw",%progbits
+ .dc.a x
diff --git a/binutils/testsuite/binutils-all/remove-relocs-08.d b/binutils/testsuite/binutils-all/remove-relocs-08.d
new file mode 100644
index 0000000000..f54462d645
--- /dev/null
+++ b/binutils/testsuite/binutils-all/remove-relocs-08.d
@@ -0,0 +1,6 @@
+#PROG: objcopy
+#source: remove-relocs-07.s
+#objcopy: --remove-section=.relFOO --remove-section=.relaFOO
+#readelf: -r
+
+There are no relocations in this file\.
--
Alan Modra
Australia Development Lab, IBM
Loading...