Bug 20765 - [2.27 Regression] gold internal error in fix_errata on aarch64-linux-gnu
Summary: [2.27 Regression] gold internal error in fix_errata on aarch64-linux-gnu
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gold (show other bugs)
Version: 2.27
: P2 normal
Target Milestone: ---
Assignee: Cary Coutant
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-03 12:35 UTC by Matthias Klose
Modified: 2017-11-30 23:19 UTC (History)
4 users (show)

See Also:
Host:
Target: aarch64-linux-gnu
Build:
Last reconfirmed:


Attachments
Update erratum_stub addresses on each relaxation pass (861 bytes, patch)
2017-11-30 10:47 UTC, Peter Smith
Details | Diff
Use a field that is not used in operator<() for erratum_stub validity (489 bytes, patch)
2017-11-30 10:50 UTC, Peter Smith
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Klose 2016-11-03 12:35:56 UTC
seen with binutils from the 2.27 branch:

$ sh link.sh 
ld.gold: internal error in fix_errata, at ../../gold/aarch64.cc:1960

This works with the BFD linker, or when dropping the --fix-cortex-a53-843419 option.

The test cases comes from the build of the gitit package on Ubuntu 16.10,
see https://launchpad.net/ubuntu/+source/gitit/0.12.1.1+dfsg-2build7

The package built successfully with binutils 2.26.1

test case at
http://people.canonical.com/~doko/tmp/tst.tar.xz
Comment 1 Han Shen 2016-12-04 23:04:34 UTC
The erratum insn address recorded in Erratum_stub is not updated after relaxation passes, which move input sections forward / backward (thus changes the erratum insn address). I've had a preliminary patch, but that leads to relocation overflow errors. I'll continue investigation.
Comment 2 Matthias Klose 2017-08-21 14:17:32 UTC
any news on that issue?
Comment 3 Peter Smith 2017-11-30 10:44:22 UTC
I have done some investigation and I think I have a fix for this PR. There are 2 separate problems with the --fix-cortex-a53-843419 shown up by the gitit build:
- Internal fault when the address of erratum stubs is modified by the addition of stubs earlier in the output section, or previous output section.
- Some erratum stubs are created but are not found, and hence skipped over, by fix_errata_and_relocate_erratum_stubs(). The root cause of this problem is that invalidate_erratum_stub() sets relobj_ to NULL. Unfortunately relobj is used in operator<() so the results of erratum_stubs_.lower_bound(), as used in find_erratum_stubs_for_input_section() are not reliable. 

The former can be exposed with a linker script or a very large test case such as gitit (requires 2 stub tables for the .text section). The latter is more likely to show up more when there are large numbers of erratum stubs, but in principle could happen in smaller examples so is more serious.

I'll attach a couple of patches to address each of the problems. The first runs at the start of scan_errata(), it goes through each of the errata that have already been created on previous patches and updates each stub with its new erratum_address and destination_address. The second adds a new field valid_ that is not used in operator<() so it can be safely changed by invalidate_erratum_stub().
Comment 4 Peter Smith 2017-11-30 10:47:50 UTC
Created attachment 10649 [details]
Update erratum_stub addresses on each relaxation pass

Attaching patch to fix the internal fault. The patch provides all the erratum_stubs with an up to date address on each relaxation pass.
Comment 5 Peter Smith 2017-11-30 10:50:43 UTC
Created attachment 10650 [details]
Use a field that is not used in operator<() for erratum_stub validity

Attached patch that adds a new field to class Erratum_stub, so that invalidate_erratum_stub can be used without affecting operator<(), which is used by erratum_stubs_.lower_bound().
Comment 6 Sourceware Commits 2017-11-30 23:09:06 UTC
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e0feb133429709603eeeb382c1ebb6edd0a886aa

