Idea to integrate cppcheck

[smoe]

There may be better ideas how to address this. The plan is to iteratively add all directories for which cppcheck works. At some point the coverage should then be good enough to simplify by calling it on larger subtrees.

[smoe]

So, here my shot at #1574 .
This is admittedly much better than I had expected. I ran into one false positive that is trivial to see by eye so I am not worried. Also

stashf_wrap.h:26:5: error: syntax error: keyword 'if' is not allowed in global scope [syntaxError]
    if(result < 0) return result;

is somewhat too unconventional for cppcheck (and it is weird, indeed, the .h ending should maybe change to .include?)

But some limitations of cppcheck are easily outweighed by basically impossible to find gems like:

emcsh.cc:3338:4: error: Read and write operations without a call to a positioning function (fseek, fsetpos or rewind) or fflush in between result in undefined behaviour. [IOWithoutPositioning]
   fputc(77, inFile); // Request data resent

or

saicanon.cc:325:30: error: Out of bounds access of nurbs_control_points, index 'nurbs_control_points.size()' is out of bounds. [containerOutOfBoundsIndexExpression]
  _sai._program_position_x = nurbs_control_points[nurbs_control_points.size()].X;

[petterreinholdtsen]

This sound like a good idea. Anything that can help us find bugs automatically is a good idea.

[smoe]

I must admit that I do not really know how to integrate this properly in our build infrastructure. Run it on all the source tree and somewhere maintain a list of known issues that we do not want to fix?

[smoe]

My initial plan IIRC was to incrementally add files that should be cppchecked and then fail if any of these shows any new errors.

[petterreinholdtsen]

[Steffen Möller]

I must admit that I do not really know how to integrate this properly in our build infrastructure. Run it on all the source tree and somewhere maintain a list of known issues that we do not want to fix?

I suspect src/Makefile would need an update with TARGETS += cppcheck or similar, and the CI scripts and d/control need a new build dependency.

…

-- Happy hacking Petter Reinholdtsen

[silopolis]

Freecad seem to have a nice and sound code checking setup in its CI

[smoe]

Note to self by our friendly Jitsie-committee: Rebase! Introduce variables!
And more !!!s

[jepler]

Hi! I like the general idea, but may I suggest to put the body of the logic in a script (a .sh file somewhere) so that not only can actions run the script but so can a developer locally. Also, the branch has a conflict now.

[rene-dev]

good idea, the Norwegian linuxcnc meeting agrees with @jepler

[heeplr]

i've sent a quick PR so lots of errors won't trigger CI for now so this could be merged without breaking stuff.

[heeplr]

@smoe can you rebase your branch/resolve conflicts? there were a lot of commits since back then ;)

[rene-dev]

best way ist using compile_commands.json
https://clang.llvm.org/docs/JSONCompilationDatabase.html

make clean
bear -- make -j
cppcheck --project=compile_commands.json

[smoe]

@smoe can you rebase your branch/resolve conflicts? there were a lot of commits since back then ;)

Rebased. Also, I split the cppcheck invocations into multiple steps. I do not expect that build error to be triggered by the rebase. Had a look at what cppcheck brings, and, seems like some of them are real:

44/46 files checked 76% done
interp_convert.cc:5900:14: error: Uninitialized variable: beta [legacyUninitvar]
        if ((beta < -small) || (beta > (M_PIl + small))) {
             ^
interp_convert.cc:5895:45: error: Uninitialized variable: gamma [legacyUninitvar]
        end_x = (px + (radius * cos(alpha + gamma)));

Yes, for the else branch beta is not set. Would not know how to set it, admittedly. 0.0?

nurbs_additional_functions.cc:969:23: error: When ii==nurbs_costant.size(), nurbs_costant[ii] is out of bounds. [stlOutOfBounds]
     a = nurbs_costant[ii]; //uj
                      ^

Yes.

stepgen.c:1207:55: error: Array 'user_step_type[1]' accessed at index 17, which is out of bounds. [arrayIndexOutOfBounds]
        master_lut[USER_STEP_TYPE][i] = user_step_type[i];
                                                      ^
stepgen.c:1208:31: error: Array 'user_step_type[1]' accessed at index 17, which is out of bounds. [arrayIndexOutOfBounds]
 used_phases |= user_step_type[i];
              ^

I am tempted to call this a false positive. Problem with designated initializers?

Anyway, first it needs to build again :-)

