Bug 148 - Expression evaluator and EQUR rewrite and/or cleanup
Summary: Expression evaluator and EQUR rewrite and/or cleanup
Status: UNCONFIRMED
Alias: None
Product: RMAC
Classification: Unclassified
Component: Core (show other bugs)
Version: unspecified
Hardware: PC Linux
: Normal enhancement
Assignee: Shamus Hammons
URL:
Depends on:
Blocks:
 
Reported: 2020-01-17 15:25 CST by ggn
Modified: 2020-01-20 00:48 CST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description ggn 2020-01-17 15:25:10 CST
It is evident that from bugs #70 and #139 (and probably a bunch of other closed tickets) that if anyone opens expr.c there a ton of weird code (and commented out) and it caters for many corner cases that one begins to wonder how it got to be that way!

Now I don't know who will do the work or when, I just wanted to jot down a few observations and ideas so we don't have to think about these each and every time.

First of all, EQUR. This has been problematic for as long as I can remember rmac. procln.c has massive amounts of code for what looks like a simple operation. And there's also some corner case handling in expr.c which really does not make any sense to me!

My main idea around this is that EQUR/UNEQUR should happen explicitly inside procln.c. Just keep a list of defined equated registers and expand them on the spot. Also, because the code was bolted in with only Tom/Jerry in mind, it prohibits us from applying it in all CPUs. Hey, if you think the Jaguar riscs have many registers just try counting the 56001 ones :).

And expr() inside expr.c has so many tests crammed in huge ifs... it's practically unreadable. Personally when I hit a bug there I just load VS and start tracing things without having a super clear vision of what the hell is going on. This is never a good idea - if we don't own the code then we will forever be afraid to change it. At the very least the excess code should be wiped (as it was relevant in god knows how many versions back), the existing code be simplified and documented better. Not to say there's no explanations in there, but they're all over the place.

While we're at it, TBL has written a preprocessor tool for vasm that has a more advanced register allocator/deallocator here: https://github.com/deplinenoise/deluxe68. I'm in favor of such stuff be an internal part of the assembler and not external (indeed in their write up they mention that they had to patch vasm because error reporting after the code was processed by the tool was very cumbersome), and maybe we could nick a few ideas to add to rmac. Again, not saying we should use any at all, but a couple don't seem half bad!

Anyway, that's that for now...
Comment 1 ggn 2020-01-20 00:48:11 CST
People at work asked me "How was your weekend?". Well, I realised that in my mind I completely blocked out what I did on Saturday, i.e. figure out why rmac wasn't passing the regression tests on ARM.

First thing I fixed was #149, which took me an age to pinpoint the exact issue . But that's just the debugging tools, nothing unexpected there.

Then... the hard drugs. expr(). To keep this short, here's the original piece of code that caused the issue (which has been rewritten a few times and left commented out in that file):

	if ((tok[1] == EOL && (tok[0] != CONST && tokenClass[tok[0]] != SUNARY))
		|| (((tok[0] == SYMBOL)	|| (tok[0] >= KW_R0 && tok[0] <= KW_R31)) && (tokenClass[tok[2]] < UNARY))
		|| ((tok[0] == CONST) && (tokenClass[tok[3]] < UNARY))
		)

The detail here is that tokenClass is an array of 256 entries. Now, imagine what could happen if tok[2] or tok[3] is greater than 255? Bingo, out of bounds access. So I.... did something. It's not a fix, lord no. Just something that lets it pass the regression tests.

	if ((tok[1] == EOL && (tok[0] != CONST && tokenClass[tok[0]] != SUNARY))
		|| (((tok[0] == SYMBOL) || (tok[0] >= KW_R0 && tok[0] <= KW_R31)) && (tok[2]>=256 || tokenClass[tok[2]] < UNARY))
		|| ((tok[0] == CONST) && (tok[3]>256 || tokenClass[tok[3]] < UNARY))
		)

No patch for this from me, this clearly needs to be rewritten. Bonus points if anyone explains to me why the KW_R0<tok[0]<KW_R31 check exists here. I didn't even bother to look at what the code does.