Bug 1197 - `fallthrough` in parser is incorrect
Summary: `fallthrough` in parser is incorrect
Status: CONFIRMED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- major
Assignee: Jacob Lifshay
URL:
Depends on:
Blocks:
 
Reported: 2023-11-01 22:47 GMT by Jacob Lifshay
Modified: 2023-11-05 02:05 GMT (History)
3 users (show)

See Also:
NLnet milestone: ---
total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0
parent task for budget allocation:
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Lifshay 2023-11-01 22:47:41 GMT
from reading through all occurrences of `switch` in Power ISA v3.1B (there aren't many), I think our parser incorrectly implements it:
mtspr:
    switch (n)
      case(129): see(Book_III_p975)
      case(808, 809, 810, 811):
      default:
        stuff-here

our parser converts that to:
    switch (n)
      case(129): see(Book_III_p975)
      case(808, 809, 810, 811): fallthrough
      default:
        stuff-here

which it then interprets (if it successfully detected the fallthrough keyword instead of trying to call `self.fallthrough()`) as:
    if n == 129:
        see(Book_III_p975)
    elif n in [808, 809, 810, 811] or True:  # True for default
        stuff-here

which is *wrong*. mtspr's description says if n is 808, 809, 810, or 811, then it *does nothing*, not tries to run the default case. This also matches the description of how `switch` is supposed to work, which has no falling-through (so like VB's Select, not like C's switch).

so, in other words, it should be:
    if n == 129:
        see(Book_III_p975)
    elif n in [808, 809, 810, 811]:
        pass  # not fallthrough
    else:
        stuff-here

I propose we remove all `fallthrough` logic completely and change p_switch_case to make `suite` optional, replacing omitted `suite` with `pass`:
(untested code, but should work)

    def p_switch_case(self, p):
        """switch_case : CASE LPAR atomlist RPAR COLON
                       | CASE LPAR atomlist RPAR COLON suite
        """
        if len(p) == 6:
            suite = [ast.Pass()]
        else:
            suite = p[6]
        p[0] = (p[3], suite)
Comment 1 Jacob Lifshay 2023-11-01 22:53:48 GMT
also, we should be using the mtspr and mfspr pseudo-code from Book III, since it is complete
Comment 2 Luke Kenneth Casson Leighton 2023-11-02 07:56:20 GMT
(In reply to Jacob Lifshay from comment #0)
> from reading through all occurrences of `switch` in Power ISA v3.1B (there
> aren't many), I think our parser incorrectly implements it:
> mtspr:
>     switch (n)
>       case(129): see(Book_III_p975)
>       case(808, 809, 810, 811):
>       default:
>         stuff-here

we'll have to check the spec, see what is intended, first.
Comment 3 Jacob Lifshay 2023-11-05 02:05:32 GMT
(In reply to Luke Kenneth Casson Leighton from comment #2)
> we'll have to check the spec, see what is intended, first.

I did:
(In reply to Jacob Lifshay from comment #0)
> This also matches
> the description of how `switch` is supposed to work, which has no
> falling-through (so like VB's Select, not like C's switch).