Merge a huge WLA parser upgrade with other good mods and bug fixes from Jason Matson

[vhelin]

I got the modified sources via email, will write the changes here later, but this one is going to be really good! :dancers:

[cr1901]

Anything that would (further :P) break existing code?

[vhelin]

I haven't looked at the code, or tested the changes, but one test in the examples directory should break according to the text that came along with the sources. Let's see, I'll try to push process and push those new sources to GitHub in the next couple of days if I find it working well enough. :)

On Tue, May 31, 2016 at 6:42 PM, William D. Jones notifications@github.com wrote:

Anything that would (further :P) break existing code?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-222664471, or mute the thread https://github.com/notifications/unsubscribe/AGHtUl9wTuBjA9lKiaegvZHbFHPnYupPks5qHB6OgaJpZM4IqOkj .

[nicklausw]

Could you describe some of the mods and fixes?

[vhelin]

Here is a quote from Jason's email (I hope this is ok with him):

...

"I wanted to share another change I put together for WLA-DX. This is a pretty big one. The inspiration behind the change was to support macro's as any part of an instruction. So something like the following would work (I got the original idea when I was trying to optimize some macros for variable register input):

.MACRO INSTRUCTION_TEST \1 \2, \3 .ENDM

INSTRUCTION_TEST LD, A, B

This ended up being a lot more difficult than I originally expected. I ended up doing a major rewrite on parse.c to replace the buffer/i usage into the new token_stack functionality that automates replacement of arguments in the buffer.

The second major change is how macro arguments are handled during a macro call. Previously they were evaluated on the initial call, the updated delays the processing until the argument is substituted in the line where its used, this properly handles usage such as:

INSTRUCTION_TEST LD A, ($0000)

So that ($0000) is treated as a memory lookup (on gameboy) instead of evaluating it early as an expression."

...

That token stack sounds like it could be quite flexible and permit future enhancements easier. :)

On Tue, May 31, 2016 at 6:56 PM, nicklausw notifications@github.com wrote:

Could you describe some of the mods and fixes?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-222667162, or mute the thread https://github.com/notifications/unsubscribe/AGHtUvilKUaXhKNrcu6d-IUILFyz_FO6ks5qHCHYgaJpZM4IqOkj .

[nicklausw]

That actually sounds super useful. I remember seeing a macro in RGBDS that went like...

.macro bla ld [\1], \2 .endm

Called like... bla hl, a

Think those changes would support this?

[vhelin]

On Tue, May 31, 2016 at 7:56 PM, nicklausw notifications@github.com wrote:

That actually sounds super useful. I remember seeing a macro in RGBDS that went like...

.macro bla ld [\1], \2 .endm

Called like... bla hl, a

Think those changes would support this?

I think they will, but not 100% sure. Need to try them out, and look at the code. :)

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-222679962, or mute the thread https://github.com/notifications/unsubscribe/AGHtUurytZ1Cmo0xPT1en7FQyde16YAIks5qHDAJgaJpZM4IqOkj .

[vhelin]

Ok this merge will take some time as it's coded 1999 C style (I think), not C89 compilant...

[vhelin]

Ok, I made a new branch for this one, as I didn't understand all the new things, plus some more testing with real project by you guys would be good. :+1:

Anyway, here are the changes, I quote Jason himself (got permission):

...

I wanted to share another change I put together for WLA-DX. This is a pretty big one. The inspiration behind the change was to support macro's as any part of an instruction. So something like the following would work (I got the original idea when I was trying to optimize some macros for variable register input):

.MACRO INSTRUCTION_TEST \1 \2, \3 .ENDM

INSTRUCTION_TEST LD, A, B

This ended up being a lot more difficult than I originally expected. I ended up doing a major rewrite on parse.c to replace the buffer/i usage into the new token_stack functionality that automates replacement of arguments in the buffer.

The second major change is how macro arguments are handled during a macro call. Previously they were evaluated on the initial call, the updated delays the processing until the argument is substituted in the line where its used, this properly handles usage such as:

INSTRUCTION_TEST LD A, ($0000)

So that ($0000) is treated as a memory lookup (on gameboy) instead of evaluating it early as an expression.

I've tested the changes quite a bit and verified the binary results of the examples match the current version of WLA-DX (with the exception of the timestamp/CRC in the Game Boy projects).

Some additional changes: Extended error reporting for macros, it reports details on what line called the macro. Split up parse.c to remove redundant parse logic. Rework operator handling in stack.c, to handle extended operator precedence. Fix a bug with wlalink/write.c that caused _f/_b to fail. Treat labels as strings in .IF directives. Fix warnings with fprintf usage in pass3.c. Fix warnings with hash key constant in hashmap.c.

