From 4dae55e02eb5c77d2a4ef993bb1f331ecaa591fd Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Fri, 23 Feb 2018 21:15:33 -0800 Subject: [PATCH] misc/kforth: Iron out numeric issues. --- doc/part-0x04.rst | 84 ++++++++++++++++++++++++++++++++++++++++++++++- io.cc | 19 +++++++---- 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/doc/part-0x04.rst b/doc/part-0x04.rst index 6f5624d..01e51d4 100644 --- a/doc/part-0x04.rst +++ b/doc/part-0x04.rst @@ -120,7 +120,7 @@ It seems like the best place for this is in ``parser.cc`` --- though I might move into a token processor later. The definition for this goes in ``parser.h``, and the body is in ``parser.cc``:: - // parse_num tries to parse the token as a signed base 10 number, + // parse_num tries to parse the token as a signed base 10 number, // pushing it onto the stack if needed. bool parse_num(struct Token *token, Stack &s) @@ -311,3 +311,85 @@ Hopefully this works:: So there's that. Okay, next time *for real* I'll do a vocabulary thing. +As before, see the tag `part-0x04 `_. + +Part B +^^^^^^^ + +So I was feeling about the work above until I tried to run this on my +Pixelbook:: + + $ ./kforth + kforth interpreter + <> + ? 2 + token: 2. + ok. + <5> + +WTF‽ I spent an hour debugging this to realise it was a bounds overflow in +``write_num``. This led me to ckecking the behaviour of the maximum and +minimum values of ``KF_INT`` which led to me revising ``io.cc``:: + + #include "defs.h" + #include "io.h" + + #include + + static constexpr size_t nbuflen = 11; + + void + write_num(IO &interface, KF_INT n) + { + + // TODO(kyle): make the size of the buffer depend on the size of + // KF_INT. + char buf[nbuflen]; + uint8_t i = nbuflen; + memset(buf, 0, i); + bool neg == n < 0; + + if (neg) { + interface.wrch('-'); + n = ~n; + } + + while (n != 0) { + char ch = (n % 10) + '0'; + if (neg && (i == nbuflen)) ch++; + +This was the source of the actual bug: ``buf[i]`` where ``i`` == ``nbuflen`` +was stomping over the value of ``n``, which is stored on the stack, too. +:: + + buf[i-1] = ch; + i--; + n /= 10; + } + + uint8_t buflen = nbuflen - i % nbuflen; + interface.wrbuf(buf+i, buflen); + } + +A couple of things here: first, the magic numbers were driving me crazy. It +didn't fix the problem, but I changed all but one of the uses of them at one +point and forgot one. So, now I'm doing the right thing (or the more right +thing) and using a ``constexpr``. Another thing is changing from ``n *= -1`` +to ``n = ~n``. This requires the check for ``neg && (i == nbuflen)`` to add +one to get it right, but it handles the case where *n* = -2147483648:: + + (gdb) p -2147483648 * -1 + $1 = 2147483648 + (gdb) p ~(-2147483648) + $2 = 2147483647 + +Notice that *$1* will overflow a ``uint32_t``, which means it will wrap back +around to -2147483648, which means negating it this way has no effect. *~n + 1* +is a two's complement. + +Finally, I made sure to wrap the buffer length so that we never try to write a +longer buffer than the one we have. + +I feel dumb for making such a rookie mistake, but I suppose that's what +happens when you stop programming for a living. The updated code is under the +tag `part-0x04-update `_. \ No newline at end of file diff --git a/io.cc b/io.cc index f9dfd1f..a4d70b8 100644 --- a/io.cc +++ b/io.cc @@ -3,25 +3,32 @@ #include +static constexpr size_t nbuflen = 11; + void write_num(IO &interface, KF_INT n) { - // TODO(kyle): make the size of the buffer depend on the size of + // TODO(kyle): make the size of the buffer depend on the size of // KF_INT. - char buf[10]; - uint8_t i = 10; + char buf[nbuflen]; + uint8_t i = nbuflen; memset(buf, 0, i); + bool neg = n < 0; + if (n < 0) { interface.wrch('-'); - n *= -1; + n = ~n; } while (n != 0) { char ch = (n % 10) + '0'; - buf[i--] = ch; + if (neg && (i == nbuflen)) ch++; + buf[i-1] = ch; + i--; n /= 10; } - interface.wrbuf(buf+i, 11-i); + uint8_t buflen = nbuflen - i % nbuflen; + interface.wrbuf(buf+i, buflen); } \ No newline at end of file