-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
emitnative: Make ARM codegen compile after full-arg support refactors. #1241
Conversation
@Vogtinator: It would be nice if looked into this. @dpgeorge: Maybe you'd know how to quickly fix it, based on transformations you applied to other archs in mentioned commit. Also let me know if you agree this should be catchable by Travis (note that for example regarding stackless, I handle that separately in https://github.com/pfalcon/micropython/tree/stackless-travis , not not burden common CI). |
I didn't have the time to fix all native emitters, and thought it better to actually push some code rather than leave it forever :) I don't currently have a way of testing the ARM emitter; how do you do it? |
@@ -108,10 +109,10 @@ void asm_arm_lsl_reg_reg(asm_arm_t *as, uint rd, uint rs); | |||
void asm_arm_asr_reg_reg(asm_arm_t *as, uint rd, uint rs); | |||
|
|||
// memory | |||
void asm_arm_ldr_reg_reg(asm_arm_t *as, uint rd, uint rn); | |||
void asm_arm_ldr_reg_reg(asm_arm_t *as, uint rd, uint rn, uint off); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably off
is a word offset so needs shifting right by 4.
Sure, supporting all arg variety was a great addition for native emitter. I popped up Samsung ARM Chromebook after not using it for a while, and tried to build uPy. It crashes, and running thru gdb, control is passed to something which doesn't look like valid ARM code. |
mp_setup_code_state takes 5 args so need special handling on thumb/arm (since 5th arg goes on stack). Therefore you need to duplicate lines 750-752 (of file before this patch) for the N_ARM case. |
Thanks for the hint, doesn't seem to help, behavior is the same as before.
|
What actually gets called:
|
So, there's some pretty weird mess. Code actually thumb2, but appears to be called as arm, and it somehow thinks that it's arm mode (though it's ubuntu armhf, and i'd think it'd use thumb2). So, looks some kind of miss-detection and mix up. (It all used to work of course.)
|
MICROPY_MAKE_POINTER_CALLABLE(p) should be setting the low bit in order to jump to Thumb2 code; copy the relevant line from stmhal/mpconfigport.h. |
Well, it detected both arm and thumb2. they should be prioritized, and yep, MICROPY_MAKE_POINTER_CALLABLE(p) for thumb was missing. |
Or how case of multiple emitters should be handled? (Taking into account that MICROPY_MAKE_POINTER_CALLABLE is global.) |
The code was apparently broken after 9988618 "py: Implement full func arg passing for native emitter.". This attempts to propagate those changes to ARM emitter.
Make thumb2 have priority over arm.
e877e30
to
bb53f34
Compare
Updated patch, all native tests pass. Few viper ones fail though. |
Merged in 351424e. |
fix tinyusb cdc issue
The code was apparently broken after 9988618
"py: Implement full func arg passing for native emitter.". However,
while these changes make it compile, it still crashes at runtime and need
more work.