...

I also made some coding style reformatting, and made a couple of fixes here and there as not everything passed through my Cygwin's compiler (Jason is using Visual Studio in Windows). I hope it works! :) And I'll spend more time in the near future trying to understand all the new stuff plus perhaps make new tests as well. When all looks good, let's merge this to master.

[vhelin]

And the new branch is called "parserUpgrade": https://github.com/vhelin/wla-dx/tree/parserUpgrade

[ssjason123]

Hey all, its Jason. I setup an account, let me know if you run into any issues and I will help take a look.

[vhelin]

Welcome here, Jason! :)

On Sat, Jun 4, 2016 at 11:15 PM, ssjason123 notifications@github.com wrote:

Hey all, its Jason. I setup an account, let me know if you run into any issues and I will help take a look.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-223763998, or mute the thread https://github.com/notifications/unsubscribe/AGHtUuW7HL70Tfz_iOOZMlfegzhluBwkks5qIaSEgaJpZM4IqOkj .

[vhelin]

cr1901 & nicklausw: Any ideas why ssjason123 cannot create pull requests? I'm using SourceTree git client, and it's been working quite ok for quite a long time, with GitHub and my own repos...

[cr1901]

@ssjason123 needs to fork a copy of WLA, make his own changes, then submit a pull request. It looks like he doesn't have any repos yet.

[ssjason123]

Thanks! I will give that a shot. I am new to using Git outside of my own local projects for revision control so please excuse my ignorance :).

[Stewmath]

Any ETA on when this is getting merged into master?

[vhelin]

I haven't gotten any reports about people using the branch, so almost no-one has tested it? :(

On Sun, Jul 17, 2016 at 5:13 AM, Drenn1 notifications@github.com wrote:

Any ETA on when this is getting merged into master?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-233161254, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHtUhE1zDixlMfIMHthXg1lGXzBF686ks5qWY_jgaJpZM4IqOkj .

[Stewmath]

Turns out my labels branch merges easily with this, so I guess I'll be a guinea pig.

Something's throwing off the sizes of my sections. I have a macro that looks like this:

.macro m_TreeRefillData
    .db \1&$ff \2|(\1>>8)
.endm

Yet it's only defining one byte per use. If I change it to this:

.macro m_TreeRefillData
    .db \1&$ff
    .db \2|(\1>>8)
.endm

Then it works. I haven't been able to replicate this outside of my project yet...

---

Here's an issue I can replicate. I have this line:

    ld bc,$84<<8 | $0c

Which produces:

main.s:66101: STACK_CALCULATE: Out of 16-bit range.
main.s:66101: ERROR: Couldn't parse "ld".
make: *** [Makefile:80: build/main.o] Error 1

Something is very, very wrong there, as when I told it to print the number, it gave me a different one every time...

That issue doesn't occur when there are brackets, like this:

    ld bc,($84<<8) | $0c

EDIT: I just realized this could be an issue with operator precedence. Did that get changed? It doesn't appear to be documented in the first place.

[Stewmath]

I figured out part of the problem with the first issue - the use of brackets in the arguments. Here's a project demonstrating the bug.

https://github.com/Drenn1/wla-dx/commit/16c496ca3ed123faeddffa03bbbfab92d3574713

[Stewmath]

So, this seems to have gotten stuck in development hell... well, perhaps that's not the right term. Anyway, it would be nice if there was some kind of game plan for dealing with this branch, since I find myself unsure whether to look at "master" or "parserUpgrade" when I want to look into bugs.

