espressif/esp-nn

Nullptr checks in test files don't work at all

maxgerhardt opened this issue · 1 comments

Consider

int8_t *input_orig = malloc(in_size + 32);
int8_t *out_c_orig = malloc(out_size + 32);
int8_t *out_opt_orig = malloc(out_size + 32);
filter_data = malloc(filter_size);
bias = malloc(bias_size * 4);
#endif
input = 16 + input_orig - ((uint32_t) input_orig & 0xf);
out_data_c = 16 + out_c_orig - ((uint32_t) out_c_orig & 0xf);
out_data_opt = 16 + out_opt_orig - ((uint32_t) out_opt_orig & 0xf);
if (bias == NULL || input == NULL || filter_data == NULL ||
out_data_c == NULL || out_data_opt == NULL || bias == NULL) {
printf(ANSI_COLOR_RED"[%d] allocations failed\n"ANSI_COLOR_RESET, itr);
goto dc_s8_cleanup;
}

If input_orig = NULL = 0, then,

        input = 16 + input_orig - ((uint32_t) input_orig & 0xf);

will compute input = 16 + 0 - (0 & 0xf) = 16;

And since 16 != NULL, this will not trigger the if(input == NULL) check at all.

You need to perform the null checks on the original data, not this weird 16 byte aligned value.

What I find also weird is how it clearly tries to do a 16-byte alignment, by adding at maximum 15 to the original pointer value, but the overallocation is by +32? Should that not need only 15 really, in the worst case that 15 bytes need to be skipped? If 16 bytes were needed to be skipped, it would already be 16-byte aligned.

Not doing this correctly has lead to me hardcrashing.

writing to input buff at input = 0x10
Guru Meditation Error: Core  1 panic'ed (StoreProhibited). Exception was unhandled.

Core  1 register dump:
PC      : 0x400d1f98  PS      : 0x00060e30  A0      : 0x800d12da  A1      : 0x3ffb7fa0
A2      : 0x00000002  A3      : 0x00000008  A4      : 0x00069200  A5      : 0x00000000
A6      : 0x00000010  A7      : 0x3ffb83a0  A8      : 0x00000010  A9      : 0x3ffb7f80
A10     : 0x00000056  A11     : 0x3ffb85dc  A12     : 0x0008125c  A13     : 0x00000000
A14     : 0x00000002  A15     : 0x00000002  SAR     : 0x00000013  EXCCAUSE: 0x0000001d
EXCVADDR: 0x00000010  LBEG    : 0x400014fd  LEND    : 0x4000150d  LCOUNT  : 0xffffffff


Backtrace: 0x400d1f95:0x3ffb7fa0 0x400d12d7:0x3ffb84b0 0x400d898d:0x3ffb84d0 0x400898bd:0x3ffb84f0
  #0  0x400d1f95 in esp_nn_depthwise_conv_s8_test at src/esp-nn-test-funcs/convolution_test.c:236 (discriminator 3)
  #1  0x400d12d7 in loop() at src/main.cpp:53
  #2  0x400d898d in loopTask(void*) at C:/Users/Max/.platformio/packages/framework-arduinoespressif32/cores/esp32/main.cpp:50
  #3  0x400898bd in vPortTaskWrapper at C:\Users\Max\.platformio\packages\framework-espidf@3.40407.240606\components\freertos\port\xtensa/port.c:142

Fair point. Checks indeed need to be done on the original pointers. I shall fix these. And the removal of over-allocation too.
That given, looks like you get a crash since, allocations are failing. Please use SPIRAM for the tests. The allocations needed for some tests are huge.