Re: Fix for PR-23611

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

I'm not a maintainer so can't approve your patch, but here's my
feedback as an interested party :)

Thanks,
Andrew


* kamlesh kumar <kamleshbhalui@gmail.com> [2018-09-07 14:25:23 +0530]:

> Hi Everyone,
> 
> Attached patch(23611.patch) has a fix for the subjected issue and here
> is the ChangeLog
> 
> 2018-09-07 Kamlesh Kumar kamleshbhalui@gmail.com

Two whitespace between date/name/email, and the email is in '<'...'>'.

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

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

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

> +	{
> +           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.

> +        }
> +      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.

> +          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*-*-*