Re: [PATCH] _bfd_mips_elf_final_link: notify user about wrong .reginfo size

[Vlad Ivanov <vlad@ivanov.email>]

Hello Maciej,

thank you for your explanations regarding patch submitting.

>  Second, if this assertion is indeed user-triggerable, then please
> describe how to trigger it. We'll need this to create a test suite case
> (if feasible), so that we can verify in the future that the error
> situation continues being correctly handled.

I ran into this assertion while transitioning between binutils versions.
Target used was mipsel-sde-elf. It's triggered when the user-provided linker 
script lacks .reginfo section. In order to fix it, the following line was 
added to the linker script:

    .reginfo                 0 : { KEEP(*(.reginfo)) }

Since the assertion itself wasn't very self-descriptive, I decided to 
submit a patch for it. I'm not sure if this change needs a test.

>  This will also help double-checking that the error is indeed issued from
> the right place -- here it is the output section that is being handled, so
> offhand it looks to me like the size should have been set to the correct
> value as the output section was created by BFD. If the incorrect size has
> been instead copied from an input section, then I think the size mismatch
> should be reported in input section processing rather than here.

As far as I understand, the issue here is not the input section being
wrong size (although this may happen too) but rather a total lack
of such input section, which leads to a creation of zero-sized output
section.

I will resubmit the patch.

Regards,

Vlad

09.01.2018, 04:30, "Maciej W. Rozycki" <macro@mips.com>:
> Hi Vlad,
>
>  Thank you for your contribution. I will be happy to accept your change
> for binutils, however your submission has a few problems, which will have
> to be corrected first.
>
>  First of all, please explain why this change is needed -- is there a way
> to trigger this assertion with user input? This explanation will become
> the patch description once the change has been committed, and is subject
> to prior review just as the code update itself. See `git log' for
> examples of how such a description might look like.
>
>  When writing the description please follow text formatting rules as per
> the GNU Coding Standard, in particular please keep your text within 74
> columns (I recommend 72 columns due to the indentation applied by `git
> show', etc.) and place two spaces between a full stop and any following
> sentence. For a complete explanation of the rules see:
>
> <https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Formatting_Your_Code>
>
> and:
>
> <https://www.gnu.org/prep/standards/standards.html#Formatting>.
>
> Please note that sentences need to start with a capital letter; this also
> applies to the patch heading in the subject.
>
>  Second, if this assertion is indeed user-triggerable, then please
> describe how to trigger it. We'll need this to create a test suite case
> (if feasible), so that we can verify in the future that the error
> situation continues being correctly handled.
>
>  This will also help double-checking that the error is indeed issued from
> the right place -- here it is the output section that is being handled, so
> offhand it looks to me like the size should have been set to the correct
> value as the output section was created by BFD. If the incorrect size has
> been instead copied from an input section, then I think the size mismatch
> should be reported in input section processing rather than here.
>
>  Third, you need to include a ChangeLog entry at the end of your patch
> description. See bfd/ChangeLog for existing examples and:
>
> <https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs>
>
> for a full explanation.
>
>  Fourth, there are a couple of formatting issues with the change itself;
> see below for details.
>
>>  diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
>>  index 6a4d3e1a6a..e27327c929 100644
>>  --- a/bfd/elfxx-mips.c
>>  +++ b/bfd/elfxx-mips.c
>>  @@ -14431,7 +14431,13 @@ _bfd_mips_elf_final_link (bfd *abfd, struct bfd_link_info *info)
>>               }
>>
>>             /* Size has been set in _bfd_mips_elf_always_size_sections. */
>>  - BFD_ASSERT(o->size == sizeof (Elf32_External_RegInfo));
>>  + if (o->size != sizeof (Elf32_External_RegInfo)) {
>
>  A conditional block's opening brace has to be on the following line,
> indented by 2 spaces. The body of the block has to be indented by further
> 2 spaces (use a tab for every block of 8 spaces).
>
>>  + _bfd_error_handler
>>  + (_("%B: .reginfo section size should be %d bytes, actual size is %d"),
>
>  Please wrap the string to stay within 74 columns, as per formatting rules
> referred above. See existing examples of `_bfd_error_handler' invocations
> throughout this file.
>
>>  + abfd, sizeof (Elf32_External_RegInfo), o->size);
>>  +
>>  + return FALSE;
>>  + }
>
>  A block's closing brace has to be placed on the same column as the
> matching opening brace.
>
>  Please resubmit with these issues corrected, or feel free to ask me if
> you have any questions.
>
>   Maciej