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

[Bug]: Invalid inline asm for ARM32 #8523

Closed
ribes96 opened this issue Mar 3, 2025 · 5 comments · Fixed by #8527
Closed

[Bug]: Invalid inline asm for ARM32 #8523

ribes96 opened this issue Mar 3, 2025 · 5 comments · Fixed by #8527
Assignees
Labels

Comments

@ribes96
Copy link

ribes96 commented Mar 3, 2025

Contact Details

[email protected]

Version

5.7.6-stable

Description

I am compiling for ARM32 using gcc 4.3.3

I run configure like this

./configure --host=arm-linux

At the end of the configure there is this line

---
./configure flags: '--host=arm-linux' 'host_alias=arm-linux' 'CC=arm-unknown-linux-gnueabi-gcc' 'CFLAGS=-march=armv5te' 'CPP=arm-unknown-linux-gnueabi-cpp'
---

When running make, it complains with this

wolfcrypt/src/sp_int.c: In function '_sp_mul_d':
wolfcrypt/src/sp_int.c:6561: error: expected string literal before ')' token

The relevant line is this

        /* Multiply and add into low and high from previous result.
         * No overflow of possible with add. */
        SP_ASM_MUL_ADD_NO(l, h, a->dp[i], d);

And for my particular build, the macro SP_ASM_MUL_ADD_NO uses this expansion

/* Multiply va by vb and add double size result into: vh | vl */
#define SP_ASM_MUL_ADD_NO(vl, vh, va, vb)                \
    __asm__ __volatile__ (                               \
        "umlal	%[l], %[h], %[a], %[b]	\n\t"            \
        : [l] "+r" (vl), [h] "+r" (vh)                   \
        : [a] "r" (va), [b] "r" (vb)                     \
        :                                                \
    )

(this is wolfcrypt/src/sp_int.c:1161)

I am not very familiar with inline assembly, but some googling shows that using cc for the clobbers would help

/* Multiply va by vb and add double size result into: vh | vl */
#define SP_ASM_MUL_ADD_NO(vl, vh, va, vb)                \
    __asm__ __volatile__ (                               \
        "umlal	%[l], %[h], %[a], %[b]	\n\t"            \
        : [l] "+r" (vl), [h] "+r" (vh)                   \
        : [a] "r" (va), [b] "r" (vb)                     \
        : "cc"                                           \
    )

Indeed, when I add the cc the code does compile fine.

In my case, I also had to fix macro SP_ASM_MUL_SET

/* Multiply va by vb and store double size result in: vo | vh | vl */
#define SP_ASM_MUL_SET(vl, vh, vo, va, vb)               \
    __asm__ __volatile__ (                               \
        "umull	%[l], %[h], %[a], %[b]	\n\t"            \
        "mov	%[o], #0		\n\t"            \
        : [l] "+r" (vl), [h] "+r" (vh), [o] "=r" (vo)    \
        : [a] "r" (va), [b] "r" (vb)                     \
        : "cc"                                           \
    )

(this is wolfcrypt/src/sp_int.c:1141)

I'm not sure if this is a problem only for old versions of gcc, or if the code is indeed wrong. If the problem is in the code, maybe the fix should be applied also to other parts that I don't happen to catch because my settings don't compile it

Reproduction steps

No response

Relevant log output

@ribes96 ribes96 added the bug label Mar 3, 2025
@SparkiDev
Copy link
Contributor

Hi @ribes96,

As the author of this code I feel most comfortable responding to this.
The instruction umlal does not change the flags.
The instruction umlals will though.
See: https://developer.arm.com/documentation/dui0379/e/arm-and-thumb-instructions/umlal

Can you tell me a little about your project?
Why do you need to use gcc.4.3.3? This is a very old version of the compiler.

Thanks,
Sean

@SparkiDev SparkiDev self-assigned this Mar 3, 2025
@ribes96
Copy link
Author

ribes96 commented Mar 3, 2025

The compiler is also happy if I just remove the last line of the macro

/* Multiply va by vb and add double size result into: vh | vl */
#define SP_ASM_MUL_ADD_NO(vl, vh, va, vb)                \
    __asm__ __volatile__ (                               \
        "umlal	%[l], %[h], %[a], %[b]	\n\t"            \
        : [l] "+r" (vl), [h] "+r" (vh)                   \
        : [a] "r" (va), [b] "r" (vb)                     \
    )

so maybe this is the solution if, as you say, the instruction doesn't change the flags. As I said, I am not very familiar with inline assembly or the ARM instruction set in general.

4.3.3 happens to be the version of the toolchain I have for my old board. I know it's old, but in general it gets the job done, and I'm not sure I would be able to build a new toolchain. I understand it if this compiler version is not supported, but the fix seems trivial.

@SparkiDev
Copy link
Contributor

Hi @ribes96,

It can become a bit of 'whack-a-mole' situation with these type of compile errors.
One compiler complains, you make a fix and another compiler and/or version complains about the fix.
We'll see how this changed goes.

Let me know if you can compile now.

Sean

@ribes96
Copy link
Author

ribes96 commented Mar 4, 2025

I can compile now
Thanks

@SparkiDev
Copy link
Contributor

Hi @ribes96

I will close this ticket as the reported issue has been fixed.

Thanks for taking the time to report the bug!

Sean :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants