informedcitizenry / 6502.Net

A .Net-based Cross-Assembler for Several 8-Bit Microprocessors
MIT License
58 stars 17 forks source link

Assigning a label broke recently #2

Closed svallee-dev closed 4 years ago

svallee-dev commented 4 years ago

I have a simple Config.a that get included once in my project, that looks like so:

Config  .block
    boot_menu = Test.Menu
.endblock

With 6502.Net from about a month ago, it assemble just fine. But tonight I tried to get the latest version of 6502.Net (May 24th 2020) and now the same code is getting me an error:

Cannot assign boot_menu after it has been assigned.

svallee-dev commented 4 years ago

As a bit of context, my project assemble a file named "Main.a" that contains all the includes that are needed (see below). And "Config.a" is only ever included once, and in Main.a. The assemble error is new with the newer version of 6502.Net.

        true = 1        ; Needed by the compiler to handle .ifdef
        false = 0       ;   for some reason

* = $801

        ; BASIC HEADER
        .byte $0c, $08, $0a, $00, $9e, $20
        .byte $34, $30, $39, $36, $00, $00, $00

* = $1000

        ; -------------------------------------------------
        ;   Bootstrap
        ; -------------------------------------------------

        jmp SYS.bootstrap

        ; -------------------------------------------------
        ;   Resources & configs
        ; -------------------------------------------------

        .include "src/config.a"
        .include "generated/english.a"
        .include "generated/zero_page.a"

        ; -------------------------------------------------
        ;   Engine Code
        ; -------------------------------------------------

        .include "src/engine/general_macros.a"
        .include "src/engine/fixedPoint.a"
        .include "src/engine/math16.a"
        .include "src/engine/utils.a"
        .include "src/engine/Inputs.a"
        .include "src/engine/Rand.a"
        .include "src/engine/VRAM.a"
        .include "src/engine/Font.a"
        .include "src/engine/BG.a"
        .include "src/engine/Text.a"
        .include "src/engine/MenuManager.a"

        ; --- SYS stuff should never be accessed by game code ---
SYS     .block
        .include "src/engine/bootstrap.a"
        .include "src/engine/vblank.a"
        .include "src/engine/gameloop.a"
        .endblock

        ; -------------------------------------------------
        ;   Game Code
        ; -------------------------------------------------

        .include "src/game/menus/TestMenu.a"
svallee-dev commented 4 years ago

I was able to make a temporary hack/fix on my side to unblock me, I figure I'd mention it as it may help you find the real source of the bug. I suspect this is due to the multi-pass system, first registering the label, then trying to give it a real value on a subsequent pass, but it gets caught by an exception.

So in SymbolManager.cs, in method DefineSymbol() :

            if (exists)
            {
                var sym = _symbols[fqdn];

                if (!sym.IsScalar() || sym.NumericValue != 65535) // HACK: FIX
                {
                    if ((!sym.IsMutable &&
                        ((sym.DefinedAtIndex == -1 && Assembler.LineIterator == null) || sym.DefinedAtIndex != Assembler.LineIterator.Index)) ||
                        sym.DataType != symbol.DataType)
                    {
                        throw new SymbolException(symbol.Name, 0, SymbolException.ExceptionReason.Redefined);
                    }
                }
informedcitizenry commented 4 years ago

curious are you still getting this issue? That section of code has been refactored a fair amount. I did a barebones re-implementation of your code like so and it assembled fine:

* = $801

        ; BASIC HEADER
        .byte $0c, $08, $0a, $00, $9e, $20
        .byte $34, $30, $39, $36, $00, $00, $00

* = $1000

        ; -------------------------------------------------
        ;   Bootstrap
        ; -------------------------------------------------

        jmp SYS.bootstrap

        ; -------------------------------------------------
        ;   Resources & configs
        ; -------------------------------------------------

        .include "src/config.a"

SYS     .block
        .include "src/bootstrap.a"
        .endblock

        ; -------------------------------------------------
        ;   Game Code
        ; -------------------------------------------------

        .include "src/TestMenu.a"
informedcitizenry commented 4 years ago

I just released a new version. Can you re-try with that? A couple of things to be aware of:

Both the constants "true" and "false" are reserved words, so you will need to comment the lines out where you define them. Also, a recent change was made to how the preprocessor processes semicolon line comments. If the comment contains a colon, the parser interprets that to be a new line. This is in keeping in line with the behavior of other 6502 assemblers. Your source contains semicolon comments with colons in several places, such as line 92 of "general_macros.a". You can either change the semicolons into C-style "//" comments, or if you don't want to change your source you can give the new command line option "--ignore-colons"

svallee-dev commented 4 years ago

Thank you! I have a fairly free week-end so I should be able to test the heck out of it.

Sorry regarding the colons: I dislike the default behavior so much that I disabled them in my local version of 6502.Net. But now that I know there's a flag to disable them, I'll be able to revert my changes :)

