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

msp430x5xx: Add fix for possible bug in msp430-elf-gcc 9.3.0. #1481

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

cr1901
Copy link
Collaborator

@cr1901 cr1901 commented May 30, 2022

Describe the PR
Consider the following contrived MSP430 code and linker script:

#include <stdint.h>

extern volatile unsigned int USBVECINT; // PROVIDE(USBVECINT = 0x0932);

int main()
{
  uint16_t curr_vector = USBVECINT;
  volatile uint8_t ep; // Call into function in another TU has same effect.

  switch(curr_vector)
  {
    case 2:
    case 1:
      ep = curr_vector;
  }

  return 0;
}
OUTPUT_ARCH(msp430)

MEMORY {
  ROM (rx)         : ORIGIN = 0x4400, LENGTH = 0xBB80 /* END=0xFF7F, size 48000 */
}

SECTIONS {
  .text :
  {
    KEEP (*(.text))
  } > ROM
}

PROVIDE(USBVECINT = 0x0932);

As of msp430-elf-gcc version 9.3.0, this produces the following code (on any opt-level besides -O0) that does two reads from volatile variable USBVECINT and then uses the second read for the remainder of main:

msp430-elf-gcc -I. -Tlink.ld -Og -o main.elf main.c
msp430-elf-objdump --disassemble=main main.elf

main.elf:     file format elf32-msp430


Disassembly of section .text:

00004400 <main>:
    4400:       21 83           decd    r1              ;
    4402:       1d 42 32 09     mov     &0x0932,r13     ;0x0932
    4406:       1c 42 32 09     mov     &0x0932,r12     ;0x0932
    440a:       3c 53           add     #-1,    r12     ;r3 As==11
    440c:       5e 43           mov.b   #1,     r14     ;r3 As==01
    440e:       0e 9c           cmp     r12,    r14     ;
    4410:       03 2c           jc      $+8             ;abs 0x4418

00004412 <.L2>:
    4412:       4c 43           clr.b   r12             ;
    4414:       21 53           incd    r1              ;
    4416:       30 41           ret

00004418 <.L3>:
    4418:       c1 4d 01 00     mov.b   r13,    1(r1)   ;
    441c:       fa 3f           jmp     $-10            ;abs 0x4412
    441e:

In a real tinyusb application, the second read of USBVECINT will produce a garbage value that causes the application to infinitely loop in the catch-all while(true) case of the MSP430 USB interrupt handler. This PR uses an inline asm statement to suppress the undesired optimization for affected compiler versions, while leaving older versions alone. 9.2.0 works fine with the original code from my testing. I may revisit this PR with a better solution later if I can find one.

Additional Context
Fixed Sample Code (applied to TinyUSB)

#include <stdint.h>

extern volatile unsigned int USBVECINT; // PROVIDE(USBVECINT = 0x0932);

int main()
{
  uint16_t curr_vector;
  volatile uint8_t ep; // Call into function in another TU has same effect.

  #if __GNUC__ > 9 || (__GNUC__ == 9 && __GNUC_MINOR__ > 2)
    asm volatile ("mov %1, %0"
                  : "=r" (curr_vector)
                  : "m" (USBVECINT));
  #else
    curr_vector = USBVECINT;
  #endif

  switch(curr_vector)
  {
    case 2:
    case 1:
      ep = curr_vector;
  }

  return 0;
}
msp430-elf-gcc -I. -Tlink.ld -Og -o main.elf main.c
msp430-elf-objdump --disassemble=main main.elf

main.elf:     file format elf32-msp430


Disassembly of section .text:

00004400 <main>:
    4400:       21 83           decd    r1              ;
    4402:       1c 42 32 09     mov     &0x0932,r12     ;0x0932
    4406:       0d 4c           mov     r12,    r13     ;
    4408:       3d 53           add     #-1,    r13     ;r3 As==11
    440a:       5e 43           mov.b   #1,     r14     ;r3 As==01
    440c:       0e 9d           cmp     r13,    r14     ;
    440e:       03 2c           jc      $+8             ;abs 0x4416

00004410 <.L2>:
    4410:       4c 43           clr.b   r12             ;
    4412:       21 53           incd    r1              ;
    4414:       30 41           ret

00004416 <.L3>:
    4416:       c1 4c 01 00     mov.b   r12,    1(r1)   ;
    441a:       fa 3f           jmp     $-10            ;abs 0x4410
    441c:

Compiler Version

msp430-elf-gcc -v
Using built-in specs.
COLLECT_GCC=C:\msys64\opt\toolchains\bin\msp430-elf-gcc.exe
COLLECT_LTO_WRAPPER=c:/msys64/opt/toolchains/bin/../libexec/gcc/msp430-elf/9.3.1/lto-wrapper.exe
Target: msp430-elf
Configured with: ../../gcc/configure --target=msp430-elf --enable-languages=c,c++ --disable-nls --enable-initfini-array --build=x86_64-pc-linux-gnu --host=x86_64-w64-mingw32 --enable-target-optspace --enable-newlib-nano-formatted-io --with-pkgversion='Mitto Systems Limited - msp430-gcc 9.3.1.11'
Thread model: single
gcc version 9.3.1 (Mitto Systems Limited - msp430-gcc 9.3.1.11)

@hathach
Copy link
Owner

hathach commented Jun 1, 2022

thank you for the fix, it is a bit awkward that later version of msp gcc generate 2 movs for no real reasons. There maybe other register with the same symptom. Btw could you try to introduce the const keyword to see if that help to fix the compiler, if not then we will merge this as it is and come back later on with better fix (if any)

const uint16_t curr_vector =  USBVECINT;

@cr1901
Copy link
Collaborator Author

cr1901 commented Jun 6, 2022

Btw could you try to introduce the const keyword to see if that help to fix the compiler

The problem still remains if I introduce the const keyword; the fix in this PR seems to be the simplest workaround for now until I can figure out how to report this to the vendor.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

thanks for testing out the 'const' keyword, this is bizarre issue. Hopefully TI will fix this in future gcc

@hathach hathach merged commit afd9b18 into hathach:master Jun 6, 2022
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.

2 participants