[jepler]

The bit about undefined beta/gamma:

5892             gamma = -M_PI_2l;
5893         } else
5894             ERS(NCE_BUG_SIDE_NOT_RIGHT_OR_LEFT);
5895         end_x = (px + (radius * cos(alpha + gamma)));

ERS is a macro which always ends with "return;", so the end_x line which depends on the value of gamma is unreachable from the else branch:

// Set an error string using printf-style formats and return
#define ERS(fmt, ...)                                      \
    do {                                                   \
        setError (fmt, ## __VA_ARGS__);                    \
        _setup.stack_index = 0;                            \
        (rtapi_strlcpy(_setup.stack[_setup.stack_index], __PRETTY_FUNCTION__, STACK_ENTRY_LEN)); \
        _setup.stack[_setup.stack_index][STACK_ENTRY_LEN-1] = 0; \
        _setup.stack_index++; \
        _setup.stack[_setup.stack_index][0] = 0;           \
        return INTERP_ERROR;                               \
    } while(0)

So, EITHER it looks like this is a false positive from cppcheck OR there's something subtle going on with the ERS macro that means the code actually can continue from line 5894 to 5895.

[jepler]

950                 for(unsigned ii=0; ii<= nurbs_costant.size(); ii=ii+6)

this loop check is probably just wrong, it's almost always an error to loop to <= size

[jepler]

 308 #define MAX_CYCLE 18
...
 319 int user_step_type[] = { [0 ... MAX_CYCLE-1] = -1 };
...
1206     for(i=0; i < MAX_CYCLE && user_step_type[i] != -1; i++) {
1207         master_lut[USER_STEP_TYPE][i] = user_step_type[i];
1208         used_phases |= user_step_type[i];
1209     }

I checked GCC documentation for this gcc extension and I think this is a false positive. The range of this kind of initializer has an inclusive upper bound, so user_step_type[17] is initialized to -1 and is valid to access.

To initialize a range of elements to the same value, write [FIRST ... LAST] = VALUE. This is a GNU extension. For example,

    int widths[] = { [0 ... 9] = 1, [10 ... 99] = 2, [100] = 3 };

If the value in it has side effects, the side effects happen only once, not for each initialized field by the range initializer.

This might be a false positive that occurs because the GCC extension is not properly implemented in cppcheck. Adding the explicit bound might fix it?

int user_step_type[MAX_CYCLE] = { [0 ... MAX_CYCLE-1] = -1 };

[jepler]

int foo(int x) {
    int i;
    if(x == 1) {
        i = 3;
    } else {
        do {
            return 2;
        } while(0);
    } 
    return i;
}

reduced cppcheck test case for false positive legacyUninitvar -- someone interested could report it at their issue tracker. A long time ago they fixed a simpler case, but not when the return is in a do/while.

[heeplr]

someone interested could report it

Thanks a lot for the investigation! Does one get a github notification when highlighted in a random issue? Then @danmar might see this.
My sf account is buried somewhere but I'll try to report it.
Sent email.

[smoe]

The bit about undefined beta/gamma:
...
ERS is a macro which always ends with "return;", so the end_x line which depends on the value of gamma is unreachable from the else branch:

I was not aware of the ERS macro. Fooled me, too. False positive, I tend to think.
Should we not possibly anyway just initialize beta and gamma with 0.0 and add a comment that the value will always be reset if used?

[danmar]

....
reduced cppcheck test case for false positive legacyUninitvar -- someone interested could report it at their issue tracker. A long time ago they fixed a simpler case, but not when the return is in a do/while.

I have reported https://trac.cppcheck.net/ticket/13227 thanks!

[jepler]

Thanks @danmar !

[heeplr]

Should we not possibly anyway just initialize beta and gamma with 0.0 and add a comment that the value will always be reset if used?

imho it probably wouldn't hurt. (Aeons ago I learned "All variable definitions which aren't static need to be initialized" to avoid nasty bugs. But not sure if that still holds.)

[smoe]

Should this have been a separate PR? Possibly, sorry for that. I have added shellcheck and applied it on *.sh in the archive. This seems to work just fine, have also added some commits that address some of the shellcheck's concerns. These were morely about missing quotes. Missing checks if an operation was successful I have addressed by setting bash's "-e" flag. Most problems were in the test scripts, so I am a bit curious if those still work now :)

