Bug 489338

Summary: arm64: Instruction fcvtas should round 322.5 to 323, but result is 322.
Product: [Developer tools] valgrind Reporter: Bernhard Übelacker <bernhardu>
Component: vexAssignee: Julian Seward <jseward>
Status: RESOLVED FIXED    
Severity: normal CC: pjfloyd
Priority: NOR    
Version First Reported In: 3.20.0   
Target Milestone: ---   
Platform: Debian unstable   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: Minimal reproducer: fp-valgrind-test.c
0001-arm64-Fix-fcvtas-instruction.patch

Description Bernhard Übelacker 2024-06-27 21:16:29 UTC 
Created attachment 171101 [details]
Minimal reproducer: fp-valgrind-test.c

Hello, I was investigating a debian bug, where the continuous integration failed
because it receives a different value when running with valgrind. [1071656]
I could track it down to a fcvtas instruction.

  => 0xaaaaaaaa4948 <ConvertToRational+104>:     fcvtas  w5, d8
(With $d8=322.5)

With plain GDB:
  (gdb) print $w5
  $2 = 323

With valgrind+GDB:
  (gdb) print $w5
  $2 = 322

Attached is a minimal reproducer.

[1071656] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1071656
Comment 1 Bernhard Übelacker 2024-06-27 21:24:20 UTC 
Created attachment 171102 [details]
0001-arm64-Fix-fcvtas-instruction.patch

I tried to look into it and I think the issue is the `fcvtas` gets stored in the intermediate representation with Irrm_NEAREST.
When this gets translated back to the native instruction i guess it results in a `fcvtns`.

Attached patch tries to preserve this difference by using Irrm_NEAREST_TIE_AWAY_0,
at least the reproducer shows with it no longer a difference.
Comment 2 Paul Floyd 2024-06-28 08:00:27 UTC 
Looks good. One problem with the regtests is that a lot of the numeric tests just use random number generators. That means that they don't cover rounding with ties very well.

This also needs doing for the vector version, as noted here

         case 3: ch = 'a'; irrm = Irrm_NEAREST; break; /* kludge? */

I'll see if I can also knock up a test that covers all 8 combinations of float/double and signed/unsigned scalar.
Comment 3 Paul Floyd 2024-07-01 05:55:14 UTC 
Pushed with a small change to use True and False rather than 1 and 0.

I'll add a couple of tests as well.

commit de4c79ffbcd2d5e89495cee8feadf77d5f3a6ef2 (HEAD -> master, origin/master, origin/HEAD)
Author: Bernhard <C3><9C>belacker <bernhardu@mailbox.org>
Date:   Thu Jun 27 22:51:09 2024 +0200

    arm64: Fix fcvtas instruction.