gbdev / rgbds

Rednex Game Boy Development System - An assembly toolchain for the Nintendo Game Boy and Game Boy Color
https://rgbds.gbdev.io
MIT License
1.33k stars 175 forks source link

Define local labels outside the associated global label #1157

Closed pinobatch closed 1 year ago

pinobatch commented 1 year ago

I'd like to create HRAM variables that are local to a particular subroutine in ROM using local label subroutine.hVarName syntax.

I'm writing a local variable allocation tool. Pending a fix for #1146, it reads all source code files in a directory, looks for global labels (which "Labels" in the manual defines as those not containing a .), infers a call graph of subroutines, reads declarations of local variables associated with each global label, and comes up with an allocation such that each subroutine's variables start after those of its callees. This way, different subroutines' variables can overlap in memory so long as a subroutine's variables doesn't overlap those of its callees. (Recursion is assumed not to happen, as it is uncommon in 8-bit programs.)

I expect the output of this call graph analyzer to look like this:

section "hSAVE_locals",HRAM
  union
  ; func1 has no callees
func1.hXPos:: ds 1
func1.hYPos:: ds 1
func1.hWidth:: ds 1
func1.hHeight:: ds 1
func1.hTileID:: ds 1
func1.hAttr:: ds 1
  nextu
  ds 6  ; func2's largest callee is func1
func2.hFoo:: ds 1
func2.hBar:: ds 1
func2.hBaz:: ds 1
  nextu
  ds 6  ; func3's largest callee is func1
func3.hSpam:: ds 1
func3.hEggs:: ds 1
func3.hBacon:: ds 1
  endu

Notice that func2 and func3 variables overlap, whereas neither overlaps func1 because the subroutines func2 and func3 call func1.

The associated subroutine would use the variable as follows:

section "demo",ROM0
func3:
  ldh a, [.hSpam]
  ret

However, RGBASM fails to build the analyzer's output, instead emitting the following diagnostic:

error: generated_union.asm(4):
    Not currently in the scope of 'func1'

Thus I'd need a way to deliberately make a label that is local to a subroutine yet defined outside the body of that subroutine.

The other way to do it would involve using _ instead of . between the subroutine and variable name and having a macro emit equs statements to define short names for each variable. That would depend on the macro knowing the name of the subroutine, as I described in #1156.

Rangi42 commented 1 year ago

I'm sure this has been proposed before, but maybe just in discussion, not its own issue. Anyway, I do support it, particularly since we can already redundantly declare main.local labels underneath a main label, just not yet above it.

Here's a (somewhat artificial) use case:

memcmp.loop:
    inc de
.foo ; this is memcmp.foo
    inc hl
memcmp::
; @input hl = pointer1, de = pointer2, c = length
; @return z if all c bytes of hl and de match, c if hl > de
; @precondition c > 0
    ld a, [de]
    cp [hl]
    ret nz
.bar ; this is memcmp.bar
    dec c
    jr nz, .loop
    ret
aaaaaa123456789 commented 1 year ago

I personally see no reason to not allow this if it's reasonably easy to implement. Preempting the usual "this can turn code unreadable" arguments from some sides: so what? Don't misuse the feature.

zlago commented 1 year ago

Preempting the usual "this can turn code unreadable" arguments from some sides: so what? Don't misuse the feature.

clearly we need to make macros only let you use d[bwls] in them, or better yet, remove macros, otherwise people will try to define code macros via db /j

ISSOtm commented 1 year ago

Label scopes are "just" a social construct anyway: in the end, it's only symbol names, which RGBLINK only treats as a blob of bytes. For that reason, scopes are merely a way of ascribing a hierarchy to labels, and in that regard it would make sense to allow defining labels in different memory regions under the same "scope".

The question is then the usual syntax bikeshedding. Pino's proposed syntax in the OP seems reasonable to me (explicitly specifying the function scope? Sure); implementing it would be as simple as removing the check that produces the error also in the OP—I don't think there is any technical reason it's this way.

I personally see no reason to not allow this if it's reasonably easy to implement. Preempting the usual "this can turn code unreadable" arguments from some sides: so what? Don't misuse the feature.

Ironically, the argument I can find in that regard comes from yourself šŸ˜› : https://github.com/gbdev/rgbds/issues/506#issuecomment-610869159

Side notes

It seems like it would also make sense to allow defining non-label symbols under "scopes"; they were rejected after #423, which is mostly technical and rejection was the simplest and sanest solution at the time; the general cleanup of the codebase may have made this easier to implement nowadays.

I think this can already be shimmed, at least for this specific use case, by making func1 into the active scope and then purgeing it, but that comes with several caveats.

aaaaaa123456789 commented 1 year ago

Ironically, the argument I can find in that regard comes from yourself šŸ˜› : #506 (comment)

Nice one :stuck_out_tongue: That being said, I was comparing it against other ideas for the specific use case in question. If this is a more general feature, I won't question it.

It seems like it would also make sense to allow defining non-label symbols under "scopes"; they were rejected after #423, which is mostly technical and rejection was the simplest and sanest solution at the time; the general cleanup of the codebase may have made this easier to implement nowadays.

Would these just be equivalent to defining them in fully-qualified form?

ISSOtm commented 1 year ago

A solution I see against the two sources of truth is to make RGBLINK warn if it sees any scoped label, but not its parent. (Though that check may be expensive, and it seems likely you'd get "undefined symbol" errors from a typo in the first place, so maybe it wouldn't be worth implementing.)

Yes, in general the ...name syntax is merely sugar / a shorthand to avoid repetition, and I intend for it to remain only an in-place name "expansion" with no semantic differences.