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

Define local labels outside the associated global label #1157

Closed
pinobatch opened this issue Aug 17, 2023 · 6 comments · Fixed by #1159
Closed

Define local labels outside the associated global label #1157

pinobatch opened this issue Aug 17, 2023 · 6 comments · Fixed by #1159
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM

Comments

@pinobatch
Copy link
Member

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

Rangi42 commented Aug 17, 2023

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

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Aug 17, 2023
@aaaaaa123456789
Copy link
Member

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

This comment was marked as off-topic.

@ISSOtm
Copy link
Member

ISSOtm commented Aug 17, 2023

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 😛 : #506 (comment)

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

Ironically, the argument I can find in that regard comes from yourself 😛 : #506 (comment)

Nice one 😛
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
Copy link
Member

ISSOtm commented Aug 17, 2023

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.

pinobatch added a commit to pinobatch/rgbds that referenced this issue Aug 17, 2023
fix gbdev#1157 for the following source code
```
section "hSAVE_locals",HRAM
func3.hSpam: ds 1  ; no longer produces an error
;.hEggs: ds 1      ; uncomment this to see the new error

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

Remove two errors:
- `Not currently in the scope of 'func3'`
- `Local label 'func3.hSpam' in main scope`

Add one error:
- `Relative local label '.hSpam' in main scope`

Add a switch to restore previous behavior in `include/asm/symbol.h`
```c
```
pinobatch added a commit to pinobatch/rgbds that referenced this issue Aug 19, 2023
fix gbdev#1157 for the following source code
```
section "hSAVE_locals",HRAM
func3.hSpam: ds 1  ; no longer produces an error
;.hEggs: ds 1      ; uncomment this to see the new error

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

Remove two errors:
- `Not currently in the scope of 'func3'`
- `Local label 'func3.hSpam' in main scope`

Add one error:
- `Relative local label '.hSpam' in main scope`

Add a switch to restore previous behavior in `include/asm/symbol.h`
```c
```

update tests of local label definition failure

now that local label definition is more lenient, fewer tests
should fail

begin removing issue number from test

Co-authored-by: Rangi <[email protected]>

treat ALLOW_LOCAL_LABEL_ANYWHERE as always on

- don't gate the new lenient behavior behind a build-time condition
- change "Relative local label" to "Unqualified local label"
  in an error message

realign tests of local labels with wrong parent

- repurpose test/asm/sym-scope from a test of producing correct
  errors to a successful test
- remove test/asm/local-wrong-parent because it is now a redundant
  successful test

based on suggestions by @Rangi42
ISSOtm pushed a commit that referenced this issue Aug 20, 2023
fix #1157 for the following source code

```
section "hSAVE_locals",HRAM
func3.hSpam: ds 1  ; no longer produces an error
;.hEggs: ds 1      ; uncomment this to see the new error

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

Remove two errors:
- `Not currently in the scope of 'func3'`
- `Local label 'func3.hSpam' in main scope`

Add one error:
- `Relative local label '.hSpam' in main scope`

Co-authored-by: Rangi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants