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

arm/arch: using __builtin_frame_address(0) implement up_getsp(). #6079

Merged
merged 1 commit into from Apr 17, 2022

Conversation

CV-Bowen
Copy link
Contributor

Summary

using compile builtin function __builtin_frame_address(0) to implement the up_getsp()

Test

stm32f7

Signed-off-by: wangbowen6 <wangbowen6@xiaomi.com>
@CV-Bowen
Copy link
Contributor Author

@xiaoxiang781216

@pkarashchenko
Copy link
Contributor

This seems to be GCC specific implementation.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Apr 15, 2022

This seems to be GCC specific implementation.

Yes, but __asm__ keyword also GCC specific too:), not worse than before.

@pkarashchenko
Copy link
Contributor

This seems to be GCC specific implementation.

Yes, but __asm__ keyword also GCC specific too:), not worse than before.

Yeah, most of other compilers have __asm or asm and we do not have inline assembly compatibility layer in compiler.n

@pkarashchenko
Copy link
Contributor

I mean what is the benefit of this change? Is it clang related or current implementation brings some issues?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Apr 15, 2022

This seems to be GCC specific implementation.

Yes, but __asm__ keyword also GCC specific too:), not worse than before.

Yeah, most of other compilers have __asm or asm and we do not have inline assembly compatibility layer in compiler.n

Yes, but the assembly grammar is dramatically different, and is hard to share the same assembly code between different compiler.

I mean what is the benefit of this change? Is it clang related or current implementation brings some issues?

Not relate to clang(has the same support of __asm__ and __builtin_frame_address). We have a very special chip which very like old ARM arch, but has the different assembly grammar, the inline assembly function make the replacement is very hard.

@CV-Bowen
Copy link
Contributor Author

I'd like to notice that the the new chip can not use the arm common context switch code because the different assembly grammar. The reason we want to put this chip in arch/arm is most of the common code can be shared.

@pkarashchenko
Copy link
Contributor

Yeah. Also __builtin_frame_address(0) can generate different code from mov %0, sp. I just tried to compile a sample code and got mov r0, fp, so it depends of optimisation options. But in general I agree that behaviour should be the same.

@pkarashchenko
Copy link
Contributor

I'd like to notice that the the new chip can not use the arm common context switch code because the different assembly grammar. The reason we want to put this chip in arch/arm is most of the common code can be shared.

Maybe a solution can be to create a new arch folder and symlink the common files instead of modifying ARM to fit non-ARM?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Apr 16, 2022

I'd like to notice that the the new chip can not use the arm common context switch code because the different assembly grammar. The reason we want to put this chip in arch/arm is most of the common code can be shared.

Maybe a solution can be to create a new arch folder

It's too heavy to create new arch for a specific chip which share many common implementation with arch/arm

and symlink the common files instead of modifying ARM to fit non-ARM?

There is no symlink in nuttx git repo, only the build script create some symlink dynamically, so:

  1. It's strange to commit the symlink to repo
  2. It isn't friend for Windows(build script already has many hack to emulate symlink on Windows, but it's hard to hack git and Windows).

BTW, this isn't the first time that arch layer use the toolchain specific function. For example, stdarg do the same thing here:
https://github.com/apache/incubator-nuttx/blob/master/arch/arm/include/stdarg.h#L34-L37

@pkarashchenko
Copy link
Contributor

Ok. Let's proceed with this pr

@xiaoxiang781216 xiaoxiang781216 merged commit 91d02f5 into apache:master Apr 17, 2022
@CV-Bowen CV-Bowen deleted the arm_getsp_pr branch June 14, 2022 02:06
@jerpelea jerpelea added this to To-Add in Release Notes - 11.0.0 Aug 30, 2022
@jerpelea jerpelea moved this from To-Add to Added in Release Notes - 11.0.0 Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants