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

[LUA+macro] Defining same labels inside differently named modules #120

Open
Liniya opened this issue Aug 20, 2020 · 11 comments
Open

[LUA+macro] Defining same labels inside differently named modules #120

Liniya opened this issue Aug 20, 2020 · 11 comments

Comments

@Liniya
Copy link
Contributor

Liniya commented Aug 20, 2020

Version Platform Topic
v1.17.0 Windows 7 64-bit LUA

Hello,

There's a problem with defining same labels inside differently named modules when using LUA blocks inside macros. See the attached example (compile lua-modules-local-labels.asm).

Particularly, it complains about duplicated local labels, which is also the case in my main code, but in this example there are also errors about labels having different values and so on.

lua-modules-local-labels.zip

@Liniya Liniya added the bug label Aug 20, 2020
@Liniya
Copy link
Contributor Author

Liniya commented Aug 20, 2020

Never mind this for now - I forgot that I need to do LUA PASS1 when defining a function, in which case it works. Nevertheless, I believe there's still some similar bug regarding LUA and labels, which is affecting some of my own code. I'll see if I can narrow it down to a reasonable example.

Edit: alright, I think I have one. I've edited the OP and reopened it.

@Liniya Liniya closed this as completed Aug 20, 2020
@Liniya Liniya reopened this Aug 20, 2020
@Liniya Liniya changed the title Defining labels inside LUA functions [LUA+macro] Defining same labels inside differently named modules Aug 20, 2020
@ped7g ped7g self-assigned this Aug 20, 2020
@ped7g
Copy link
Collaborator

ped7g commented Aug 24, 2020

I see... this is technically "by design", although it looks a bit confusing at first sight.

Docs in macro chapter:

Labels in a macro starting with a dot are local to each macro expansion.

This is actually higher priority than module/etc, i.e. in macros the "local labels" are like new class of local labels... "macro local labels" ... :) And the regular "local labels" are not accessible from macros (to define one, you can access them in expression to evaluate their values).

To get the label you expect (?) m1.MyLabel.asd you need to specify it as non-local in the include file:

MyLabel
	ld a,a
MyLabel.asd
	ld c,b

I don't see any particularly meaningful workaround in your case (also it depends a lot what you expect from that line .asd, whether you expect module-limited label starting with m1, or macro-instance label starting with instance number).

And I don't see any obvious fix (the current design have its own purpose and inner logic, covering different kind of use cases where the same macro is emitted multiple times and where macros are nested).

I think I can add module name within the macro-local-labels without any harm to current design (ie. your example would produce 0>m1.asd and 0>m2.asd instead of 2x 0>asd, but if you need m1.MyLabel.asd, then this is of no help to you. Such change would allow for macro-local-labels to be modularized... but I see little point in that, macros are generally expected to be small and not span across multiple different module namespaces, that sounds to me like really strange use-case, I would probably try hard to avoid macros in such case (just my opinion without understanding your real use case, maybe I would agree with you on the macro usage if I would understand the full problem).

Maybe more meaningful "fix" would be to make these macro labels explicitly separate class, i.e. they would be defined with other prefix, not dot... maybe >asd as internally the > is already used for these labels any way. And use dot only for "local labels" using as prefix last non-local label. So in such case macro with .asd emitted twice in source would create error duplicating the mainLabel.asd label (or create two different labels if there was other non-local label between the two instances) - but that's new syntax design, breaking current usage of macros in sjasmplus v1.x ... so I'm making note about it - if I would work on new assembler ("v2.x"), but in current sjasmplus v1.x this works as designed.

@Liniya
Copy link
Contributor Author

Liniya commented Aug 24, 2020

Yep - I realized it might've been something similar some time after. Somewhat disappointing. The macro temp labels feature is great, but having them outright replace one kind of regular labels is frankly an awful decision. I hope this changes in the future.
-I would even go as far as to say a source INCLUDEd inside a macro doesn't necessarily have to abide by its rules for the labels, although that's perhaps arguable.

This example comes from a loader maker system, and it's how it includes various components of itself. It can build images for several different formats at once, differing mostly by the 'runtime' component of the loader, and/or the ORG to which it is compiled. The reason for using modules is exactly that: to avoid label clashing (the modules are named after the image formats, e.g. ls_loader_tap, ls_loader_scl and so on). I can accomodate for the local label issue in my own code, but the problem is that one of the included files is an unpacker, which is logically not part of the loader source but rather 'third party code', so there can be anything in it, including local labels.

I think in my situation it would help even if you just add module names to temp.label names (m1.asd). However - and while I'm not sure if it's hard to do internally - I think it makes most sense for these temp labels to abide by the same rules as all others, even if they are temporary. I.e. m1.MyLabel.asd.

@ped7g
Copy link
Collaborator

ped7g commented Aug 24, 2020

I would even go as far as to say a source INCLUDEd inside a macro doesn't necessarily have to abide by its rules for the labels, although that's perhaps arguable

Yep, I can imagine assembler where it would be defined like that, but it would probably need more time arguing and figuring out how to define it well, but it also sounds like something what would not work very well with current sjasmplus architecture. That parser code is any way good candidate for rewrite (as the mkoloberdin did in sjasmplus/sjasmplus fork), but that's quite a big task, and the instant benefits are tiny (the current 1.17.0 is already solid tool in terms of doing its job, even if the source code is sometimes questionable). But long-term it would be nice to clean the main parser architecture, then things like idea above would be open for discussion, right now I'm not even sure how the include inside macro works, I mean I spent some serious time fixing the original code to at least work as intended, but it still feels to me quite hack-ish in this regard, so it's sort of nice surprise this works so well and the files are included and assembled.

I think it makes most sense for these temp labels to abide by the same rules as all others, even if they are temporary. I.e. m1.MyLabel.asd

I will take a look on that, but it will be still in form 0>m1.MyLabel.asd (the 0> is the macro instance prefix and when nested, then the instance prefixes get appended from left like 1.0>m1.MyLabel.asd -> 25.1.0>m1.MyLabel.asd). But I need to make sure it will not affect old sources... I think it should not, because macro-defined local labels like .asd had to been accessed inside macro in the same form .asd, so however I extend the expansion, it should expand it the same way on both definition/usage, so that part may be non-breaking change? Only the labels table for old projects will change, with the macro labels becoming longer... and without non-local label inside macro I'm not sure if the one from outside of macro would be added, or the default _ would prevail, probably the outside one would leak also into macro instance.

About the loader use case: still too little info to discuss it seriously, but just a dumb question.. maybe avoid macros and use lua or explicit code + includes, so when the unpacker source is included, that's not happening inside macro instancing, but as regular code include... Or if the different modules are not used together, like producing different platform targets, then I would rather build each platform separately, doing multiple assembling steps (using probably some -Dsomething=platformSpecific defines to affect results of each build). But I guess you have some serious reasons why you did end up doing it in macro, so take these just as annoying questions from somebody who haven't seen the real thing. :)

@Liniya
Copy link
Contributor Author

Liniya commented Aug 24, 2020

I will take a look on that, but it will be still in form 0>m1.MyLabel.asd (the 0> is the macro instance prefix and when nested, then the instance prefixes get appended from left like 1.0>m1.MyLabel.asd -> 25.1.0>m1.MyLabel.asd).

I would appreciate that!

I guess you have some serious reasons why you did end up doing it in macro

Mostly ease of use. I think of it as mainly using macros that just might sometimes use LUA in them if needed. It's possible to write LUA .. ENDLUA of course, but I find it rather cumbersome, whereas with macros it's concise and feels more like 'extending sjasmplus with your own commands'. You also don't need to remember additional details such as whether some function needs ALLPASS.

@ped7g
Copy link
Collaborator

ped7g commented Mar 30, 2021

note to explore: since 1.18.0 there is exclamation mark to prevent main label to redefine the current main-prefix for following local labels (matching the syntax of current sjasm).

Maybe it could be extended to work with local-macro labels to change expansion not to macro-instance prefix, but current main label prefix, as if spawned outside of macro.

TODO:

  • check how much does that clash with original sjasm definition, IIRC from head, it has no meaning for local labels, so such new usage would not clash directly with sjasm, just add extra
  • check the original zip in this issue, if such addition makes sense (as I forgot what is in the zip, and too lazy/busy to check it now)

@ped7g
Copy link
Collaborator

ped7g commented Jul 19, 2021

So it turned out the original sjasm 0.42 has now extra syntax to create non-macro local label from inside macro, see #84 for details or check the test file listing how the new syntax works macros/local_labels.lst test

I still didn't check your original zip and whether this new feature does cover your original issue. I don't remember much about this ticket, if you would be kind enough to learn about this new syntax, and see if it can be used to resolve your original issue, and refresh this issue with new details, I would be very thankful. (please add fresh info here then, if it does apply or not, or let me know if you can't check it at all. Or close the ticket if the new feature covers it completely. Thank you.)

@Liniya
Copy link
Contributor Author

Liniya commented Sep 1, 2021

But you expect me to remember these details after a year. Hah! :)

Anyway, the issue can be summed up as follows:

	MACRO my_macro
	 INCLUDE "filename.asm"
	ENDM

filename.asm is, naturally, an arbitrary file (could be a 3rd party supplied one) that can have REGULAR local labels of its own. I want them to register as such when a macro like this is used.

A syntax to create regular local labels is a good thing, but that alone doesn't help here. I can see a couple of options that could help:
-Change the way the INCLUDE directive works inside a macro, in that all local labels found in the included files are registered as regular local labels.
-A new subdirective to switch the 'mode' for the local label creation in a macro, so that one could say 'OK, from this point all new .dot labels should register as regular ones' (or vice versa).

It's up to you what to do with this information. :)

@ped7g
Copy link
Collaborator

ped7g commented Sep 1, 2021

uff... it always surprises me the INCLUDE inside macro even works... I will not even take a look how that works and what it does internally... :D

Ok, the 1) other mode for include inside macro - my first instinct is just "no", will re-check and think about reasons, but I'm almost sure that's not a good idea.

  1. switching mode yourself before include.... ok, ok... that makes some sense, I'm not rejecting this one instantly. Will keep this open and take a deeper look on that.

@ped7g ped7g added this to the v1.18.4 milestone Sep 8, 2021
@ped7g
Copy link
Collaborator

ped7g commented Mar 27, 2022

I took a look into this, and the internal code is too complex and convoluted to simply switch off local macro labels into regular local labels. Not going to try to implement it now.

@ped7g ped7g removed this from the v1.19.0 milestone Mar 27, 2022
@ped7g ped7g removed their assignment Mar 27, 2022
@ped7g
Copy link
Collaborator

ped7g commented Aug 29, 2024

issue cleanup notes:
without major rewrite of sjasmplus internals the INCLUDE inside macros will be always pushing things way beyond expected limits and I should document that in docs that this is currently "unsupported" and you are using it on your own risk, any related bugs are likely to stall and be difficult to fix.

Keeping this around tagged as v2.x just in case I would consider refactoring the whole parsing system, to think also about this use case and make sure it is implemented better than now.

@ped7g ped7g added this to the v2.x milestone Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants