Change in coreboot[master]: libpayload: cbgfx: Fix add_fractions() overflow reduction

[Julius Werner (Code Review)]

Hello Yu-Ping Wu,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/42845

to review the following change.

Change subject: libpayload: cbgfx: Fix add_fractions() overflow reduction
......................................................................

libpayload: cbgfx: Fix add_fractions() overflow reduction

log2(1) is 0 and log2(0) is -1. If we have the int64_t 0xffffffff then
log2(0xffffffff >> 31) = log2(0x1) = 0, so the current reduction code
would not shift. That's a bad idea, though, since 0xffffffff when
interpreted as an int32_t would become a negative number.

We need to always shift one more than the current code does to get a
safe reduction. This also means we can get rid of another compare/branch
since -1 is the smallest result log2() can return, so the shift can no
longer go negative now.

Signed-off-by: Julius Werner 
Change-Id: Ib1eb6364c35c26924804261c02171139cdbd1034
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 3 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/42845/1

diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c
index 81d2bb9..97334af 100644
--- a/payloads/libpayload/drivers/video/graphics.c
+++ b/payloads/libpayload/drivers/video/graphics.c
@@ -85,13 +85,9 @@
    n = (int64_t)f1->n * f2->d + (int64_t)f2->n * f1->d;
    d = (int64_t)f1->d * f2->d;
    /* Simplest way to reduce the fraction until fitting in int32_t */
-	shift = log2(MAX(ABS(n), ABS(d)) >> 31);
-	if (shift > 0) {
-		n >>= shift;
-		d >>= shift;
-	}
-	out->n = n;
-	out->d = d;
+	shift = log2(MAX(ABS(n), ABS(d)) >> 31) + 1;
+	out->n = n >> shift;
+	out->d = d >> shift;
 }

static void add_scales(struct scale *out,

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42845 )

Change subject: libpayload: cbgfx: Fix add_fractions() overflow reduction
......................................................................

Patch Set 1: Code-Review+1

Thanks for fixing it.

I don't have CR+2 permission, so add Joel as another reviewer.

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42845 )

Change subject: libpayload: cbgfx: Fix add_fractions() overflow reduction
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42845 )

Change subject: libpayload: cbgfx: Fix add_fractions() overflow reduction
......................................................................

Patch Set 1: Code-Review+1

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42845 )

Change subject: libpayload: cbgfx: Fix add_fractions() overflow reduction
......................................................................

libpayload: cbgfx: Fix add_fractions() overflow reduction

log2(1) is 0 and log2(0) is -1. If we have the int64_t 0xffffffff then
log2(0xffffffff >> 31) = log2(0x1) = 0, so the current reduction code
would not shift. That's a bad idea, though, since 0xffffffff when
interpreted as an int32_t would become a negative number.

We need to always shift one more than the current code does to get a
safe reduction. This also means we can get rid of another compare/branch
since -1 is the smallest result log2() can return, so the shift can no
longer go negative now.

Signed-off-by: Julius Werner 
Change-Id: Ib1eb6364c35c26924804261c02171139cdbd1034
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42845
Tested-by: build bot (Jenkins) 
Reviewed-by: Yu-Ping Wu 
Reviewed-by: Joel Kitching 
Reviewed-by: Paul Menzel 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 3 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Joel Kitching: Looks good to me, approved
  Yu-Ping Wu: Looks good to me, but someone else must approve

diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c
index 13eac28..fa72c9b 100644
--- a/payloads/libpayload/drivers/video/graphics.c
+++ b/payloads/libpayload/drivers/video/graphics.c
@@ -85,13 +85,9 @@
    n = (int64_t)f1->n * f2->d + (int64_t)f2->n * f1->d;
    d = (int64_t)f1->d * f2->d;
    /* Simplest way to reduce the fraction until fitting in int32_t */
-	shift = log2(MAX(ABS(n), ABS(d)) >> 31);
-	if (shift > 0) {
-		n >>= shift;
-		d >>= shift;
-	}
-	out->n = n;
-	out->d = d;
+	shift = log2(MAX(ABS(n), ABS(d)) >> 31) + 1;
+	out->n = n >> shift;
+	out->d = d >> shift;
 }

static void add_scales(struct scale *out,