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

backwards indirect goto does not compile #1209

Open
jmr opened this issue Aug 21, 2020 · 18 comments
Open

backwards indirect goto does not compile #1209

jmr opened this issue Aug 21, 2020 · 18 comments

Comments

@jmr
Copy link
Contributor

jmr commented Aug 21, 2020

int f (void)
{
    static void *x[1];
    L: if (x[0] != 0) return 0;
    x[0] = &&L;
    goto *x[0];
}

Results in:

Unresolved external 'L0004' referenced in:
  bugXXXX-ind-goto-rev.s(61)
  bugXXXX-ind-goto-rev.s(62)

This works with gcc.

It looks like something is wrong with the way GotoStatement is trying to prevent labels from getting deleted.

I'll send a PR with a test case soon.

@mrdudz
Copy link
Contributor

mrdudz commented Aug 21, 2020

Now this is some crazyness. I'd file it under "barely legal" :=)

@acqn
Copy link
Contributor

acqn commented Aug 21, 2020

Is this the expected output (compiled with a #897 solution)?

; ---------------------------------------------------------------
; int __near__ f (void)
; ---------------------------------------------------------------

.segment	"CODE"

.proc	_f: near

.segment	"CODE"

;
; L: if (x[0] != 0) return 0;
;
L0002:	lda     M0001
	ldx     M0001+1
	cpx     #$00
	bne     L0006
	cmp     #$00
L0006:	jsr     boolne
	jeq     L0003
	ldx     #$00
	lda     #$00
	jmp     L0001
;
; x[0] = &&L;
;
L0003:	lda     #<(L0002)
	ldx     #>(L0002)
	sta     M0001
	stx     M0001+1
;
; goto *x[0];
;
	ldy     #$00
	lda     M0001,y
	ldx     M0001+1,y
	jmp     callax
;
; }
;
L0001:	rts

.segment	"BSS"

M0001:
	.res	2,$00

.endproc

@jmr
Copy link
Contributor Author

jmr commented Aug 22, 2020

Yes, that looks right.

jmr added a commit to jmr/cc65 that referenced this issue Aug 22, 2020
Change err/ tests to use cl65 and .prg instead of cc65 and .s
since this test only fails at the link stage.
jmr added a commit to jmr/cc65 that referenced this issue Aug 22, 2020
Change err/ tests to use cl65 and .prg instead of cc65 and .s
since this test only fails at the link stage.
@jmr
Copy link
Contributor Author

jmr commented Aug 22, 2020

@acqn If you have a fix for #897, it would be great to get that in. Maybe it could allow cleanup of the indirect goto handling code and the jump tables I'm working on for #1049.

oliverschmidt pushed a commit that referenced this issue Aug 22, 2020
Change err/ tests to use cl65 and .prg instead of cc65 and .s
since this test only fails at the link stage.
@jmr
Copy link
Contributor Author

jmr commented Aug 22, 2020

@clbr it looks like you implemented computed gotos.

@clbr
Copy link
Contributor

clbr commented Aug 23, 2020

Dynamic computed gotos are not supported, so from that sense this is not a bug.

@jmr
Copy link
Contributor Author

jmr commented Aug 23, 2020

Based on the generated code, there's no reason it shouldn't work. It's just the code preventing the label deletion that doesn't work.

If the intent really were to not support this, there should be an error like "dynamic computed goto not supported", rather than an obscure linker error.

@jmr
Copy link
Contributor Author

jmr commented Aug 23, 2020

Here's version with the same failure and no dynamic computed goto.

static unsigned char y = 0;
int f(void) {
    L: if (y) return 0;
    {
        static const void *const x[1] = {&&L};
        y = 1;
        goto *x[0];
    }
}
% cl65 -t sim6502 /tmp/back.c
Unresolved external 'L0003' referenced in:
  /tmp/back.s(47)
Unresolved external '_main' referenced in:
  runtime/callmain.s(27)
ld65: Error: 2 unresolved external(s) found - cannot create output file

@oliverschmidt
Copy link
Contributor

Now this is some crazyness. I'd file it under "barely legal" :=)

Dynamic computed gotos are not supported, so from that sense this is not a bug.

If the intent really were to not support this, there should be an error like "dynamic computed goto not supported", rather than an obscure linker error.

That's what made me classify this issue as bug.

@clbr
Copy link
Contributor

clbr commented Aug 23, 2020

There is no such error message because it didn't occur to me at the time someone would try something that crazy...

@clbr
Copy link
Contributor

clbr commented Aug 23, 2020

The new sample still confuses me on why you would do that in the first place, but ok, it's a valid bug.

@jmr
Copy link
Contributor Author

jmr commented Aug 23, 2020

I don't actually want to do any of the things in this bug, but there should be no reason not to allow them. I was trying to look at the computed goto label handling code to see how things should work because there are (unsurprisingly) similar issues for #1049 trying to use jump tables for switch. I looked at the code, said, "wait, how does this work?" and found that it doesn't. #1211 is equivalent to something that needs to be handled for #1049, so I'm more interested in a fix for that one.

jmr added a commit to jmr/cc65 that referenced this issue Aug 23, 2020
This test case don't use dynamic labels.
cc65#1209 (comment)
jmr added a commit to jmr/cc65 that referenced this issue Aug 24, 2020
These test cases don't use dynamic labels.
cc65#1209 (comment)

Also update the original test case for consistency:
* Change failure message to just "FAIL", as there is only one failure
* Outdent label definitions
* Clarify description
jmr added a commit to jmr/cc65 that referenced this issue Aug 24, 2020
misc/ is the correct place for tests that should compile
but do not.

Revert err/Makefile changes from cc65#1210.
oliverschmidt pushed a commit that referenced this issue Aug 24, 2020
misc/ is the correct place for tests that should compile
but do not.

Revert err/Makefile changes from #1210.
jmr added a commit to jmr/cc65 that referenced this issue Aug 24, 2020
These test cases don't use dynamic labels.
cc65#1209 (comment)

Also update the original test case for consistency:
* Change failure message to just "FAIL", as there is only one failure
* Outdent label definitions
* Clarify description
oliverschmidt pushed a commit that referenced this issue Aug 24, 2020
These test cases don't use dynamic labels.
#1209 (comment)

Also update the original test case for consistency:
* Change failure message to just "FAIL", as there is only one failure
* Outdent label definitions
* Clarify description
@acqn
Copy link
Contributor

acqn commented Aug 24, 2020

My fix for this depends on something tha depends on something that depends on something that depends on something... that is seemingly irrelevent to this one.... The "gap" between the current HEAD of the master branch and the fix is 20 31 commits. I'll see if I can rebase it.

@oliverschmidt
Copy link
Contributor

If you plan to close the gap in the foreseeable future, I personally don't see why you should invest into changing the order of your commits just because of the existence of this issue.

@acqn
Copy link
Contributor

acqn commented Sep 5, 2020

I've been closing the gap. There are still (3 + 1) + 21 = 25 commits to go before the fix branch could be merged and tested. That would be the 2 current PRs plus about 6 new ones.

@greg-king5
Copy link
Contributor

I've been closing the gap. There are still (3 + 1) + 21 = 25 commits to go before the fix branch could be merged and tested. That would be the 2 current PRs plus about 6 new ones.

We "nosy bees" appreciate the progress report.

@oliverschmidt
Copy link
Contributor

+1

@mrdudz
Copy link
Contributor

mrdudz commented Sep 6, 2020

yes! I am totally overwhelmed by how it is moving forward atm. Don't stop :)

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

6 participants