Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assigning a label broke recently #2

Closed
svallee-dev opened this issue May 25, 2020 · 19 comments
Closed

Assigning a label broke recently #2

svallee-dev opened this issue May 25, 2020 · 19 comments
Labels

Comments

@svallee-dev
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

informedcitizenry commented Jul 2, 2020

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

"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
Copy link
Owner

informedcitizenry commented Jul 12, 2020

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
Copy link
Author

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
Copy link
Owner

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

@svallee-dev
Copy link
Author

svallee-dev commented Jul 17, 2020

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
Copy link
Author

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

@informedcitizenry
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants