dworkin / dgd

Dworkin's Game Driver, an object-oriented database management system originally used to run MUDs.
https://www.dworkin.nl/dgd/
GNU Affero General Public License v3.0
103 stars 31 forks source link

simfloat.cpp bugs #43

Closed nyankers closed 3 years ago

nyankers commented 3 years ago

After running make install SIMFLOAT=1, I noticed a couple bugs. I haven't really analyzed them that deeply (I'm not quite sure how the math works to do any more than shallow analysis in gdb), but there seem to be more than one underlying cause.

The first issue is that Float::ftoa() can get the output off by a factor of ten sometimes, best exemplified by (string) 1001.0 which produces "100.0" because the function incorrectly calculated that there are 3 (rather than 4) digits before the decimal.

The second issue, which seems to happen before Float::ftoa() (based on debugging the a.ftoi() printouts in gdb), is that it can get numbers entirely wrong, eg. (string) 1100.0 becomes "588", and this may not be a bug in Float::ftoa() because a.ftoi() reports the same thing immediately following the a.fromFloat(this) call.

nyankers commented 3 years ago

Ah, I think I spotted the issue. It seems to be the same underlying cause for both actually. Flt::mult() is missing a cast to (short) when checking if high is negative. I can throw this in with the other pull request if you'd like, or try to figure out how to make a separate one.

dworkin commented 3 years ago

Good catch. I'll push a fix for this (more than one cast is missing).

dworkin commented 3 years ago

That code worked for 20 years, but the bitrot set in quickly after I made host floats the default. Does the latest update to master branch solve all issues for you?

nyankers commented 3 years ago

Wasn't sure the earlier arithmetic needed (short) since it seemed the assumption was it should never be high enough to matter at that point anyway, but I must be getting rusty because I definitely missed the very rare but very possible rounding overflow only a couple lines below the main culprit.

Looks like it's fixed to me, though admittedly I only found this issue when testing the other issue, so my test cases are pretty limited to begin with. I actually only found these issues because I was trying to understand sscanf(). :)

Thanks!

dworkin commented 3 years ago

I missed all of them when converting this code in 2019, where the field high is used in this function there was originally a local variable of type short, and I forgot to add casts as needed...

I recently looked at sscanf myself because it is used so much in cloudlib that it makes sense to speed it up.

nyankers commented 3 years ago

I figured it was something like that, widespread recoding tends to risk leaving tiny bugs, and I'd guess most DGD users stick with old versions rather than keeping up-to-date, whereas I'm the oddball running it on the cutting edge and hacking it so I can someday learn enough to implement the one feature I really wish it had: object variable initialization (so I can just stick mapping data = ([ ]); in a lib rather than dance around with lazy initialization or write a create() function that inherited objects just have to know to call).

Though I was just looking at sscanf because I hastily wrote sscanf(txt, "%d-", start) expecting it will match "2-" and found out it also matches "2" as well. I was actually going to ask if that was intended (that only %s looks ahead to the next match token, whereas the rest ignore it), but it seems like it in fact is!

dworkin commented 3 years ago

Yes, that is according to spec in LPC. If you want to be sure that the "-" is matched, you should use sscanf(txt, "%d-%*s", start).

Initializers in LPC is a hairy problem which has screwed up a lot of programming languages (especially with multiple inheritance) so I have avoided tackling it. But if you really want it, the best solution may be to translate another language to LPC, and implement initializers in that language, which can itself be LPC. Cloudlib has basic support for an LPC-to-LPC compiler.

nyankers commented 3 years ago

Ah, I see! I honestly haven't really done much with other LPC drivers, I'm sort of using and really liking DGD's persistence.

That is an option, but frankly I'd like to at least understand the source code enough that I can hack in totally new language features, even if they're just thought experiments in practice.

Honestly the easiest approach wouldn't even require the effort of making a new LPC derivative, I could just give libraries the capacity to have a unique create()-style method signature (based on its name, eg. "/lib/foo_bar" -> "_lib_foo__bar" or something) that the object server can track on compilation and _F_create() or its equivalent can call... all things Cloud Server basically already does to support upgrades and ~System/lib/auto, but that is getting super off-topic, so I'll spare you any further of my disastrous thoughts. :)