Skip to content

Add some NORETURN attributes #3469

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

Merged
merged 3 commits into from
Oct 1, 2020
Merged

Add some NORETURN attributes #3469

merged 3 commits into from
Oct 1, 2020

Conversation

jepler
Copy link

@jepler jepler commented Sep 25, 2020

I have (or will have, in stm32 canbus) a function where it should be impossible to reach the end, so I put in a safe-mode reset at the bottom:

int find_unused_slot(void) {
    // precondition: you already verified that a slot was available
    assert(available_slot());
    for (int i=0; i<NUM_SLOTS; i++) {
        if (slot_free(i)) {
            return i;
        }
    }
    safe_mode_reset(MICROPY_FATAL_ERROR);
}

However, the compiler still gave a diagnostic, because safe_mode_reset was not declared NORETURN.

So I started by teaching the compiler that reset_into_safe_mode never returned. This leads at least one level deeper due to reset_cpu needing to be a NORETURN function. Each port is a little different in this area. I also marked reset_to_bootloader as NORETURN.
Additional notes:

  • stm32's reset_to_bootloader was not implemented, but now does a bare reset. Most stm32s are not fitted with uf2 bootloaders anyway.
  • ditto cxd56
  • esp32s2 did not implement reset_cpu at all. I used esp_restart(). (not tested)
  • litex did not implement reset_cpu at all. I used reboot_ctrl_write. But notably this is what reset_to_bootloader already did, so one or the other must be incorrect (not tested). reboot_ctrl_write cannot be declared NORETURN, as it returns unless the special value 0xac is written), so a new unreachable forever-loop is added.
  • cxd56's reset is via a boardctl() call which can't generically be declared NORETURN, so a new unreacahble "for(;;)" forever-loop is added.
  • In several places, NVIC_SystemReset is redeclared with NORETURN applied. This is accepted just fine by gcc. I chose this as preferable to editing the multiple copies of CMSIS headers where it is normally declared.
  • the stub safe_mode reset simply aborts. This is used in mpy-cross.

@jepler jepler mentioned this pull request Sep 25, 2020
tannewt
tannewt previously approved these changes Sep 25, 2020
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you!

@tannewt
Copy link
Member

tannewt commented Sep 25, 2020

Looks like iMX RT and ESP32-S2 builds are broken.

@tannewt
Copy link
Member

tannewt commented Sep 28, 2020

Please resolve the conflict. Looks good otherwise.

I have a function where it should be impossible to reach the end, so I put in a safe-mode reset at the bottom:
```
int find_unused_slot(void) {
    // precondition: you already verified that a slot was available
    for (int i=0; i<NUM_SLOTS; i++) {
        if( slot_free(i)) {
            return i;
        }
    }
    safe_mode_reset(MICROPY_FATAL_ERROR);
}
```
However, the compiler still gave a diagnostic, because safe_mode_reset was not declared NORETURN.

So I started by teaching the compiler that reset_into_safe_mode never returned.  This leads at least one level deeper due to reset_cpu needing to be a NORETURN function.  Each port is a little different in this area.  I also marked reset_to_bootloader as NORETURN.
Additional notes:

 * stm32's reset_to_bootloader was not implemented, but now does a bare reset.  Most stm32s are not fitted with uf2 bootloaders anyway.
 * ditto cxd56
 * esp32s2 did not implement reset_cpu at all.  I used esp_restart().  (not tested)
 * litex did not implement reset_cpu at all.  I used reboot_ctrl_write.  But notably this is what reset_to_bootloader already did, so one or the other must be incorrect (not tested).  reboot_ctrl_write cannot be declared NORETURN, as it returns unless the special value 0xac is written), so a new unreachable forever-loop is added.
 * cxd56's reset is via a boardctl() call which can't generically be declared NORETURN, so a new unreacahble "for(;;)" forever-loop is added.
 * In several places, NVIC_SystemReset is redeclared with NORETURN applied.  This is accepted just fine by gcc.  I chose this as preferable to editing the multiple copies of CMSIS headers where it is normally declared.
 * the stub safe_mode reset simply aborts.  This is used in mpy-cross.
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@tannewt tannewt merged commit d62ac24 into adafruit:main Oct 1, 2020
@jepler jepler deleted the noreturn branch November 3, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants