Skip to content

Fix type mismatch of line and column number and add extra information in misra-reports #5407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
PeterSchops wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Fix type mismatch of line and column number and add extra information in misra-reports #5407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1c6f79f
Fix type mismatch in MacroUsage
PeterSchops Jul 31, 2023
608281e
Adding extra information in report for some issues
PeterSchops Jul 31, 2023
7ecf24b
Merge branch 'danmar:main' into PeterSchops_Fix_type_mismach_in_Macro…
PeterSchops Aug 1, 2023
b8690f5
Merge branch 'danmar:main' into PeterSchops-misra_add_optional_extra_…
PeterSchops Aug 1, 2023
9a4912b
Update string formatting for python2 compatability
PeterSchops Aug 2, 2023
ceb8905
fix argument count mismatch
PeterSchops Aug 2, 2023
9c4aec1
Merge pull request #2 from PeterSchops/PeterSchops_Fix_type_mismach_i…
PeterSchops Sep 1, 2023
2f5b55d
Merge branch 'danmar:main' into PeterSchops-misra_add_optional_extra_…
PeterSchops Sep 1, 2023
05df446
Merge branch 'danmar:main' into PeterSchops-misra_add_optional_extra_…
PeterSchops Sep 1, 2023
9e0c2db
Merge branch 'danmar:main' into PeterSchops-misra_add_optional_extra_…
PeterSchops Sep 5, 2023
097672b
Merge pull request #3 from PeterSchops/PeterSchops-misra_add_optional…
PeterSchops Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions addons/cppcheckdata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ def __init__(self, element):
self.name = element.get('name')
_load_location(self, element)
self.usefile = element.get('usefile')
self.useline = element.get('useline')
self.usecolumn = element.get('usecolumn')
self.useline = int(element.get('useline'))
self.usecolumn = int(element.get('usecolumn'))
self.isKnownValue = element.get('is-known-value', 'false') == 'true'

def __repr__(self):
Expand Down Expand Up @@ -1612,7 +1612,7 @@ def is_suppressed(location, message, errorId):
return True
return False

def reportError(location, severity, message, addon, errorId, extra=''):
def reportError(location, severity, message, addon, errorId, misra_severity='', extra=''):

@danmar danmar Sep 11, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spontanously it does not feel very good to add a "misra_severity" here. This is the generic function it's not related to misra. We have other addons also and I don't want to add various special parameters for those.

So well one approach could be to rename the parameters somehow to have generic parameters that works for all addons..

@PeterSchops PeterSchops Sep 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not technically add the misra_severity, I just renamed the parameter, to the argument name used in misra.py, and then I re-used the extra parameter, for the extra information I wanted to add, since it seemed like a good name for additional error information.

https://github.com/danmar/cppcheck/blob/a87e9e104239785058e91811ea1dbf5bdc44e919/addons/misra.py#L4132

image

@danmar danmar Sep 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is not a good name for all the other addons.

If I report an error in the misc.py addon and then I want to provide some extra information to the error message then I don't feel that we should use misra_severity.. and I don't want to add more parameters like misc_severity or misc_hint.. I want to reuse the same options.

I want a generic argument name that will make sense in other addons also.

if '--cli' in sys.argv:
msg = { 'file': location.file,
'linenr': location.linenr,
Expand All @@ -1621,12 +1621,14 @@ def reportError(location, severity, message, addon, errorId, extra=''):
'message': message,
'addon': addon,
'errorId': errorId,
'extra': extra}
'extra': misra_severity}

@danmar danmar Sep 12, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just use the extra. If "extra" has some text (reported from misc.py or any other addon) then why isn't that used in the output?

@PeterSchops PeterSchops Sep 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not use extra, because today in misra.py misra_severity is stored in the extra field.
When I look in the cppcheck XML output I find following fields:

  • id: misra_2012_x_y
  • severity: would be ideal for the severity information
  • msg: place for the error message from the file
  • verbose: the place for extra information
  • cwe
  • file0
  • location of the error

I use my own reportError function, that does write a XML-file, that is close to what cppcheck creates today. And I did not want to change the reportError function provided in cppcheckdata.py significantly to avoid any problems.

So since misra.py writes severity information twice (cppcheck_severity and misra_severity)
https://github.com/danmar/cppcheck/blob/a87e9e104239785058e91811ea1dbf5bdc44e919/addons/misra.py#L4132

I could remove the misra_severity from misra.py, and replace that with extra or verbose information. But then I might break something when the misra checks are called through cppcheck, because then the --cli option seems to get added automatically. And the misra_severity could be used somewhere.
image

I would like to get the extra information inside, so if you can tell me how you would like me to update the code. I will try to prepare the update.

@danmar danmar Sep 13, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use my own reportError function, that does write a XML-file, that is close to what cppcheck creates today

I do not see that here in this PR.. do you have some special function locally that writes different xml file?

@PeterSchops PeterSchops Sep 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using cppcheck since 1.80, and misra.py evolved a lot. I customized some checks, to not show certain issues, added custom checks, .. and to avoid merging misra.py with mine, I added a wrapper around the misra.py, and it has a XML-write feature, but I can't recall why I did that.
I'm using the python script with the dump-files generated by cppcheck, I'm not sure if misra.py can write XML-files when used as standalone.
It was also not my intention to change anything there. I just want to add some extra/verbose information in the reports, and contribute to the tool, so it can benefit everybody.

@danmar danmar Sep 15, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks

sys.stdout.write(json.dumps(msg) + '\n')
else:
if is_suppressed(location, message, '%s-%s' % (addon, errorId)):
return
loc = '[%s:%i]' % (location.file, location.linenr)
if len(misra_severity) > 0:
message += ' (' + severity + ')'

@danmar danmar Sep 12, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code is strange. is the intention to append the misra severity to the message?

if len(extra) > 0:
message += ' (' + extra + ')'
sys.stderr.write('%s (%s) %s [%s-%s]\n' % (loc, severity, message, addon, errorId))
Expand Down
36 changes: 18 additions & 18 deletions addons/misra.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1549,12 +1549,12 @@ def misra_2_7(self, data):
if func_param.nameToken:
linenr = func_param.nameToken
if linenr not in reported_linenrs:
self.reportError(func_param.nameToken, 2, 7)
self.reportError(func_param.nameToken, 2, 7, '{} is unused'.format(func_param.nameToken.str))
reported_linenrs.add(linenr)
else:
linenr = func.tokenDef.linenr
if linenr not in reported_linenrs:
self.reportError(func.tokenDef, 2, 7)
self.reportError(func.tokenDef, 2, 7, '{} is unused'.format(func.tokenDef.str))
reported_linenrs.add(linenr)

def misra_3_1(self, rawTokens):
Expand All @@ -1580,13 +1580,13 @@ def misra_3_2(self, rawTokens):
# Check for comment ends with trigraph which might be replaced
# by a backslash.
if token.str.endswith('??/'):
self.reportError(token, 3, 2)
self.reportError(token, 3, 2, "found comment ending with '??/', this might be changed by a backslash")
# Check for comment which has been merged with subsequent line
# because it ends with backslash.
# The last backslash is no more part of the comment token thus
# check if next token exists and compare line numbers.
elif (token.next is not None) and (token.linenr == token.next.linenr):
self.reportError(token, 3, 2)
self.reportError(token, 3, 2, "commented line ends with a backslash, which will extend the comment to the next line")

def misra_4_1(self, rawTokens):
for token in rawTokens:
Expand Down Expand Up @@ -1823,7 +1823,7 @@ def misra_7_1(self, rawTokens):
def misra_7_2(self, data):
for token in data.tokenlist:
if token.isInt and ('U' not in token.str.upper()) and token.valueType and token.valueType.sign == 'unsigned':
self.reportError(token, 7, 2)
self.reportError(token, 7, 2, "suffix 'u' missing for {}".format(token.str))

