Skip to content

Commit

Permalink
dev-util/rgbds: fix UB revealed by test failure
Browse files Browse the repository at this point in the history
With some arm CPUs and with clang's ubsan an asm test fails converting
infinity and NaN to integers which is undefined behavior.

Closes: https://bugs.gentoo.org/928268
Upstream-Issue: gbdev/rgbds#1387
Upstream-PR: gbdev/rgbds#1388
Upstream-Commit: gbdev/rgbds@9ab3446
Signed-off-by: orbea <[email protected]>
Closes: gentoo#36021
Signed-off-by: Joonas Niilola <[email protected]>
  • Loading branch information
orbea authored and juippis committed May 9, 2024
1 parent 9fa0d9e commit 4789378
Show file tree
Hide file tree
Showing 2 changed files with 202 additions and 0 deletions.
147 changes: 147 additions & 0 deletions dev-util/rgbds/files/rgbds-0.7.0-fix-nan-tests.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
https://bugs.gentoo.org/928268
https://github.com/gbdev/rgbds/issues/1387
https://github.com/gbdev/rgbds/pull/1388
https://github.com/gbdev/rgbds/commit/9ab3446d1a3d84d6b34062b8287be9169fbe663b

From 1afbaa3cf2b667c33ae02e899ad7a833e3b71292 Mon Sep 17 00:00:00 2001
From: Sylvie <[email protected]>
Date: Sun, 31 Mar 2024 12:53:20 -0400
Subject: [PATCH] Fix two bugs with RGBASM fixed-point math (#1388)

- Fixed-point formulas are implemented using IEEE-754 floating-point
internally, which could give infinity or NaN values whose conversion
to fixed-point integer was platform-dependent.
- Formatting fixed-point $8000_0000 (INT32_MIN, -2147483648) was
not putting the negative sign in front.
---
src/asm/fixpoint.cpp | 10 +++++++++-
src/asm/format.cpp | 22 +++++++++++++---------
test/asm/format-extremes.asm | 8 ++++++++
test/asm/format-extremes.out | 4 ++++
test/asm/math.asm | 8 ++++++--
5 files changed, 40 insertions(+), 12 deletions(-)
create mode 100644 test/asm/format-extremes.asm
create mode 100644 test/asm/format-extremes.out

diff --git a/src/asm/fixpoint.cpp b/src/asm/fixpoint.cpp
index 97a091af..9334bbba 100644
--- a/src/asm/fixpoint.cpp
+++ b/src/asm/fixpoint.cpp
@@ -15,7 +15,6 @@
#endif

#define fix2double(i, q) ((double)((i) / pow(2.0, q)))
-#define double2fix(d, q) ((int32_t)round((d) * pow(2.0, q)))

// 2*pi radians == 1 turn
#define turn2rad(f) ((f) * (M_PI * 2))
@@ -33,6 +32,15 @@ double fix_PrecisionFactor(void)
return pow(2.0, fixPrecision);
}

