rgbds icon indicating copy to clipboard operation
rgbds copied to clipboard

Implement smart linking, take 3

Open Rangi42 opened this issue 1 year ago • 14 comments

Fixes #82.

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

  • [ ] Allow referencing sections from the linkerscript? Or even from the assembly too?
  • [ ] 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
    • [ ] Multiple -s referenced sections
    • [ ] Any still-uncovered code (use contrib/coverage.bash)
  • [x] Sections should not be purged if fully constrained
  • [x] Apply smart linking after linker script (avoids errors & interacts with above)

Rangi42 avatar Mar 27 '24 19:03 Rangi42

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 avatar Mar 27 '24 20:03 Rangi42

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.

daid avatar Mar 28 '24 10:03 daid

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?

ISSOtm avatar Mar 28 '24 10:03 ISSOtm

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.

daid avatar Mar 28 '24 10:03 daid

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.

aaaaaa123456789 avatar Mar 28 '24 11:03 aaaaaa123456789

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

Rangi42 avatar Mar 28 '24 12:03 Rangi42

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 avatar Mar 28 '24 22:03 Rangi42