[GASPP] Conditionally work around an armasm64 bug in the scale operand to fcvtzs/scvtf

Message ID 1508533488-2940-1-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö Oct. 20, 2017, 9:04 p.m.
The operand shouldn't be stored as is, but stored as 64-scale, in
the opcode, but armasm64 currently misses to do this.

This bug will be fixed in a future release, and a fix for the bug
will break the workaround. Therefore, only do the workaround as
long as a environment variable is set.
---
Suggestions on what to name the environment variable are welcome;
it's hard to guess what to name it as I don't know what version
the fix will appear in.
---
 gas-preprocessor.pl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Diego Biurrun Oct. 21, 2017, 8:04 a.m. | #1
On Sat, Oct 21, 2017 at 12:04:48AM +0300, Martin Storsjo wrote:
> The operand shouldn't be stored as is, but stored as 64-scale, in
> the opcode, but armasm64 currently misses to do this.
> 
> This bug will be fixed in a future release, and a fix for the bug
> will break the workaround. Therefore, only do the workaround as
> long as a environment variable is set.
> ---
> Suggestions on what to name the environment variable are welcome;
> it's hard to guess what to name it as I don't know what version
> the fix will appear in.

When do you expect this future release to appear? It may be sensible
to just wait for the fix to appear and live with a bit of pain in the
meantime...

Diego
Diego Biurrun Oct. 21, 2017, 8:04 a.m. | #2
On Sat, Oct 21, 2017 at 12:04:48AM +0300, Martin Storsjo wrote:
> The operand shouldn't be stored as is, but stored as 64-scale, in
> the opcode, but armasm64 currently misses to do this.
> 
> This bug will be fixed in a future release, and a fix for the bug
> will break the workaround. Therefore, only do the workaround as
> long as a environment variable is set.
> ---
> Suggestions on what to name the environment variable are welcome;
> it's hard to guess what to name it as I don't know what version
> the fix will appear in.

GASPP_ARMASM_SCALE_WORKAROUND

Diego
Martin Storsjö Oct. 21, 2017, 8:34 a.m. | #3
On Sat, 21 Oct 2017, Diego Biurrun wrote:

> On Sat, Oct 21, 2017 at 12:04:48AM +0300, Martin Storsjo wrote:
>> The operand shouldn't be stored as is, but stored as 64-scale, in
>> the opcode, but armasm64 currently misses to do this.
>> 
>> This bug will be fixed in a future release, and a fix for the bug
>> will break the workaround. Therefore, only do the workaround as
>> long as a environment variable is set.
>> ---
>> Suggestions on what to name the environment variable are welcome;
>> it's hard to guess what to name it as I don't know what version
>> the fix will appear in.
>
> When do you expect this future release to appear? It may be sensible
> to just wait for the fix to appear and live with a bit of pain in the
> meantime...

I didn't get any estimate on that, but lately they've released fixes every 
couple months, so it could possibly appear before the OS itself becomes 
public.

// Martin
Martin Storsjö Oct. 26, 2017, 9:51 a.m. | #4
On Sat, 21 Oct 2017, Martin Storsjo wrote:

> The operand shouldn't be stored as is, but stored as 64-scale, in
> the opcode, but armasm64 currently misses to do this.
>
> This bug will be fixed in a future release, and a fix for the bug
> will break the workaround. Therefore, only do the workaround as
> long as a environment variable is set.
> ---
> Suggestions on what to name the environment variable are welcome;
> it's hard to guess what to name it as I don't know what version
> the fix will appear in.
> ---
> gas-preprocessor.pl | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
> index 7644682..58d36cc 100755
> --- a/gas-preprocessor.pl
> +++ b/gas-preprocessor.pl
> @@ -1037,6 +1037,18 @@ sub handle_serialized_line {
>                 $line =~ s/$instr$suffix/${instr}u$suffix/;
>               }
>             }
> +
> +            if ($ENV{GASPP_FIX_EARLY_ARMASM64}) {
> +                # Instructions like fcvtzs and scvtf store the scale value
> +                # inverted in the opcode (stored as 64 - scale), but armasm64
> +                # in early versions stores it as-is. Thus convert from
> +                # "fcvtzs w0, s0, #8" into "fcvtzs w0, s0, #56".
> +                if ($line =~ /(?:fcvtzs|scvtf)\s+(\w+)\s*,\s*(\w+)\s*,\s*#(\d+)/) {
> +                    my $scale = $3;
> +                    my $inverted_scale = 64 - $3;
> +                    $line =~ s/#$scale/#$inverted_scale/;
> +                }
> +            }
>         }
>         # armasm is unable to parse &0x - add spacing
>         $line =~ s/&0x/& 0x/g;
> -- 
> 2.7.4

OK'd by Janne on irc, with the name suggestion 
GASPP_ARMASM64_INVERT_SCALE - will push with that name.

// Martin

Patch

diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
index 7644682..58d36cc 100755
--- a/gas-preprocessor.pl
+++ b/gas-preprocessor.pl
@@ -1037,6 +1037,18 @@  sub handle_serialized_line {
                 $line =~ s/$instr$suffix/${instr}u$suffix/;
               }
             }
+
+            if ($ENV{GASPP_FIX_EARLY_ARMASM64}) {
+                # Instructions like fcvtzs and scvtf store the scale value
+                # inverted in the opcode (stored as 64 - scale), but armasm64
+                # in early versions stores it as-is. Thus convert from
+                # "fcvtzs w0, s0, #8" into "fcvtzs w0, s0, #56".
+                if ($line =~ /(?:fcvtzs|scvtf)\s+(\w+)\s*,\s*(\w+)\s*,\s*#(\d+)/) {
+                    my $scale = $3;
+                    my $inverted_scale = 64 - $3;
+                    $line =~ s/#$scale/#$inverted_scale/;
+                }
+            }
         }
         # armasm is unable to parse &0x - add spacing
         $line =~ s/&0x/& 0x/g;