llvm dev - May 2015 - [LLVMdev] Infinite loop in ScaledNumber when calling toInt

Duncan,

I'm tracking down an infinite recursion problem when calling toInt.  The
problem seems to be that in the call to toInt, we call compareTo which, in
turn, calls toInt in one of its paths. This does not happens on 64bit
Scaled numbers.  I'm trying to work out a fix, but maybe you'll spot the
problem quicker.

Here's a unittest that triggers the problem. I ran into it with another
patch I'm working on that needs to multiply scaled32 numbers that are often
1. In tracing this, I'm thinking that we could probably add some
short-circuits to avoid so many calls when dealing with trivial numbers
(but that's unrelated).


Thanks. Diego.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150501/3fbe3128/attachment.html>
-------------- next part --------------
diff --git a/unittests/Support/ScaledNumberTest.cpp
b/unittests/Support/ScaledNumberTest.cpp
index 3872155..2f38b2a 100644
--- a/unittests/Support/ScaledNumberTest.cpp
+++ b/unittests/Support/ScaledNumberTest.cpp
@@ -556,4 +556,9 @@ TEST(ScaledNumberHelpersTest, arithmeticOperators) {
   EXPECT_EQ(ScaledNumber<uint64_t>(1, 4), ScaledNumber<uint64_t>(1,
3) << 1);
 }
 
+TEST(ScaledNumberHelpersTest, toIntBug) {
+  ScaledNumber<uint32_t> n(1, 0);
+  EXPECT_EQ(1u, (n * n).toInt<uint32_t>());
+}
+
 } // end namespace

> On 2015 May 1, at 07:52, Diego Novillo <dnovillo at google.com>
wrote:
> 
> 
> Duncan,
> 
> I'm tracking down an infinite recursion problem when calling toInt. 
The problem seems to be that in the call to toInt, we call compareTo which, in
turn, calls toInt in one of its paths. This does not happens on 64bit Scaled
numbers.  I'm trying to work out a fix, but maybe you'll spot the
problem quicker.
> 
> Here's a unittest that triggers the problem. I ran into it with another
patch I'm working on that needs to multiply scaled32 numbers that are often
1.
I was over-thinking this =/.  The attached patch removes the
(accidental) co-recursion by gutting the logic in `compareTo()`.
Feel free to commit with your test, or I can.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: simplify-compare-to.patch
Type: application/octet-stream
Size: 812 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150501/fa864e55/attachment.obj>
-------------- next part --------------

> In tracing this, I'm thinking that we could probably add some
short-circuits to avoid so many calls when dealing with trivial numbers (but
that's unrelated).

Go ahead and micro-optimize if you think it'll make the code faster
in practice.

Thanks! Committed at r236326.


Diego.

On Fri, May 1, 2015 at 12:52 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2015 May 1, at 07:52, Diego Novillo <dnovillo at google.com>
wrote:
> >
> >
> > Duncan,
> >
> > I'm tracking down an infinite recursion problem when calling
toInt.  The
> problem seems to be that in the call to toInt, we call compareTo which, in
> turn, calls toInt in one of its paths. This does not happens on 64bit
> Scaled numbers.  I'm trying to work out a fix, but maybe you'll
spot the
> problem quicker.
> >
> > Here's a unittest that triggers the problem. I ran into it with
another
> patch I'm working on that needs to multiply scaled32 numbers that are
often
> 1.
>
> I was over-thinking this =/.  The attached patch removes the
> (accidental) co-recursion by gutting the logic in `compareTo()`.
> Feel free to commit with your test, or I can.
>
>
>
>
> > In tracing this, I'm thinking that we could probably add some
> short-circuits to avoid so many calls when dealing with trivial numbers
> (but that's unrelated).
>
>
> Go ahead and micro-optimize if you think it'll make the code faster
> in practice.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150501/bbe3d400/attachment.html>