def misra_7_3(self, rawTokens):
compiled = re.compile(r'^[0-9.]+[Uu]*l+[Uu]*$')
Expand Down Expand Up @@ -2744,10 +2744,10 @@ def misra_14_1(self, data):
continue
for counter in findCounterTokens(exprs[1]):
if counter.valueType and counter.valueType.isFloat():
self.reportError(token, 14, 1)
self.reportError(token, 14, 1, "loop counter in for loop is of type float")
elif token.str == 'while':
if isFloatCounterInWhileLoop(token):
self.reportError(token, 14, 1)
self.reportError(token, 14, 1, "loop counter in while loop is of type float")

def misra_14_2(self, data):
for token in data.tokenlist:
Expand All @@ -2756,27 +2756,27 @@ def misra_14_2(self, data):
if not expressions:
continue
if expressions[0] and not expressions[0].isAssignmentOp:
self.reportError(token, 14, 2)
self.reportError(token, 14, 2, "first clause is not empty, and does not contain an assignement operator")
if countSideEffectsRecursive(expressions[1]) > 0:
self.reportError(token, 14, 2)
self.reportError(token, 14, 2, "second clause contains side effects")
if countSideEffectsRecursive(expressions[2]) > 1:
self.reportError(token, 14, 2)
self.reportError(token, 14, 2, "third clause contains more then one side effects")

counter_vars_first_clause, counter_vars_exit_modified = getForLoopCounterVariables(token)
if len(counter_vars_exit_modified) == 0:
# if it's not possible to identify a loop counter, all 3 clauses must be empty
for idx in range(len(expressions)):
if expressions[idx]:
self.reportError(token, 14, 2)
self.reportError(token, 14, 2, "if it's not possible to identify a loop counter, all 3 clauses must be empty")
break
elif len(counter_vars_exit_modified) > 1:
# there shall be a single loop counter
self.reportError(token, 14, 2)
self.reportError(token, 14, 2, "there shall be a single loop counter")
else: # len(counter_vars_exit_modified) == 1:
loop_counter = counter_vars_exit_modified.pop()
# if the first clause is not empty, then it shall (declare and) initialize the loop counter
if expressions[0] is not None and loop_counter not in counter_vars_first_clause:
self.reportError(token, 14, 2)
self.reportError(token, 14, 2, "if the first clause is not empty, then it shall (declare and) initialize the loop counter")

# Inspect modification of loop counter in loop body
body_scope = token.next.link.next.scope
Expand All @@ -2788,7 +2788,7 @@ def misra_14_2(self, data):
if tn.next:
# TODO: Check modifications in function calls
if countSideEffectsRecursive(tn.next) > 0:
self.reportError(tn, 14, 2)
self.reportError(tn, 14, 2, "loop counter modified in loop body")
tn = tn.next

def misra_14_4(self, data):
Expand Down Expand Up @@ -3119,9 +3119,9 @@ def misra_17_1(self, data):
for token in data.tokenlist:
if isFunctionCall(token) and token.astOperand1.str in (
'va_list', 'va_arg', 'va_start', 'va_end', 'va_copy'):
self.reportError(token, 17, 1)
self.reportError(token, 17, 1, "function '{}' should not be used".format(token.astOperand1.str))
elif token.str == 'va_list':
self.reportError(token, 17, 1)
self.reportError(token, 17, 1, "function '{}' should not be used".format(token.str))

def misra_17_2(self, data):
# find recursions..
Expand Down Expand Up @@ -4085,7 +4085,7 @@ def setSuppressionList(self, suppressionlist):

self.addSuppressedRule(ruleNum)

def reportError(self, location, num1, num2):
def reportError(self, location, num1, num2, extra=''):
ruleNum = num1 * 100 + num2

if self.isRuleGloballySuppressed(ruleNum):
Expand Down Expand Up @@ -4129,7 +4129,7 @@ def reportError(self, location, num1, num2):
# skip it since it has already been displayed.
if this_violation not in self.existing_violations:
self.existing_violations.add(this_violation)
cppcheckdata.reportError(location, cppcheck_severity, errmsg, 'misra', errorId, misra_severity)
cppcheckdata.reportError(location, cppcheck_severity, errmsg, 'misra', errorId, misra_severity, extra)

if misra_severity not in self.violations:
self.violations[misra_severity] = []
Expand Down