This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] _bfd_mips_elf_final_link: notify user about wrong .reginfo size
- From: Vlad Ivanov <vlad@ivanov.email>
- To: Maciej W. Rozycki <macro at mips dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Tue, 09 Jan 2018 09:46:12 +0300
- Subject: Re: [PATCH] _bfd_mips_elf_final_link: notify user about wrong .reginfo size
- Authentication-results: sourceware.org; auth=none
- Authentication-results: mxback15j.mail.yandex.net; dkim=pass header.i=@ivanov.email
- References: <20171223164610.11596-1-vlad@ivanov.email> <alpine.DEB.2.00.1801070020500.20647@tp.orcam.me.uk>
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