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

py: Fix compilation of persistentcode with uncommon configs #4753

Closed
wants to merge 2 commits into from

Conversation

quark-zju
Copy link
Contributor

With the following configuration:

#define MICROPY_PERSISTENT_CODE_SAVE (1)
#define MICROPY_PERSISTENT_CODE_LOAD (1)

The code fails to compile:

persistentcode.c: In function 'load_raw_code':
persistentcode.c:457:13: error: 'n_obj' undeclared
         n_obj, n_raw_code,

If MICROPY_EMIT_NATIVE disabled, there are more errors due to the use of
undefined fields in mp_raw_code_t.

This patch fixes compilation by avoiding undefined fields.

py/persistentcode.c Show resolved Hide resolved
py/persistentcode.c Show resolved Hide resolved
There are a lot of places testing MICROPY_EMIT_NATIVE || MICROPY_EMIT_INLINE_ASM.

Define a new macro for it.
With the following configuration:

    #define MICROPY_PERSISTENT_CODE_SAVE (1)
    #define MICROPY_PERSISTENT_CODE_LOAD (1)

The code fails to compile:

    persistentcode.c: In function 'load_raw_code':
    persistentcode.c:457:13: error: 'n_obj' undeclared
             n_obj, n_raw_code,

If MICROPY_EMIT_NATIVE disabled, there are more errors due to the use of
undefined fields in mp_raw_code_t.

This patch fixes compilation by avoiding undefined fields.

MICROPY_EMIT_NATIVE was changed to MICROPY_EMIT_ANY_ASM in this file to
match mp_raw_code_t definition.
@@ -329,6 +329,9 @@
// Convenience definition for whether any inline assembler emitter is enabled
#define MICROPY_EMIT_INLINE_ASM (MICROPY_EMIT_INLINE_THUMB || MICROPY_EMIT_INLINE_XTENSA)

// Convenience definition for whether any native or inline assembler emitter is enabled
#define MICROPY_EMIT_ANY_ASM (MICROPY_EMIT_NATIVE || MICROPY_EMIT_INLINE_ASM)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's probably a good idea to make this define. But, I'm just thinking if there's a better name for it, because "any asm" could be mistaken for meaning "any inline asm" (which is already what MICROPY_EMIT_INLINE_ASM is for).

Maybe MICROPY_EMIT_MACHINE_CODE?

@dpgeorge
Copy link
Member

@quark-zju I'm happy to make the final changes (rename to MICROPY_EMIT_MACHINE_CODE) and merge if you aren't able to.

@pmp-p
Copy link
Contributor

pmp-p commented Jun 25, 2019

MICROPY_EMIT_MACHINE_CODE options seems a very good way to open doors to the new WASM inlining possibility javascript port can offer and its name is generic enough for future use. it would also help to formulate PR for javascript port more early.

@dpgeorge
Copy link
Member

Ok, I renamed MICROPY_EMIT_ANY_ASM to MICROPY_EMIT_MACHINE_CODE and merged in b152bbd and d165a40

@dpgeorge dpgeorge closed this Jun 28, 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.

None yet

3 participants