Lora-net / LoRaMac-node

Reference implementation and documentation of a LoRa network node.
Other
1.87k stars 1.09k forks source link

Potentitally uninitialised MlmeConfirm_Queue_t when building master/v4.7.0 #1352

Closed dmeehan1968 closed 1 year ago

dmeehan1968 commented 1 year ago

I had reason to grab the master branch to check if some unexpected behaviour was manifesting in later build (I was building against 4.4.5 for an existing project).

I'm not building the sample projects, or using the included CMakeLists.txt, but have created my own CMakeLists.txt to pull in the LoRaMac sources.

I'm building for STM32L082 MCU on a custom board, closely related to the B-L072Z-LRWAN1 reference board, and using gcc-arm-none-eabi compiler. I'm getting the following error which seems genuine, and maybe this is a warning that you are suppressing in your builds.

I appreciate that master is not a release, but as work seems to be complete on 4.7.0 milestone thought you might like to check this before release.

/Users/XXX/Documents/Dropbox/Projects/XXX/LORA/XXX/build/shared/loramac/src/src/mac/LoRaMacConfirmQueue.c: In function 'LoRaMacConfirmQueueHandleCb':
/Users/XXX/Documents/Dropbox/Projects/XXX/LORA/XXX/build/shared/loramac/src/src/mac/LoRaMacConfirmQueue.c:175:46: warning: 'mlmeConfirmToStore.ReadyToHandle' may be used uninitialized in this function [-Wmaybe-uninitialized]
  175 |     ConfirmQueueCtx.BufferEnd->ReadyToHandle = mlmeConfirm->ReadyToHandle;
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/XXX/Documents/Dropbox/Projects/XXX/LORA/XXX/build/shared/loramac/src/src/mac/LoRaMacConfirmQueue.c:284:24: note: 'mlmeConfirmToStore.ReadyToHandle' was declared here
  284 |     MlmeConfirmQueue_t mlmeConfirmToStore;
      |                        ^~~~~~~~~~~~~~~~~~
mluis1 commented 1 year ago

I have tried to run the build using GCC with -Wmaybe-uninitialized option and my version of the compiler does not produce the mentioned warning.

Maybe the GCC version that I am using understands that in the mentioned case the variable is ensured to always be initialized as the initialization and variable usage only happens if readyToHandle variable is equal to false.

Variable initialization: https://github.com/Lora-net/LoRaMac-node/blob/e533790a3322041dc63a2edbf1b496c57b1054fa/src/mac/LoRaMacConfirmQueue.c#L292-L302

Variable usage: https://github.com/Lora-net/LoRaMac-node/blob/e533790a3322041dc63a2edbf1b496c57b1054fa/src/mac/LoRaMacConfirmQueue.c#L307-L311

For info the GCC version that I am currently using is:

C:\Program Files (x86)\GNU Arm Embedded Toolchain\10 2020-q4-major\bin>arm-none-eabi-gcc.exe --version
arm-none-eabi-gcc.exe (GNU Arm Embedded Toolchain 10-2020-q4-major) 10.2.1 20201103 (release)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
mluis1 commented 1 year ago

I have tried as well to compile with latest available GCC version and no warning is generated when using -Wmaybe-uninitialized

C:\Program Files (x86)\GNU Arm Embedded Toolchain\10 2021.10>arm-none-eabi-gcc --version
arm-none-eabi-gcc (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Could you please indicate which compiler version you are using? Would be nice as well if you could provide the compiler options that you are using.

dmeehan1968 commented 1 year ago

Running on MacOS 12.6

arm-none-eabi-gcc (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)

On closer inspection I realised that this was only warning on a release build, where -O3 is used for max optimisation. -O2 also produces the same warning, and I believe that higher optimisations removes default initialisation and therefore the lack of specific initialisation on LoRaMacConfirmQueue.c:284 means that ReadyToHandle could be uninitialised.

The following mutes the warning:

MlmeConfirmQueue_t mlmeConfirmToStore = {};

You might have a recommendation about maximum optimisation levels, but I've been compiling with -O3 for some time, and this just cropped up on the master branch (HEAD @ e533790a3322041dc63a2edbf1b496c57b1054fa). I just checked the blame history for the code and neither the definition or the usage appears to have changed in several years so I'm not sure why it would have started bleating about it now.

mluis1 commented 1 year ago

Usually we only compile with Og for debug and Os for release. These two optimizations do not generate the observed warning. I have tried O1 optimization which does not generate the warning. I was able to see the warning with O2 and O3.

In order to suppress the warning when using O2 and O3 optimizations we will add the code to initialize the variable.