svallee-dev commented 4 years ago

Ok looks like on subsequent assembly (after the first one), the _constants dictionary is empty. I'll investigate further, shouldn't be too hard to figure out. Screen Shot 2020-07-11 at 4 45 05 PM

svallee-dev commented 4 years ago

Ok so every time Assembler.Initialize() is called, it instantiate a new SymbolManager. But the SymbolManager only ever get populated on the Evaluator static constructor, which just happen to execute after a first Assembly, but never again in the same app instance.

One solution for me would be to never call Initialize more than once, but it feels a bit flimsy/fragile of a design. Maybe a better solution would be to move some of the Evaluator initialization into its own static method, and make sure to call it from the Assembler.Initialize().

I'd rather the second solution personally, as in my app if I ever want to toggle between different projects (z80, 6502, etc), it would be handy to run the assembler in a clean slate.

What do you think?

informedcitizenry commented 4 years ago

Wait, have you confirmed that the Assembler.Initialize() method gets called more than once? That is different to my expectation of behavior. I will try to confirm on my end as well. This is very surprising to me. Are you seeing this with even the latest version I released yesterday (2.1.7)?

informedcitizenry commented 4 years ago

OK, I saw your response on the other bug. I think for your use case you will need to handle initialization differently. I may actually end up refactoring that part of code myself. I wasn't really too happy with it tbh, but for a run-once scenario it works.

svallee-dev commented 4 years ago

That worked just great. I am now able to build my entire project, with not problem, and multiple times, with your latest repo and my older "hacks" now removed. I'm super happy!

So this last fix is pretty simple, I moved the SymbolManager specific code into their own static method: Screen Shot 2020-07-11 at 5 12 03 PM

and then I'm calling it when I instantiate the SymbolManager: Screen Shot 2020-07-11 at 5 12 15 PM

svallee-dev commented 4 years ago

"That is different to my expectation of behavior"

Yeah sorry about that. I figured you intended the code to run as a command-line. But with this simple change I just posted, it makes the code more flexible, and can accommodate projects like mine :)

informedcitizenry commented 4 years ago

Yeah. The Evaluator is a static class, which I always wondered if it was the right choice. The only reason I did it was so you could just call Evaluator.Evaluate() rather than Assembler.Evaluator.Evaluate(), or something like that. Probably shouldn't put too much state in the Evaluator itself regardless. I will look at your suggestion and see if there's another place that makes sense to initialize the constants.

EDIT: For instance, putting the code you put in your PopulateSymbolManager method inside the SymbolManager itself when it is constructed.

svallee-dev commented 4 years ago

That would indeed make more sense. I went with the quickest solution just to test it out, but you know the code base a LOT more than I do, and figured you'd think of the better way to do it :)

informedcitizenry commented 4 years ago

anyway, thanks again for your input. you're uncovering some things that needed to be looked at.

svallee-dev commented 4 years ago

Hello again! Just a quick follow up regarding the static states. While it's working most of the time, after maybe a dozen or more assembling, it does sometimes goes into a weird erroneous state and I need to restart my App to have 6502.Net assemble fine again.

I understand I'm not using it the way you intended, and to be honest what I have right now is definitely "good enough" for my needs. I just wanted to mention it in case you might decide to refactor the code to remove the static states.

Once again thank you so much for all the work you put into this project. I'll keep trying to help whenever I can :)

svallee-dev commented 4 years ago

Btw is there a message board for the project? I could not find one on this page, maybe I missed it?

informedcitizenry commented 4 years ago

Are you running your app in a multi-threaded way? That might be what's contributing to the issue. I have now committed a new build. Can you test with that?

informedcitizenry commented 4 years ago

I don't have a message board. This is really just a side thing I do when I have time to spare. Sorry. If this issue can be closed please let me know.

svallee-dev commented 4 years ago

Oh apparently I can close these bugs myself. I will go ahead. Thank you again for the help on these!