GCC ARM: an undefined instruction possibly inserted - a missing sanity of the pointer parameter value

[jirij]

Dear all, the GCC ARM compiler does generate the undefined instruction "0xdeff" in the module "spiffs_cache.c" for following lines (157 in the version 0.3.6): s32_t res2 = SPIFFS_HAL_READ(fs, addr - SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr),

The used version of (GNU Tools for ARM Embedded Processors) is 6.2.1. Please may you treat an input in some "safer" way to avoid such a behavior (regardless of the fact/evaluation that is the proper or false one ;-))

Just a suggestion:

{ s32_t res2 = SPIFFS_ERR_NOT_READABLE; u32_t pAddr = addr - SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr); if (pAddr > 0u) { res2 = SPIFFS_HAL_READ(fs, pAddr, SPIFFS_CFG_LOG_PAGE_SZ(fs), spiffs_get_cache_page(fs, cache, cp->ix)); } if (res2 != SPIFFS_OK) { res = res2; } }

Thanks a lot, BR, JiriJ

[pellepl]

I am not sure I understand. What input do you mean? The sources? Exactly what line in spiffs_cache.c is it? What gcc switches are you using? Arm/thumb/thumb2, optimization flags, floating point support etc.

Den 13 mars 2017 3:15 PM skrev "jirij" notifications@github.com:

Dear all, the GCC ARM compiler does generate the undefined instruction "0xdeff" in the module "spiffs_cache.c" for following lines: s32_t res2 = SPIFFS_HAL_READ(fs, addr - SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr),

The used version of (GNU Tools for ARM Embedded Processors) is 6.2.1. Please may you treat an input in some "safer" way to avoid such a behavior (regardless of the fact/evaluation that is the proper or false one ;-))

Thanks a lot, BR, JiriJ

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pellepl/spiffs/issues/134, or mute the thread https://github.com/notifications/unsubscribe-auth/AE3NVykE_tzno-WbcFWiROspiVacA958ks5rlU-UgaJpZM4MbTtb .

[jirij]

Hi, I am sorry for incomplete and confusing information!

Compiler flags / switches: CF= -mcpu=cortex-r5 -mbig-endian -mthumb -mthumb-interwork -mfpu=vfpv3-d16 -mfloat-abi=hard -fomit-frame-pointer The lines of the object listing file (spiffs_cache.o.lst) follow:

 151:src/ext/spiffs/src/spiffs_cache.c ****         addr - SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr),
 486                    .loc 1 151 0
 487 01e6 2300          movs    r3, #0
 488 01e8 785B          ldrb    r3, [r3, #1]    @ zero_extendqisi2
 489 01ea DEFF          .inst   0xdeff

As you can see the compiler inserts an opcode for the undefined instruction (as GCC defines that ;-)) at the line of 489. During run-time this will throw the undefined instruction exception. So the compiler has some issue with the macro parameters. The suggested code snippet above was just a try - I must admit that was the false try because the zero value of the addr argument is acceptable - the file system cannot mount without a following condition

if (pAddr >= 0u)

Hope this helps demystify my issue a bit. BR, JiriJ

[pellepl]

Hi Jirij, ah ok, you seem to have stumbled on a gcc quirk. That's always fun! :) This might be a stupid suggestion, or a not feasible option, but did you try a later version of gcc? Anyhow, the macros should not be the problem but rather the whole preprocessed thing. Could you do this for me:

• preprocess spiffs_cache.c using your configurations and attach that file (add -E to your compile flags and the output will end up the .o files)
• attach a disassembly file of spiffs_cache.c with source code interleaved with assembly I am interested in what goes wrong in gcc. It might turn out that it is a spiffs bug also, however it really does look like a gcc thing.

Also, please try replacing current code with this (original spiffs_cache.c lines 157-160):

      s32_t res2 = SPIFFS_HAL_READ(fs,
          (u32_t)((u32_t)addr - (u32_t)SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr)),
          SPIFFS_CFG_LOG_PAGE_SZ(fs),
          spiffs_get_cache_page(fs, cache, cp->ix));

Finally, did you build the compiler yourself or did you download it from somewhere?

Cheers / P

[jirij]

Hi pellepl, I have got the same result with the previous GCC version - 5.4_2016q3. And yes, we have an own build (done by my colleague) of the compiler - the motivation for this procedure is to patch the official GCC release for supporting big-endian Cortex-R...

Preprocessor results: 1) for the original code s32_t res2 = (fs)->cfg.hal_read_f((fs), (addr - ( ((addr) - ((fs)->cfg.phys_addr)) % ((fs)->cfg.log_page_size) )), (((fs)->cfg.log_page_size)), (((u8_t *)(&((cache)->cpages[(cp->ix) * (sizeof(spiffs_cache_page) + ((fs)->cfg.log_page_size))])) + sizeof(spiffs_cache_page))))

2) for the dirty "work-around" u32_t pAddr = addr - ( ((addr) - ((fs)->cfg.phys_addr)) % ((fs)->cfg.log_page_size) ); if (pAddr >= 0u) { res2 = (fs)->cfg.hal_read_f((fs), (pAddr), (((fs)->cfg.log_page_size)), (((u8_t *)(&((cache)->cpages[(cp->ix) * (sizeof(spiffs_cache_page) + ((fs)->cfg.log_page_size))])) + sizeof(spiffs_cache_page)))) }

Further observation will come.. ;-)

BR, JiriJ

[jirij]

Hi pellepl,

I am very sorry that was a false error! I have got into a mess in my code - jumping over the spiffs versions and got lost... The GCC ARM compiler does NOT generate the opcode for the undefined instruction when I am using the latest spiffs version of 0.3.6. Anyway the previous versions (I have tested 0.3.3, 0.3.4 and 0.3.5) are somehow affected - I have not revealed what is behind, where could an issue lie... if I am not so busy I will try to get more information. Thanks for your support, BR, JiriJ

[pellepl]

Hehe, no problem - I am glad it was false alarm, as I'd like to avoid compiler specifics as much as possible. If you get time to find the culprit in earlier versions, please let me know - this is valuable information for future bug tracking. And hey, all cool guys use homebuilt compilers. 😉