I don't understand these parts of WLA very well, but if it helps at all, I've looked a bit further into compatibility issues (this is from the parserUpgrade branch on jason's account):

• Macros seem to be separating arguments by whitespace. That's a problem for existing programs that do stuff like this:

  mymacro 1 | 2 ; This will be seen as 3 arguments

  It seems like this change can be reverted in the "parse_type_expression" function (by removing the special-case behaviour for macro arguments), but I'm not really sure what I'm doing when I change that...

• I think precedence got changed somehow. The expression 2<<4+1 evaluates to $21 in the current "master" branch, and $40 in this branch. It seems like "shift" has lower precedence in general, which will break a lot of code...

[ssjason123]

Operator precedence in the parser upgrade is handled in stack.c get_operator_precedence. The shift operator precedence should be much higher, but to work properly for the 65816 - linker_test project it had to be placed lower than XOR to evaluate correctly. I included the following notes in the source: /* TODO: shift should be precedence 5 to match the c standard, but to maintain compatibility with test it's flagged below XOR: 65816 - linker_test L73: .db $ff~(%0+$ff-1)<<1 The normal priority of left shift should perform it before the XOR but the test expects the shift after the XOR. */

Feel free to reorder to whatever makes sense.

As for macro arguments, WLA originally implemented both space and comma delimiters for arguments. I am not a fan of this as it makes it ambiguous if you are expecting to pass a single argument as an expression or individual arguments to the macro. The else case in parse_type_expression is used to delay the expression evaluation to prevent stack evaluation on symbols before the final expression is constructed which would result in a stack being created that looks for an invalid symbol eventually leading to a link failure. I think parse_type_argument_to_string is what handles this specific case and would need to be updated to handle un-grouped "()" expressions to avoid sending each space delimited item in the expression as a separate argument.

As for the future for the parserUpgrade branch, I cant say. It seems like it was kind of doomed from the start by being placed on a separate branch that no one is really aware of resulting in it getting stuck in a cycle of needing to be tested but since no one is using it its not getting tested.

I've stepped away from development using WLA and started playing with a design for my own assembler to try and resolve some of the concerns I had with WLA. I will still be around to help resolve issues with the parserUpgrade branch if its something that people see value in, otherwise I think it would be better to make it obsolete and just continue using the main branch.

[vhelin]

Sorry guys, I'm not much help currently, having some real life problems that come with the highest priority... Anyway, I was hoping people would try out the parserUpgrade branch and report back any bugs they found - doesn't seem to work that way as I've gotten zero feedback on the branch.

Perhaps we should just merge it with the main branch and live with it? :) For me the parserUpgrade seemed to work nicely, but then again I'm not using WLA DX these days for anything real... Ideas? Lets come to some conclusion about this asap, ok?

On Sat, Jan 7, 2017 at 7:40 PM, ssjason123 notifications@github.com wrote:

Operator precedence in the parser upgrade is handled in stack.c get_operator_precedence. The shift operator precedence should be much higher, but to work properly for the 65816 - linker_test project it had to be placed lower than XOR to evaluate correctly. I included the following notes in the source: / TODO: shift should be precedence 5 to match the c standard, but to maintain compatibility with test it's flagged below XOR: 65816 - linker_test L73: .db $ff~(%0+$ff-1)<<1 The normal priority of left shift should perform it before the XOR but the test expects the shift after the XOR. /

Feel free to reorder to whatever makes sense.

As for macro arguments, WLA originally implemented both space and comma delimiters for arguments. I am not a fan of this as it makes it ambiguous if you are expecting to pass a single argument as an expression or individual arguments to the macro. The else case in parse_type_expression is used to delay the expression evaluation to prevent stack evaluation on symbols before the final expression is constructed which would result in a stack being created that looks for an invalid symbol eventually leading to a link failure. I think parse_type_argument_to_string is what handles this specific case and would need to be updated to handle un-grouped "()" expressions to avoid sending each space delimited item in the expression as a separate argument.

As for the future for the parserUpgrade branch, I cant say. It seems like it was kind of doomed from the start by being placed on a separate branch that no one is really aware of resulting in it getting stuck in a cycle of needing to be tested but since no one is using it its not getting tested.

I've stepped away from development using WLA and started playing with a design for my own assembler to try and resolve some of the concerns I had with WLA. I will still be around to help resolve issues with the parserUpgrade branch if its something that people see value in, otherwise I think it would be better to make it obsolete and just continue using the main branch.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-271097980, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHtUlxNT6lskFiMDRy8yUxCJwJnQsu-ks5rP84JgaJpZM4IqOkj .

[Stewmath]

If you want to keep parserUpgrade, you'll need to be prepared for a lot of breakage - at least, if this macro behaviour stays the same. Perhaps using commas as delimeters will help a bit - this way, macros with one argument, at least, won't break.

Personally I'm not sure if it's worth the breakage. If you want to use this branch, though, I'll keep reporting issues until I get my project working again.

[vhelin]

Good to know at least someone has tested the parserUpgrade branch. :)

Ouch.

Even though it breaks compatilibity I think this new parser is much better than what we've had so far. We could increase the major number of WLA DX and add a big warning in the documents about this... But yeah, might not be a nice move.

What other cases is the new parser breaking, what have you seen?

On Sat, Jan 7, 2017 at 10:43 PM, Drenn1 notifications@github.com wrote:

If you want to keep parserUpgrade, you'll need to be prepared for a lot of breakage - at least, if this macro behaviour stays the same. Perhaps using commas as delimeters will help a bit - this way, macros with one argument, at least, won't break.

