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

Use returns_twice attribute to preserve regs in nlrthumb nlr_push() #7368

Merged
merged 1 commit into from Dec 21, 2022

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Dec 21, 2022

This program crashed on SAMD51 when compiled with DEBUG=1:

def crash():
    try:
        None.wrong()
    except AttributeError:
        pass
    i = None

The stack variable fastn in mp_execute_bytecode() in py/vm.c was getting smashed. It was a Heisenbug because adding print statements caused it to work.

After narrowing down the point of smashing, I looked at the assembly code and saw that a call to nlr_push() in mp_execute_bytecode assumed that register r3 was preserved, when it was not. nlr_push() is a "naked" function that does not have the usual prologue and postlogue.

This all seemed more and more familiar as I progressed, and it turns out I fixed this problem already, about five years ago, in #506! However, the fix got undone during an upstream MicroPython merge in 2021. The problem manifested differently and we didn't see this problem again for a while.

My #506 fix involved adding "don't clobber these registers" indicators to nlr_push(). There is a long discussion in the related issue, #500. A MicroPython developer discovered the same problem in a different context, but it never really got fixed in MicroPython, because it occurs due to LTO, and they rarely use LTO. See micropython#3889 (which was closed and not merged).

The fix proposed in micropython#3889 is to declare nlr_push() with the attribute returns_twice. That is simpler than my #506 fix, so I tried that this time, and lo and behold, it works.

Note that this does not solve #7279 (-O2 not working on SAMx5x).

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

This makes sense, thanks

@dhalbert dhalbert merged commit 2f14768 into adafruit:main Dec 21, 2022
@dhalbert dhalbert deleted the preserve-nlr_push-regs branch December 21, 2022 13:40
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.

HardFault with DEBUG build on Metro M4 due to caught AttributeError
2 participants