Bug 20364 - Data-based padding causes current address evaluation to be non-constant on aarch64
Summary: Data-based padding causes current address evaluation to be non-constant on aa...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Nick Clifton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-13 08:30 UTC by Paul Kocialkowski
Modified: 2016-10-19 08:33 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2016-08-02 00:00:00


Attachments
Proposed patch (592 bytes, patch)
2016-08-02 15:47 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Kocialkowski 2016-07-13 08:30:00 UTC
It appears that starting with binutils 2.26, evaluating the current address (using the dot symbol) after a .align directive using data-based padding (e.g. .align 6, 0) makes it non-constant, resulting in errors such as: "Error: non-constant expression in ".if" statement" when evaluating "." inside an .if statement. This is on aarch64.

The issue appeared when build the ARM Trusted Firmware: https://github.com/ARM-software/arm-trusted-firmware

The related assembly is a combination of the vector_entry/vector_base (that does the alignment) and check_vector_size (that uses the dot symbol) macros, defined at: https://github.com/ARM-software/arm-trusted-firmware/blob/master/include/common/asm_macros.S#L69 and used at https://github.com/ARM-software/arm-trusted-firmware/blob/master/bl31/aarch64/runtime_exceptions.S#L171

The assembly is correct and is apparently not the problem. However, switching the ".align x, 0" directives to ".align x" solves the issue, but upstream is not going to make that change.

The issue was discussed in depth at https://www.mail-archive.com/linaro-toolchain@lists.linaro.org/msg05685.html and an issue was opened on the ARM Trusted Firmware tracker at https://github.com/ARM-software/tf-issues/issues/401

A test case was provided at https://www.mail-archive.com/linaro-toolchain@lists.linaro.org/msg05688/tmp.s

The issue was bisected down to (bad) commit c1baaddf8861aea666b84baeb4746caff51a579d

commit c1baaddf8861aea666b84baeb4746caff51a579d
Author: Renlin Li <renlin...@arm.com>
Date:   Thu Apr 2 14:59:45 2015 +0100

    [AArch64] Emit DATA_MAP in order within text section

    2015-03-27  Renlin Li  <renlin...@arm.com>

    gas/
      * config/tc-aarch64.c (mapping_state): Emit MAP_DATA within text
section in order.
      (mapping_state_2): Don't emit MAP_DATA here.
      (s_aarch64_inst): Align frag during state transition.
      (md_assemble): Likewise.
Comment 1 Nick Clifton 2016-08-02 15:47:01 UTC
Created attachment 9419 [details]
Proposed patch

Hi Paul,

  This is a subtle one.  What is happening is that ".align <num>" in a code 
section (like .text or .vectors) will use the default NOP value to fill in
the space.  But ".align <num>, <fill>" will use the provided fill value.  This
matters because the AArch64 backend knows that NOP is an instruction, but it
does not know about <fill> and it assumes that it is a *data* value, not an
*instruction*.  So it changes the mapping state to MAP_DATA, creates a
mapping symbol ($d) indicate the change of state, and creates a new fragment
to contain the data.

  All of this means that when the macro parser tries to evaluate the .if
statement it finds that dot is one fragment and the symbol is in another 
fragment.  Since fragments can grow or shrink the distance between them 
cannot be computed reliably (at the time that the .if expressions is evaluated)
and so the assembler issues its error message.

  I have uploaded a small patch which I think will fix the problem.  It tells
the AArch64 backend not to assume that alignment directives in code sections
contain data.  Instead it should assume that they contain instructions, and
so not change the mapping state.  This causes a subtle problem with regard to
literal pools, but the patch takes care of that.

  Please give the patch a try and let me know how you get on.

Cheers
  Nick
Comment 2 cvs-commit@gcc.gnu.org 2016-08-05 09:39:07 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 7ea12e5c3ad54da440c08f32da09534e63e515ca
Author: Nick Clifton <nickc@redhat.com>
Date:   Fri Aug 5 10:37:57 2016 +0100

    Fix the generation of alignment frags in code sections for AArch64.
    
    	PR gas/20364
    	* config/tc-aarch64.c (s_ltorg): Change the mapping state after
    	aligning the frag.
    	(aarch64_init): Treat rs_align frags in code sections as
    	containing code, not data.
    	* testsuite/gas/aarch64/pr20364.s: New test.
    	* testsuite/gas/aarch64/pr20364.d: New test driver.
Comment 3 Nick Clifton 2016-08-05 09:42:23 UTC
I have gone ahead and applied the patch.
Comment 4 Paul Kocialkowski 2016-08-07 09:33:28 UTC
Thanks for the explanation and the fix! It does solve the problem we had with the ARM Trusted Firmware codebase. I'm glad it was merged, too.
Comment 5 cvs-commit@gcc.gnu.org 2016-10-19 08:33:48 UTC
The linaro_binutils-2_27-branch branch has been updated by Christophe Lyon <clyon@sourceware.org>:

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

commit eb65c97589b3c6a5c2954cea45c068b30c751b51
Author: Nick Clifton <nickc@redhat.com>
Date:   Fri Aug 5 10:37:57 2016 +0100

    Fix the generation of alignment frags in code sections for AArch64.
    
    	PR gas/20364
    	* config/tc-aarch64.c (s_ltorg): Change the mapping state after
    	aligning the frag.
    	(aarch64_init): Treat rs_align frags in code sections as
    	containing code, not data.
    	* testsuite/gas/aarch64/pr20364.s: New test.
    	* testsuite/gas/aarch64/pr20364.d: New test driver.