4ms / metamodule

MetaModule virtual patch module firmware
Other
8 stars 0 forks source link

Multiple compiler errors when enabling ISR in A7 #163

Closed LnnrtS closed 10 months ago

LnnrtS commented 10 months ago

This all for aeeec7c6faaccdbd9494d81ea1201fa4bfbd7e1f with make configure PRESET=full

First I get multiple ODR warnings when linking

[65/65] Linking CXX executable mp1corea7/medium/main.elf
/home/user/Code/metamodule/firmware/../shared/patch_convert/ryml/rapidyaml/src/c4/yml/common.hpp:164:20: warning: type 'struct Callbacks' violates the C++ One Definition Rule [-Wodr]
  164 | struct RYML_EXPORT Callbacks
      |                    ^
/home/user/Code/metamodule/firmware/../shared/patch_convert/ryml/rapidyaml/src/c4/yml/common.hpp:164:20: note: a different type is defined in another translation unit
  164 | struct RYML_EXPORT Callbacks
      |                    ^
/home/user/Code/metamodule/firmware/../shared/patch_convert/ryml/rapidyaml/src/c4/yml/common.hpp:167:18: note: the first difference of corresponding definitions is field 'm_allocate'
  167 |     pfn_allocate m_allocate;
      |                  ^
/home/user/Code/metamodule/firmware/../shared/patch_convert/ryml/rapidyaml/src/c4/yml/common.hpp:167:18: note: a field of same name but different type is defined in another translation unit
  167 |     pfn_allocate m_allocate;
...

This can be removed by disabling LFO (but it's just a warning anyway)

I also get this

/tmp/cceuprik.s: Assembler messages:
/tmp/cceuprik.s:1641: Error: r13 not allowed here -- `and r2,sp,#4'

Which seems to originate from aligning the stack pointer in IRQ_Handler in mdrivlib. Looking around the internet indicates that this operation is not allowed for thumb code.I haven't found a proof in the documentation though. But I can make this go away by compiling the inline assembly in arm mode (adding .arm as the first line). Still unclear why this appears only sometimes.

With adding .arm or even removing the alignment I can compile but the code hangs when the ISR required for the usart for talking to the boot rom fires (commenting out LL_USART_EnableIT_RXNE_RXFNE in BufferedUSART::initPeripheral() fixes it at the expense of no received answer from the boot rom)

LnnrtS commented 10 months ago

Tried this simple IRQ handler but doesn't work either.

Instead I get warning: FP registers might be clobbered despite 'interrupt' attribute: compile with '-mgeneral-regs-only' which basically means that we are responsible for saving/restoring registers on armv7-A (and this handler doesn't do it).

danngreen commented 10 months ago

On other commits (main, for example), the IRQ_Handler assembly is arm, not thumb. Otherwise, it would never compile on any branch (but I had to check the disassembly to be sure!)

Even if I add the .arm directive to get it to compile, and turn off all LTO flags, the compiler is still confused about arm/thumb. The compiler has replaced the IRQ_Handler call in the vector table with a call to a generated arm-to-thumb interwork shim:

0xC2000058     EA0521FA            b       0xC2148848       ; __IRQ_Handler_from_arm
0xC200005C     EA052221            b       0xC21488E8       ; __FIQ_Handler_from_arm

And then here's the shim code:

E51FF004  __IRQ_Handler_from_arm:    ldr     pc,0xC214884C    ; pc,=IRQ_Handler
C2000211                             dcd     0xC2000211       ; IRQ_Handler

The thumb bit is set in the address, therefore it's trying to execute the IRQ_Handler as thumb code. So, it crashes. Obviously, none of this is present in other branches (Edit: or even on this commit but with --preset base). Could it be a new compile or link flag introduced?

Does the previous commit work for you? I'm not able to get it to compile (I think I need to configure it for MD5 support, didn't look into it much).

The ODR violations could be from our partial-LTO system, though it's weird they show up only in this commit.

BTW I have to change the commit for flatbuffers submodule back to a v2 version, like 06c5c7ed0bd987a918cf88caafb094f22cdd1721. What commit should we have it set to?

danngreen commented 10 months ago

Aha! It's a simple copy/paste error. This: https://github.com/4ms/metamodule/blob/aeeec7c6faaccdbd9494d81ea1201fa4bfbd7e1f/firmware/src/CMakeLists.txt#L213

should link to the a7 interface lib, not m4

danngreen commented 10 months ago

Though I get some errors when I set it to a7 (missing include paths perhaps?) just commenting out that line works since it's a header-only library.

danngreen commented 10 months ago

Also, main.elf is linked to lockfree twice: once in src/wifi/flasher/CMakeLists.txt, and once in src/CMakeLists.txt

Pretty sure cmake ignores the extra one, but that could be confusing if we decide to move it later.

danngreen commented 10 months ago

Also this fixes the LTO warnings, too!

LnnrtS commented 10 months ago

Nice! Thanks for the help!

This surfaced a problem with the cmake build: You cannot have the same dependency multiple times with different flags - even though these are actually completely different binaries. I just worked around this by using util/lockfree_fifo_spsc.hh instead of the lockfree library (which I should have done in the first place).

As a sidenote: lockfree also has a bipartite buffer which can be used for efficient DMA based RX but I found that implementation to be broken on a different project.

I still feel there is room for improvement in the way the build is structured but it's not an issue at the moment. So I am not going down that rabbit hole now ;)

BTW I have to change the commit for flatbuffers submodule back to a v2 version, like 06c5c7ed0bd987a918cf88caafb094f22cdd1721. What commit should we have it set to?

..and this was me moving around a folder that was actually a submodule