code cleanup for building with latest tools

[elidwa]

H5Coro.cpp has an issue with info.data which is uint8_t* It gets allocation as byte array but later the pointer gets casted to float/double etc. This is not safe. Starting in C++ 17 'new' on byte array may be aligned to 2, 4, 8, 16 bytes. Before C++ 17 it was either 4 or 8 bytes. cpp-check is very unhappy about it. For now, I added this line to project-config.cmake to disable the warning:

"--suppress=invalidPointerCast:/H5Coro.cpp" # info.data (uint8_t being cast to float/double ptrs. Alignment issues)

We could ignore it (what we do now) Allocate memory with with std::aligned_alloc() guaranteeing the max needed alignment.

BathyOcensEyes.cpp line 653. Added check for kd_range_index value being out of bounds. cpp-check detected that UNCERTAINTY_COEFF_MAP may be indexed out of bounds.

[jpswinski]

Instead of the addition on line 653 of BathyOcensEyes.cpp, can we modify the lines above it with the following (notice the minus one in the kd_range_index check):

            /* get kd range index */
            int kd_range_index = 0;
            while(kd_range_index < (NUM_KD_RANGES - 1) && KD_RANGES[kd_range_index][1] < kd)
            {
                kd_range_index++;
            }

[jpswinski]

Also, do we need the memset on line 459 of H5Coro.cpp that was added?

[elidwa]

Updated BathyOcensEyes.cpp as requested. As for the memset on line 459 in H5Coro.cpp the cpp-checker gets confused and reports:

[ 1%] Building CXX object CMakeFiles/slideruleLib.dir/packages/h5/H5Coro.cpp.o /home/elidwa/ICESat2/sliderule/packages/h5/H5Coro.cpp:466:29: error: Uninitialized variable: &info [uninitvar] const H5Dataset dataset(&info, context, datasetname, slice, slicendims, _meta_only);

I have two choices. Keep the memset and initialize the variable or disable cppcheck-suppress uninitvar I would have to do it for the whole file. Doing it per file/line works but the moment the file changes the check would be in a wrong place and would fail again.

I suspect that cppchecker gets confused in void H5Dataset::readDataset (info_t* info)
there are 'if' statements which check for particular case of metaData.type but these don't have default 'else'. Checker sees that info->datatype only gets set for some of the metaData.type and complains about it. There may be other reasons as well. I tried fixing it by adding 'else' but it still complained.

[jpswinski]

I've been trying to move away from memsets when possible (though I am sure there are still a ton in the code). The reason is that it precludes any meaningful check of uninitialized access that would be caught by the address sanitizer in the self test.

As for the alignment of new I didn't realize that new might not align to 8 bytes - I think we should fix this in the code. Do you know if there are any performance considerations between using new uint64_t [] vs std::aligned_alloc()? I think x86 and ARM both support misaligned access, but is there a performance penalty to it?

[elidwa]

memset has been removed

info.data is being newed with C++ 17 overloaded new operator which allows for alignment. This method is preferred from st::aligned_alloc() Notice how operator delete[] also has to specify alignment or address sanitizer blows up if regular delete[] is called.

x86 and ARM both support misaligned access. There is a small performance hit for both architectures since loading unaligned address to the register may require multiple fetch data instructions. ARM seems to be better at it than x86. There is also the data cache performance hit. Unaligned data may span multiple cache lines.