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

fix raise_exception() #6288

Merged
merged 11 commits into from
Jul 15, 2019
Merged

fix raise_exception() #6288

merged 11 commits into from
Jul 15, 2019

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Jul 11, 2019

This is a proposal

It seems this has no visible effect:

__asm__ __volatile__ ("syscall"); // triggers gdb or does nothing

On panic/abort/assert called from user context, WDT happens due to the following while(1); and finally produces an exception with the right message (after the WDT reason and delay).
On ISR context, only the WDT reason is visible.

This commit permits to avoid WDT and show the right message from both user or ISR context.

Before #4482, *((int*)0)=0 was used to trigger an hardware exception (similar to the WDT exception we have now, also showing an additional message unrelated to the real cause).

On real hardware exception, raise_exception() is not called.

fixes #6283 (thanks @mhightower83)

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jul 12, 2019

This is tested with gdb:

  • user exception in user context:
    gdb doesn't show the exact line,
    but that kind of exception is anyway displayed on console (panic/abort/assert)
  • hardware exception in user context - ok
  • user exception in sys context - ok
  • hardware exception in sys context - ok

So there is room for improvement on gdb side (I'm not a specialist)

@d-a-v d-a-v changed the title (WIP) fix raise_exception() fix raise_exception() Jul 12, 2019
@earlephilhower
Copy link
Collaborator

I think that's exactly as it was for GDB. On assert/panic/etc. you get a breakpoint in the raise_exception() call and need to "up" one or two levels to get to the actual line of code (but it's still there in the stack so it's fully functional).

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Minor cleanings only. Looks good logically.

cores/esp8266/core_esp8266_postmortem.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_postmortem.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_postmortem.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_postmortem.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM now.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Overall, I'm ok with the approach, pending my minor comments. In essence, if there is a code path that calls raise_exception(), then set a global flag before manually calling __real_system_restart_local(), then interpret that flag first.
Of course, if syscall were to actually work as it's supposed to, it would be much better and standard, but I have no idea how to do that.
And great work figuring out this solution!

cores/esp8266/core_esp8266_postmortem.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_postmortem.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_postmortem.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Nothing jumps out at me.
Out of curiosity, does this work correctly with exceptions enabled? (after they get fixed again)

@earlephilhower
Copy link
Collaborator

It should work w/C++ exceptions, should they get fixed at some point. Exceptions don't involve this bit at all unless they're uncaught, at which point panic() is called and an exception reason printed (__unhandled_exception).

@earlephilhower earlephilhower merged commit 3cc64f7 into esp8266:master Jul 15, 2019
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.

panic() called from ISR causes Hardware WDT
4 participants