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

Implement smart linking, take 3 #1382

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Mar 27, 2024

Fixes #82.

Replaces #654 and #782. Many thanks to @ISSOtm and @daid for getting smart linking to this point!

  • Add a mechanism so that RGBASM-elided references are still emitted (lacking this is why test/link/smart/chain-wram.asm fails)
  • Add more tests covering more edge cases
  • Sections should not be purged if fully constrained
  • Apply smart linking after linker script (avoids errors & interacts with above)

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgblink This affects RGBLINK labels Mar 27, 2024
@Rangi42 Rangi42 added this to the v0.8.0 milestone Mar 27, 2024
@Rangi42 Rangi42 requested a review from ISSOtm March 27, 2024 19:30
@Rangi42 Rangi42 changed the title Implement smart linking Implement smart linking, take 3 Mar 27, 2024
This was referenced Mar 27, 2024
@Rangi42 Rangi42 force-pushed the smart-linking branch 3 times, most recently from 4c242e1 to 7f81f1e Compare March 27, 2024 20:32
@Rangi42
Copy link
Contributor Author

Rangi42 commented Mar 27, 2024

One of the test cases from #782 currently fails. Note that #782 failed it as well, so that's a TODO.

smart/chain-wram.smart.bin /tmp/tmp.ETsdvFO98G differ: char 3, line 1
00:0000 
< 00:0000: 00c0 0102 0000 0000 0000 0000 0000 0000  ................
---
00:0000 
> 00:0000: 00c0 0000 0000 0000 0000 0000 0000 0000  ................
smart/chain-wram.smart.out.bin mismatch!

This is the asm:

SECTION "root", ROM0[0]
    dw WRAMLabel

; This section should be kept thanks to the reference from the WRAM section
SECTION "A", ROM0
Label:
    db $01, $02
.end:

SECTION "wram", WRAM0
WRAMLabel:
    ds Label.end - Label

SECTION "UnRef", ROM0
UnRef:
    db UnRef

@Rangi42 Rangi42 added the WIP This PR is a work in progress label Mar 27, 2024
@Rangi42 Rangi42 force-pushed the smart-linking branch 3 times, most recently from f67797a to 64cedb2 Compare March 28, 2024 00:48
@daid
Copy link
Contributor

daid commented Mar 28, 2024

One of the test cases from #782 currently fails. Note that #782 failed it as well, so that's a TODO.

smart/chain-wram.smart.bin /tmp/tmp.ETsdvFO98G differ: char 3, line 1
00:0000 
< 00:0000: 00c0 0102 0000 0000 0000 0000 0000 0000  ................
---
00:0000 
> 00:0000: 00c0 0000 0000 0000 0000 0000 0000 0000  ................
smart/chain-wram.smart.out.bin mismatch!

This is the asm:

SECTION "root", ROM0[0]
    dw WRAMLabel

; This section should be kept thanks to the reference from the WRAM section
SECTION "A", ROM0
Label:
    db $01, $02
.end:

SECTION "wram", WRAM0
WRAMLabel:
    ds Label.end - Label

SECTION "UnRef", ROM0
UnRef:
    db UnRef

Not sure why that would be a valid test. There is no way to know where section "A" is located, you only know how big it is.

@ISSOtm
Copy link
Member

ISSOtm commented Mar 28, 2024

RGBASM is able to compute the difference between these two labels itself, and so it does: the ds line is processed as if it was ds 2, and RGBASM does not emit a reference to Label nor Label.end.

Yet, since section "wram" is kept, and it contains a source-code reference to Label, "A" should be kept by "smart linking". Shouldn't it?

@daid
Copy link
Contributor

daid commented Mar 28, 2024

At a detailed source level, yes, it contains a reference. But at a practical level higher overview, it does not contain a reference. It only contains a reference to the size (which is constant, and thus can be pre-computed before linking)

IMHO, it's one of those edge cases that does not require to work as there is no practical usage of this in any way. While fixing it is annoyingly needlessly complicated.

@aaaaaa123456789
Copy link
Member

Before this is merged, I should probably say that specifying the starting sections in the command line isn't the best idea.
I'd expect this to be given as a section attribute in the source code itself: SECTION "Foobar", ROM0, NODISCARD (or whatever you want to call the "treat this section as always referenced" attribute).

Of course, this doesn't mean that the command line flag cannot be used, but I'd expect to see a way to specify this in code — which would also function as an escape hatch for the above "should this section be included?" problem.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Mar 28, 2024

IIRC we wanted the linker script to also support designating root sections.

@Rangi42 Rangi42 force-pushed the smart-linking branch 5 times, most recently from 8fd60eb to 656b87d Compare March 28, 2024 21:27
@Rangi42
Copy link
Contributor Author

Rangi42 commented Mar 28, 2024

Proposal: add an RPN_REFCONST expression type, which would be like a constant expression, but also tracking lists of the symbol and section names which went into computing its value. When an asm-time constant is emitted that depends on a name and/or on other REFCONST expressions, build up the list. Then RGBLINK's patch_FindReferencedSections would take those names into account for finding references.

(This would be a new expression type so the existing RPN_CONST would remain unchanged, and would not be bloated by 0 section and symbol counts for the majority of cases where there aren't any.)

@ISSOtm @daid What do you think?

Edit: Oh, y'all discussed the exemplifying test case already. I agree with ISSOtm that using ld a, BANK(Label) or dw Label - OtherLabel or so on is intuitively as much of a reference as ld hl, Label is; the asm-time constant evaluation is just an optimization by us.

@Rangi42 Rangi42 force-pushed the smart-linking branch 4 times, most recently from fe2d958 to e9af039 Compare March 29, 2024 18:47
@Rangi42 Rangi42 force-pushed the smart-linking branch 3 times, most recently from 21a1eba to 832e561 Compare April 2, 2024 15:11
@Rangi42 Rangi42 force-pushed the smart-linking branch 3 times, most recently from 2eda5d6 to 75ef1b6 Compare April 21, 2024 00:30
@Rangi42 Rangi42 force-pushed the smart-linking branch 2 times, most recently from 62da49f to e958055 Compare June 12, 2024 17:27
@Rangi42 Rangi42 modified the milestones: v0.8.0, v0.9.0 Jun 13, 2024
@Rangi42 Rangi42 force-pushed the smart-linking branch 3 times, most recently from aa7a745 to 6d6da0d Compare June 19, 2024 00:36
@Rangi42 Rangi42 removed the request for review from ISSOtm June 28, 2024 21:09
@Rangi42 Rangi42 removed this from the v0.9.0 milestone Aug 6, 2024
@Rangi42 Rangi42 force-pushed the smart-linking branch 2 times, most recently from ca7ff98 to b0771cb Compare August 8, 2024 17:56
@Rangi42 Rangi42 force-pushed the smart-linking branch 2 times, most recently from e645325 to 785f338 Compare August 27, 2024 18:23
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 rgblink This affects RGBLINK WIP This PR is a work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Smart linking
4 participants