Skip to content

Track additional actions for inline ICACHE_RAM_ATTR + compiler optimizations case #6900

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

Closed
6 tasks done
devyte opened this issue Dec 12, 2019 · 5 comments
Closed
6 tasks done

Comments

@devyte
Copy link
Collaborator

devyte commented Dec 12, 2019

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Core Version: 2.6.x +

Problem Description

Related: #6898 and #6894

Ref: I2C stability

Per discussions with @Tech-TX and @earlephilhower, it looks like inline ICACHE_RAM_ATTR functions in conjunction with compiler optimizations can cause crashes. Specifically, the current state (prior to either of the two PRs above) seems to be that the compiler decides to break up Twi::reply() into parts (ref here ) , and then the attribute doesn't get propagated to all the parts correctly. Of course, any part of reply() that is not in IRAM can cause a crash.

PR #6898 addresses the immediate issue by removing inline from the function declaration. However, the deeper issue is that any such inline ICACHE_RAM_ATTR function anywhere in the code could run into the same optimization.

This issue is meant to track a more thorough solution. Specifically:
A. retest inline ICACHE_RAM_ATTR Twi::reply() with gcc 9.x (PR #6294 )
B. If A still shows the iissue, look into disabling the specific optimization with -fdisable-ipa-fnsplit (flag isn't available in gcc 4.8.x)
C. If B doesn't work, document that inline ICACHE_RAM_ATTR is undefined behavior and can cause a crash.

@earlephilhower
Copy link
Collaborator

https://unix.stackexchange.com/questions/223013/function-symbol-gets-part-suffix-after-compilation

Underlying issue is that a GCC optimization used for a small number of inline functions ends up generating another pseudo-function. The pseudo-function (named XXXXX.part.XXX) doesn't preserve the attributes (segment) of the original function.

Thinking about this, I have a sneaky suspicion that this is caused by calling a inline function in both an IRAM function and one on flash. When you inline a function, only the segment of the calling fcn can be preserved, of course, so maybe in this case, TWI::reply(), a call from a non-IRAM routine happened before a call from an IRAM one, so the pre-generated xxx.part.2 was stuck with that flash segment...

@earlephilhower
Copy link
Collaborator

A simple workaround, and a way of making core code simpler, would simply specify that any function with __iram__ in its name will be placed in .text by the linker. GCC segments and ICACHE_RAM_ATTR no longer needed for functions to be in IRAM.

@earlephilhower earlephilhower modified the milestones: 3.0.0, 3.0.1 Mar 31, 2021
@earlephilhower
Copy link
Collaborator

I think this could be a non-breaking change in 3.0.1 by retaining the IRAM_ATTR macros, but moving our own stuff to the iram naming. However, I don't think we have consensus yet so don't think it should be put in just yet.

@d-a-v d-a-v modified the milestones: 3.0.1, 3.1 Jun 16, 2021
@mcspr
Copy link
Collaborator

mcspr commented Feb 16, 2022

Seems to be fixed some time in during GCC 9 release?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63560
gcc-mirror/gcc@3755592

@earlephilhower
Copy link
Collaborator

Yes, this was fixed in 3.0.0 with GCC 10.3:
https://github.com/gcc-mirror/gcc/blob/f00b5710a30f22efc3171c393e56aeb335c3cd39/gcc/ipa-split.c#L1781-L1793

Closing as fixed with toolchain rev.

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

4 participants