Personally I'm not sure if it's worth the breakage. If you want to use this branch, though, I'll keep reporting issues until I get my project working again.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-271108876, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHtUoXag8eigGuJpj4-XLT9DKdTVefLks5rP_jegaJpZM4IqOkj .

[vhelin]

On Sat, Jan 7, 2017 at 11:34 PM, Ville Helin ville.helin@gmail.com wrote:

Good to know at least someone has tested the parserUpgrade branch. :)

Ouch.

Even though it breaks compatilibity I think this new parser is much better than what we've had so far. We could increase the major number of WLA DX and add a big warning in the documents about this... But yeah, might not be a nice move.

What other cases is the new parser breaking, what have you seen?

... in addition to that problem with shifting... Which is actually a pretty big problem.

On Sat, Jan 7, 2017 at 10:43 PM, Drenn1 notifications@github.com wrote:

If you want to keep parserUpgrade, you'll need to be prepared for a lot of breakage - at least, if this macro behaviour stays the same. Perhaps using commas as delimeters will help a bit - this way, macros with one argument, at least, won't break.

Personally I'm not sure if it's worth the breakage. If you want to use this branch, though, I'll keep reporting issues until I get my project working again.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-271108876, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHtUoXag8eigGuJpj4-XLT9DKdTVefLks5rP_jegaJpZM4IqOkj .

[vhelin]

On Sat, Jan 7, 2017 at 11:37 PM, Ville Helin ville.helin@gmail.com wrote:

On Sat, Jan 7, 2017 at 11:34 PM, Ville Helin ville.helin@gmail.com wrote:

Good to know at least someone has tested the parserUpgrade branch. :)

Ouch.

Even though it breaks compatilibity I think this new parser is much better than what we've had so far. We could increase the major number of WLA DX and add a big warning in the documents about this... But yeah, might not be a nice move.

What other cases is the new parser breaking, what have you seen?

... in addition to that problem with shifting... Which is actually a pretty big problem.

... and if one of my tests in WLA DX is somehow bad or broken, please don't let it stop you from fixing WLA DX. :D i.e. old WLA behaviour might be wrong and the current tests somehow broken. Sorry guys, my head is spinning right now. I'll need to digest this thread for a while...

On Sat, Jan 7, 2017 at 10:43 PM, Drenn1 notifications@github.com wrote:

If you want to keep parserUpgrade, you'll need to be prepared for a lot of breakage - at least, if this macro behaviour stays the same. Perhaps using commas as delimeters will help a bit - this way, macros with one argument, at least, won't break.

Personally I'm not sure if it's worth the breakage. If you want to use this branch, though, I'll keep reporting issues until I get my project working again.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/vhelin/wla-dx/issues/111#issuecomment-271108876, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHtUoXag8eigGuJpj4-XLT9DKdTVefLks5rP_jegaJpZM4IqOkj .

[Stewmath]

Thinking about it a bit more, if wla were to check for arithmetic operators when collecting arguments, then put them all together, that might solve some of the issues. No evaluation needed at that time. (Basically, what Jason said =P )

Though that wouldn't fix everything. Assuming that wla currently evaluates macro arguments immediately (I'm not actually sure), this could lead to situations where a macro has an expression like this:

\1 * \2

And then it's given parameters 1+2, 3... and you have a classic C-style macro issue.

There seem to be some other issues - seemingly a bug where wlalink is silently failing to evaluate a label...

[Stewmath]

Here's my current version of parserUpgrade: https://github.com/Drenn1/wla-dx/tree/parserUpgrade

History's a bit messy with recent merging, but this is the only real significant commit after Jason's: https://github.com/Drenn1/wla-dx/commit/12634e87d06a4d8fc49ef994869030c08a8ea999

[lf94]

Please make this happen soon! :D I encountered needing this functionality this week.

My work around was to compile the instructions and do this:

.macro myMacro
; Stuff here
.db \1
.if NARGS == 2
.db \2
.endif
.if NARGS == 3
.db \3
.endif
; Other things there
.endm

[Stewmath]

I dunno what your use-case is, but that macro can be done like this:

.macro myMacro

.rept NARGS
    .db \1
    .shift
.endr

.endm

Anyway, this branch currently has a conflict with master from commit d19cd977527a01a04427acbbea9b51a2c957f34f. I didn't actually change that much in that commit - git seriously went screwy when generating that diff. I could revert it and try to avoid the monster diff, but that all depends if this branch is still desired.

[vhelin]

Ok, I have to admit that too much time has passed since this - I guess the change was too big and happened at personally at a bad time so I guess we'll need to close this issue and move on. If you find out that this change would have added something really useful, please create a new issue here and I'll deal with it faster.