Fix for PR-23611

[kamlesh kumar]

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  <kamleshbhalui@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
<andrew.burgess@embecosm.com> wrote:
> 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*-*-*
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 23611.patch
Type: text/x-patch
Size: 4048 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20180907/a8bdd47f/attachment.bin>