commit e0feb133429709603eeeb382c1ebb6edd0a886aa
Author: Cary Coutant <ccoutant@gmail.com>
Date:   Thu Nov 30 13:44:28 2017 -0800

    Fix problem where erratum stubs are not always applied.
    
    I checked over the results of applying --fix-cortex-a53-843419 to
    a very large program (gitit) with two stub tables and thousands
    of erratum fixes. I noticed that all the erratum_stubs were being
    created but about 1/3 of them were being skipped over by
    fix_errata_and_relocate_erratum_stubs(). By skipped over I mean
    no branch relocation or adrp -> adr transformation was applied to
    the erratum address, leaving the erratum_stub unreachable, and
    with a branch with a 0 immediate.
    
    The root cause of the skipped over erratum_stubs is
    Erratum_stub::invalidate_erratum_stub() that is used to set
    relobj_ to NULL when an erratum_stub has been processed.
    Unfortunately relobj_ is used in operator<() so altering relobj
    makes the results from erratum_stubs_.lower_bound() as used in
    find_erratum_stubs_for_input_section() unreliable.
    
    2017-11-30  Peter Smith  <peter.smith@linaro.org>
    	    Cary Coutant  <ccoutant@gmail.com>
    
    gold/
    	PR gold/20765
    	* aarch64.cc (Erratum_stub::invalidate_erratum_stub): Use erratum_insn_
    	instead of relobj_ to invalidate the stub.
    	(Erratum_stub::is_invalidated_erratum_stub): Likewise.
Comment 7 Sourceware Commits 2017-11-30 23:09:11 UTC
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=fd6798fa2d6f0374d45449c4212869da93623b1e

commit fd6798fa2d6f0374d45449c4212869da93623b1e
Author: Peter Smith <peter.smith@linaro.org>
Date:   Thu Nov 30 15:07:26 2017 -0800

    Fix internal error in fix_errata on aarch64.
    
    The addresses of erratum stubs can be changed by relaxation passes, and
    need to be updated.
    
    gold/
    	PR gold/20765
    	* aarch64.cc (Aarch64_relobj::update_erratum_address): New method.
    	(AArch64_relobj::scan_errata): Update addresses in stub table after
    	relaxation pass.
Comment 8 Sourceware Commits 2017-11-30 23:17:42 UTC
The binutils-2_29-branch branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=c2fd86f2bdfa2f565cf6da6530f1bfb65824e2d3

commit c2fd86f2bdfa2f565cf6da6530f1bfb65824e2d3
Author: Cary Coutant <ccoutant@gmail.com>
Date:   Thu Nov 30 13:44:28 2017 -0800

    Fix problem where erratum stubs are not always applied.
    
    I checked over the results of applying --fix-cortex-a53-843419 to
    a very large program (gitit) with two stub tables and thousands
    of erratum fixes. I noticed that all the erratum_stubs were being
    created but about 1/3 of them were being skipped over by
    fix_errata_and_relocate_erratum_stubs(). By skipped over I mean
    no branch relocation or adrp -> adr transformation was applied to
    the erratum address, leaving the erratum_stub unreachable, and
    with a branch with a 0 immediate.
    
    The root cause of the skipped over erratum_stubs is
    Erratum_stub::invalidate_erratum_stub() that is used to set
    relobj_ to NULL when an erratum_stub has been processed.
    Unfortunately relobj_ is used in operator<() so altering relobj
    makes the results from erratum_stubs_.lower_bound() as used in
    find_erratum_stubs_for_input_section() unreliable.
    
    2017-11-30  Peter Smith  <peter.smith@linaro.org>
    	    Cary Coutant  <ccoutant@gmail.com>
    
    gold/
    	PR gold/20765
    	* aarch64.cc (Erratum_stub::invalidate_erratum_stub): Use erratum_insn_
    	instead of relobj_ to invalidate the stub.
    	(Erratum_stub::is_invalidated_erratum_stub): Likewise.
Comment 9 Sourceware Commits 2017-11-30 23:17:47 UTC
The binutils-2_29-branch branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ce8bf2af57bc1d188f52e815805be202c6877d19

commit ce8bf2af57bc1d188f52e815805be202c6877d19
Author: Peter Smith <peter.smith@linaro.org>
Date:   Thu Nov 30 15:07:26 2017 -0800

    Fix internal error in fix_errata on aarch64.
    
    The addresses of erratum stubs can be changed by relaxation passes, and
    need to be updated.
    
    gold/
    	PR gold/20765
    	* aarch64.cc (Aarch64_relobj::update_erratum_address): New method.
    	(AArch64_relobj::scan_errata): Update addresses in stub table after
    	relaxation pass.
Comment 10 Cary Coutant 2017-11-30 23:19:00 UTC
Fixed on trunk and 2.29.