MISRSA results different in CI and workstation
Closed this issue · 11 comments
Running test_misra.sh on my workstation, I get no errors, but CI reports a few style violations and doesn't fail. We need to figure out why the results are different and why CI doesn't fail even though errors are reported. Also need to fix these violations.
https://github.com/commaai/panda/actions/runs/7770060952/job/21189783553
** PANDA F4 CODE **
Checking /tmp/openpilot/panda/board/main.c ...
Checking /tmp/openpilot/panda/board/main.c: CAN3=1;PANDA=1;STM32F4=1;UID_BASE=1;STM32F423xx...
/tmp/openpilot/panda/board/health.h:24:9: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
float interrupt_load;
^
/tmp/openpilot/panda/board/drivers/interrupts.h:35:7: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
float interrupt_load = 0.0f;
^
/tmp/openpilot/panda/board/health.h:27:12: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
uint16_t spi_checksum_error_count;
^
/tmp/openpilot/panda/board/drivers/spi.h:43:10: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
uint16_t spi_checksum_error_count = 0;
^
/tmp/openpilot/panda/board/main_comms.h:71:58: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
int comms_control_handler(ControlPacket_t *req, uint8_t *resp) {
^
/tmp/openpilot/panda/board/drivers/usb.h:82:9: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
uint8_t resp[USBPACKET_MAX_SIZE];
^
/tmp/openpilot/panda/board/health.h:26:11: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
uint8_t safety_rx_checks_invalid;
^
/tmp/openpilot/panda/board/safety_declarations.h:227:6: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
bool safety_rx_checks_invalid = false;
^
/tmp/openpilot/panda/board/drivers/can_common.h:280:53: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
bool is_speed_valid(uint32_t speed, const uint32_t *speeds, uint8_t len) {
^
/tmp/openpilot/panda/board/stm32fx/llbxcan.h:22:16: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
const uint32_t speeds[] = {100U, 200U, 500U, 1000U, 1250U, 2500U, 5000U, 10000U};
^
nofile:0:0: information: Active checkers: 279/792 (use --checkers-report=<filename> to see details) [checkersReport]
With #1859 , the errors in my local system have now become consistent with the CI.
Can you confirm @adeebshihadeh ?
I am also looking into what commit in cppcheck fixed this issue.
OS: Manjaro Linux x86_64
Kernel: 5.15.146-1-MANJARO
danmar/cppcheck#5925 seems to be the PR that generates the misra 5.8 errors in my local system.
Need to do a deeper dive as to understand why this happens.
Confirmed the results seem to match on my workstation now
** PANDA F4 CODE **
Checking /home/batman/sixpilot/panda/board/main.c ...
Checking /home/batman/sixpilot/panda/board/main.c: CAN3=1;PANDA=1;STM32F4=1;UID_BASE=1;STM32F423xx...
/home/batman/sixpilot/panda/board/libc.h:50:10: warning: Uninitialized variable: dest [uninitvar]
return dest;
^
/home/batman/sixpilot/panda/board/can_comms.h:72:20: note: Calling function 'memcpy', 1st argument '&to_push' value is <Uninit>
(void)memcpy(&to_push, can_write_buffer.data, can_write_buffer.ptr);
^
/home/batman/sixpilot/panda/board/libc.h:50:10: note: Uninitialized variable: dest
return dest;
^
/home/batman/sixpilot/panda/board/health.h:24:9: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
float interrupt_load;
^
/home/batman/sixpilot/panda/board/drivers/interrupts.h:35:7: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
float interrupt_load = 0.0f;
^
/home/batman/sixpilot/panda/board/provision.h:8:35: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
void get_provision_chunk(uint8_t *resp) {
^
/home/batman/sixpilot/panda/board/drivers/usb.h:82:9: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
uint8_t resp[USBPACKET_MAX_SIZE];
^
/home/batman/sixpilot/panda/board/drivers/can_common.h:280:53: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
bool is_speed_valid(uint32_t speed, const uint32_t *speeds, uint8_t len) {
^
/home/batman/sixpilot/panda/board/stm32fx/llbxcan.h:22:16: style: misra violation (use --rule-texts=<file> to get proper output) [misra-c2012-5.8]
const uint32_t speeds[] = {100U, 200U, 500U, 1000U, 1250U, 2500U, 5000U, 10000U};
^
nofile:0:0: information: Active checkers: 279/792 (use --checkers-report=<filename> to see details) [checkersReport]
However, if I run it again, I don't get back all the failures, particularly the 5.8 ones. Perhaps the cache is the reason?
** PANDA F4 CODE **
Checking /home/batman/sixpilot/panda/board/main.c ...
/home/batman/sixpilot/panda/board/libc.h:50:10: warning: Uninitialized variable: dest [uninitvar]
return dest;
^
/home/batman/sixpilot/panda/board/can_comms.h:72:20: note: Calling function 'memcpy', 1st argument '&to_push' value is <Uninit>
(void)memcpy(&to_push, can_write_buffer.data, can_write_buffer.ptr);
^
/home/batman/sixpilot/panda/board/libc.h:50:10: note: Uninitialized variable: dest
return dest;
^
nofile:0:0: information: Active checkers: 177/792 (use --checkers-report=<filename> to see details) [checkersReport]
Can confirm it is the cache. Deleting and running a fresh cppcheck returns back the misra errors.
Alright, so the update seems to partially solve this. Let's just get the violations fixed in the update PR and get that merged, then we can tackle the rest of this separately. Bounty is locked to you
inconsistent results from multiple cppcheck analysis seems to be an issue from cppchecker's side.
This issue seems to be caused by the caching done by --cppcheck-build-dir flag.
We can probably creating a work around by deleting the cache everytime we run into an error while running cppchecker. Though we would probably lose some of the speed up that come with caching.
Let me know what you think @adeebshihadeh .
If that's true, let's just drop the caching entirely. Any speedup is not worth an inconsistent test.
The other part of this is the not failing CI even though it reports errors; is this a separate cppcheck bug?
Seems like it.
Running the cppchecker with the --addon flag seems to generate the wrong exit code of 0.
Creating a dump and then running the misra addon over it directly seems to generate the correct exit code of 1.
Created an issue on cppchecker's issue tracker for the same.
Seems like it. Running the cppchecker with the
--addonflag seems to generate the wrong exit code of 0. Creating a dump and then running the misra addon over it directly seems to generate the correct exit code of 1. Created an issue on cppchecker's issue tracker for the same.
Nice find. Can we fix this for now by just adding a second pass? Or maybe it's just easier too fix this in upstream cppecheck?
If the time difference between both of these options isn't too substantial, I would prefer we just run the misra addon over the dump, as we are already cherrypicking my previous commit.
Nice job @0x41head. There's a bunch of good open openpilot bounties up for grabs if you want - https://github.com/orgs/commaai/projects/26