[smoe]

Should we not possibly anyway just initialize beta and gamma with 0.0 and add a comment that the value will always be reset if used?

imho it probably wouldn't hurt. (Aeons ago I learned "All variable definitions which aren't static need to be initialized" to avoid nasty bugs. But not sure if that still holds.)

Done in 4c42080 .

[heeplr]

@smoe just want to say that i appreciate all your effort so much. This will add a good amount of solidness to the codebase.

If we can at some point remove the "continue-on-error" to make CI fail on warnings, less efforts need to be taken when merging PRs aswell. A bright future lies ahead. All hail to static code analysis! ;)

[smoe]

The next upload will have the separate scripts that @jepler and the Oslo folks suggested. Testing it, I was directed to ./src/hal/classicladder/edit_gtk.c

Checking edit_gtk.c ...
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.VarType [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.VarNum [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.IndexedVarType [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.IndexedVarNum [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.DynamicInput [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.DynamicState [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.DynamicVarBak [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized struct member: ToolBarEle.DynamicOutput [uninitStructMember]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^
edit_gtk.c:423:76: error: Uninitialized variables: ToolBarEle.VarType, ToolBarEle.VarNum, ToolBarEle.IndexedVarType, ToolBarEle.IndexedVarNum, ToolBarEle.DynamicInput, ToolBarEle.DynamicState, ToolBarEle.DynamicVarBak, ToolBarEle.DynamicOutput [uninitvar]
    DrawElement( cr, 0, 0, PIXELS_SIZE_IN_TOOLBAR, PIXELS_SIZE_IN_TOOLBAR, ToolBarEle, TRUE);
                                                                           ^

The "error" is a bit drastic, not? Shall I find out how to override this one?

Have started extending the range of files to be checked. The directory src/hal/drivers is challenging:

I tend to agree that a "return 0;" is missing:

Checking src/hal/drivers/mesa-hostmot2/uart.c ...
src/hal/drivers/mesa-hostmot2/uart.c:340:5: error: Found a exit path from function with non-void return type that has missing return statement [missingReturn]
    }

The following may need some comments in the code - or a fix :-)

src/hal/drivers/mesa-hostmot2/sserial.c:1550:83: error: Shifting 32-bit value by 48 bits is undefined behaviour [shiftTooManyBits]
                            if (abs(((int)(p->s64_param) - (int)(p->s64_written)) >> shift) > 2) break;
                                                                                  ^
src/hal/drivers/mesa-hostmot2/sserial.c:1542:49: note: Assignment 'shift=52-4', assigned value is 48
                                    shift = (52 -  4); break; // 1.3.4 minifloat, if we ever add them
                                                ^
src/hal/drivers/mesa-hostmot2/sserial.c:1550:83: note: Shift
                            if (abs(((int)(p->s64_param) - (int)(p->s64_written)) >> shift) > 2) break;

The following reads like intentional, a bit weird, though:

src/hal/drivers/mesa-hostmot2/pwmgen.c:671:48: error: Signed integer overflow for expression '1<<31'. [integerOverflow]
            hm2->pwmgen.pwm_value_reg[i] |= (1 << 31);

The corresponding source code is

            register_value = scaled_value * (double)(((1 << bits) - topdrop))+ (1 << bits);                   
    }                                                
        hm2->pwmgen.pwm_value_reg[i] = register_value * 65536;
        if (scaled_value < 0) {                      
            hm2->pwmgen.pwm_value_reg[i] |= (1 << 31);
        }    

so it looks a bit like an intentional effort to reach the bit indicating the sign on 32bit systems.

@rene-dev , I have seen your comment in #1581 (comment) but have no exact idea how to incorporate this idea into this PR. Is this possibly for later once we have the cppcheck mandatory for all of our code?