+static int32_t double2fix(double d, int32_t q)
+{
+ if (isnan(d))
+ return 0;
+ if (isinf(d))
+ return d < 0 ? INT32_MIN : INT32_MAX;
+ return (int32_t)round(d * pow(2.0, q));
+}
+
int32_t fix_Sin(int32_t i, int32_t q)
{
return double2fix(sin(turn2rad(fix2double(i, q))), q);
diff --git a/src/asm/format.cpp b/src/asm/format.cpp
index 553e5c77..2b8b8a8a 100644
--- a/src/asm/format.cpp
+++ b/src/asm/format.cpp
@@ -180,11 +180,10 @@ void fmt_PrintNumber(char *buf, size_t bufLen, struct FormatSpec const *fmt, uin
char sign = fmt->sign; // 0 or ' ' or '+'

if (fmt->type == 'd' || fmt->type == 'f') {
- int32_t v = value;
-
- if (v < 0 && v != INT32_MIN) {
+ if (int32_t v = value; v < 0) {
sign = '-';
- value = -v;
+ if (v != INT32_MIN)
+ value = -v;
}
}

@@ -229,15 +228,20 @@ void fmt_PrintNumber(char *buf, size_t bufLen, struct FormatSpec const *fmt, uin
fracWidth = 255;
}

- snprintf(valueBuf, sizeof(valueBuf), "%.*f", (int)fracWidth,
- value / fix_PrecisionFactor());
+ double fval = fabs(value / fix_PrecisionFactor());
+ snprintf(valueBuf, sizeof(valueBuf), "%.*f", (int)fracWidth, fval);
+ } else if (fmt->type == 'd') {
+ // Decimal numbers may be formatted with a '-' sign by `snprintf`, so `abs` prevents that,
+ // with a special case for `INT32_MIN` since `labs(INT32_MIN)` is UB. The sign will be
+ // printed later from `signChar`.
+ uint32_t uval = value != (uint32_t)INT32_MIN ? labs((int32_t)value) : value;
+ snprintf(valueBuf, sizeof(valueBuf), "%" PRIu32, uval);
} else {
- char const *spec = fmt->type == 'd' ? "%" PRId32
- : fmt->type == 'u' ? "%" PRIu32
+ char const *spec = fmt->type == 'u' ? "%" PRIu32
: fmt->type == 'X' ? "%" PRIX32
: fmt->type == 'x' ? "%" PRIx32
: fmt->type == 'o' ? "%" PRIo32
- : "%" PRId32;
+ : "%" PRIu32;

snprintf(valueBuf, sizeof(valueBuf), spec, value);
}
diff --git a/test/asm/format-extremes.asm b/test/asm/format-extremes.asm
new file mode 100644
index 00000000..19ddb677
--- /dev/null
+++ b/test/asm/format-extremes.asm
@@ -0,0 +1,8 @@
+MACRO test
+ def v = \1
+ println "{#09x:v} = {#012o:v} = {#033b:v} = {u:v}U = {+d:v} = {+.16f:v}"
+ENDM
+ test $7fff_ffff ; INT32_MAX
+ test $8000_0000 ; INT32_MIN
+ test $0000_0000 ; UINT32_MIN
+ test $ffff_ffff ; UINT32_MAX
diff --git a/test/asm/format-extremes.out b/test/asm/format-extremes.out
new file mode 100644
index 00000000..9e19b2f4
--- /dev/null
+++ b/test/asm/format-extremes.out
@@ -0,0 +1,4 @@
+$7fffffff = &17777777777 = %01111111111111111111111111111111 = 2147483647U = +2147483647 = +32767.9999847412109375
+$80000000 = &20000000000 = %10000000000000000000000000000000 = 2147483648U = -2147483648 = -32768.0000000000000000
+$00000000 = &00000000000 = %00000000000000000000000000000000 = 0U = +0 = +0.0000000000000000
+$ffffffff = &37777777777 = %11111111111111111111111111111111 = 4294967295U = -1 = -0.0000152587890625
diff --git a/test/asm/math.asm b/test/asm/math.asm
index b189fca8..9f87a11b 100644
--- a/test/asm/math.asm
+++ b/test/asm/math.asm
@@ -19,14 +19,18 @@ ENDM

assert DIV(5.0, 2.0) == 2.5
assert DIV(-5.0, 2.0) == -2.5
- assert DIV(-5.0, 0.0) == $8000_0000
+ assert DIV(5.0, 0.0) == $7fff_ffff ; +inf => INT32_MAX
+ assert DIV(-5.0, 0.0) == $8000_0000 ; -inf => INT32_MIN
+ assert DIV(0.0, 0.0) == $0000_0000 ; nan => 0

assert MUL(10.0, 0.5) == 5.0
assert MUL(10.0, 0.0) == 0.0

assert FMOD(5.0, 2.0) == 1.0
assert FMOD(-5.0, 2.0) == -1.0
- assert FMOD(-5.0, 0.0) == $8000_0000
+ assert FMOD(5.0, 0.0) == 0 ; nan
+ assert FMOD(-5.0, 0.0) == 0 ; nan
+ assert FMOD(0.0, 0.0) == 0 ; nan

assert POW(10.0, 2.0) == 100.0
assert POW(100.0, 0.5) == 10.0
55 changes: 55 additions & 0 deletions dev-util/rgbds/rgbds-0.7.0-r1.ebuild
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Copyright 2022-2024 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2

EAPI=8

inherit flag-o-matic toolchain-funcs

DESCRIPTION="Rednex Game Boy Development System"
HOMEPAGE="https://rgbds.gbdev.io/"
if [[ "${PV}" == *9999 ]] ; then
inherit git-r3
EGIT_REPO_URI="https://github.com/gbdev/${PN}.git"
else
SRC_URI="https://github.com/gbdev/${PN}/archive/v${PV}/${P}.tar.gz"
KEYWORDS="~amd64 ~arm ~arm64 ~ppc ~ppc64 ~x86"
fi

LICENSE="MIT"
SLOT="0"

DEPEND="media-libs/libpng"
RDEPEND="${DEPEND}"
BDEPEND="
sys-devel/bison
virtual/pkgconfig
"

PATCHES=(
# https://bugs.gentoo.org/928268
"${FILESDIR}"/${P}-fix-nan-tests.patch
)

src_compile() {
append-flags -DNDEBUG

emake Q= \
CC="$(tc-getCC)" \
CXX="$(tc-getCXX)" \
PKG_CONFIG="$(tc-getPKG_CONFIG)"
}

src_test() {
local dir
for dir in asm link fix gfx; do
pushd "test/${dir}" >/dev/null || die
einfo "Running ${dir} tests."
./test.sh || die
popd >/dev/null || die
done
}

src_install() {
emake DESTDIR="${D}" PREFIX="${EPREFIX}"/usr Q= STRIP= install
dodoc README.rst
}

0 comments on commit 4789378

Please sign in to comment.