PC-lint: support supplemental messages

[bloodlee]

Sometimes PCLint plus will generate a lot of useful supplemental messages, which helps us to locate why the issue was found.
My change loads the supplemental into the issue flow. The following is an example.

Thanks.

---

This change is 

[bloodlee]

BTW, I noticed that - if the flow only has one location, this flow will be ignored. So the newer commit uses the secondary locations, not the flow.

Please help to review this update for PC-Lint issues.

Thanks very much.

[guwirth]

@bloodlee thanks for providing this. Please have a look to comments.

[guwirth]

Hi @bloodlee,

do you have some time to have a look to the review comments?

Regards,

[bloodlee]

Yes. Thanks for your comments. I will fix the issue soon.

[ivangalkin]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @bloodlee and @guwirth)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 137 at r2 (raw file):

Quoted 5 lines of code…

              CxxReportIssue issue = new CxxReportIssue(id, file, line, msg);
              if (currentIssue != null) {
                saveUniqueViolation(context, currentIssue);
              }
              currentIssue = issue;

should be IMHO

               if (currentIssue != null) {
                 saveUniqueViolation(context, currentIssue);
               }
               currentIssue = new CxxReportIssue(id, file, line, msg);

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 191 at r2 (raw file):

PREFIX_DURING_SPECIFIC_WALK_MSG + String.format(" %s:%s %s", file, line, msg);

which is basically

msg = String.format("%s %s:%s %s", PREFIX_DURING_SPECIFIC_WALK_MSG, file, line, msg);

BTW I can not agree with your statement "the UI is not ready";
Locations from other files work well and can be referenced/drilled down.
However such "external" file must be included in the SQ project.
Also if the project is a multi-module project, locations MUST be from the same module.

I would recommend to replace the condition if (!primaryLocation.getFile().equals(file)) with the check

if (getInputFileIfInProject(context, file) == null) //  or something like that

You also might rethink all that logic with adding and extracting of PREFIX_DURING_SPECIFIC_WALK_MSG. It is hard to understand.

[bloodlee]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @bloodlee, @guwirth, and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 186 at r2 (raw file):

9

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 191 at r2 (raw file):

Previously, ivangalkin wrote…

PREFIX_DURING_SPECIFIC_WALK_MSG + String.format(" %s:%s %s", file, line, msg);

which is basically

msg = String.format("%s %s:%s %s", PREFIX_DURING_SPECIFIC_WALK_MSG, file, line, msg);

BTW I can not agree with your statement "the UI is not ready";
Locations from other files work well and can be referenced/drilled down.
However such "external" file must be included in the SQ project.
Also if the project is a multi-module project, locations MUST be from the same module.

I would recommend to replace the condition if (!primaryLocation.getFile().equals(file)) with the check

if (getInputFileIfInProject(context, file) == null) //  or something like that

You also might rethink all that logic with adding and extracting of PREFIX_DURING_SPECIFIC_WALK_MSG. It is hard to understand.

Thanks for the comments.

1. About the message formatting, yes, I will improve it.
2. I am not sure which version of Sonarqube you are using. I debugged with Sonarqube latest codeq. On ModuleIssues.java:L114, it checks if secondary component ref and the primary component ref are equal. Code also points to the issue SONAR-9929 in comments.
3. I don't quite familiar with the sonar APIs and I will double check.
4. About extraction, yes, i will do.

[bloodlee]

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @guwirth and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 168 at r1 (raw file):

Previously, bloodlee (Bloodlee) wrote…

You convinced me! I will commit the update.

Done.

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 180 at r1 (raw file):

Previously, bloodlee (Bloodlee) wrote…

Agreed! I will update my implementation.

Done.

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 137 at r2 (raw file):

Previously, ivangalkin wrote…

              CxxReportIssue issue = new CxxReportIssue(id, file, line, msg);
              if (currentIssue != null) {
                saveUniqueViolation(context, currentIssue);
              }
              currentIssue = issue;

should be IMHO

               if (currentIssue != null) {
                 saveUniqueViolation(context, currentIssue);
               }
               currentIssue = new CxxReportIssue(id, file, line, msg);

Done.

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 189 at r2 (raw file):

xxReportLocation primaryLocation = currentIssue.getLocations().get(0);
        if (!primaryLocation.getFile().equals(file)) {
          if (!msg.startsWith(PREFIX_DURING_SPECIFIC_WALK_MSG)) {

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 191 at r2 (raw file):

Previously, bloodlee (Bloodlee) wrote…

Thanks for the comments.

1. About the message formatting, yes, I will improve it.
2. I am not sure which version of Sonarqube you are using. I debugged with Sonarqube latest codeq. On ModuleIssues.java:L114, it checks if secondary component ref and the primary component ref are equal. Code also points to the issue SONAR-9929 in comments.
3. I don't quite familiar with the sonar APIs and I will double check.
4. About extraction, yes, i will do.

I don't think getInputFileIfInProject() is good for this case. Even the file is not in the project, the supplemental message is still meaningful. I don't want to throw them away.

And the logic here is to check if the supplemental message points to the same file as the primary location points to. If no, as what I saw in the code of SQ, I have to use the primary location's file to keep the supplemental message in the project. The file in the supplemental message will show up in msg.

[guwirth]

@bloodlee whole file is marked as changed? Do you use a different charset or line ending? Makes it hard to review...

[guwirth]

@bloodlee one other thing. Think you should use addFlowElement and not addLocation?

[guwirth]

@bloodlee name of your PR should not be master. This is confusing the CI.

[bloodlee]

Sorry about the inconvenience. This is the first time I use this review system.

And "addFlowElement" is not working. I tried but as I commented in the code, there is guard says if the flow had only one location, this flow will be ignored. Please check CxxIssuesReportSensor.java:215. This class is shared with other sensors. I am not sure if I can update it.

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @guwirth and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 174 at r3 (raw file):

not be the first one?
Yes, you are right. I will adjust it.

[ivangalkin]

Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @bloodlee, @guwirth, and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 191 at r2 (raw file):

Previously, bloodlee (Bloodlee) wrote…

I don't think getInputFileIfInProject() is good for this case. Even the file is not in the project, the supplemental message is still meaningful. I don't want to throw them away.

And the logic here is to check if the supplemental message points to the same file as the primary location points to. If no, as what I saw in the code of SQ, I have to use the primary location's file to keep the supplemental message in the project. The file in the supplemental message will show up in msg.

Please grep for getInputFileIfInProject() in Valgrind and cppcheck sensors. It covers the cases, which you want to implement too: link the files/locations, which cannot be represented properly in the WebUI. You are right in your logic, that for the files/location which are not in the project one need to use the primary location.

Also please double check your paths. If they are relative, they have to be relative to the project/module base dir. Otherwise they won't be referenced.

[bloodlee]

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 191 at r2 (raw file):

Previously, ivangalkin wrote…

Please grep for getInputFileIfInProject() in Valgrind and cppcheck sensors. It covers the cases, which you want to implement too: link the files/locations, which cannot be represented properly in the WebUI. You are right in your logic, that for the files/location which are not in the project one need to use the primary location.

Also please double check your paths. If they are relative, they have to be relative to the project/module base dir. Otherwise they won't be referenced.

The paths in pclint report are relative ones to the base dir. Basically, it needs pclint check and sonar-scanner run in the same base dir.

I will double-check the API usage in other sensors. Thanks.

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 174 at r3 (raw file):

Previously, bloodlee (Bloodlee) wrote…

not be the first one?
Yes, you are right. I will adjust it.

Done.

[guwirth]

@bloodlee

And "addFlowElement" is not working

The CxxClangSASensor.java is also using addFlowElement:
https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/cxx-sensors/src/main/java/org/sonar/cxx/sensors/clangsa/CxxClangSASensor.java#L189

If the addFlowElement is not generic enough we have to change it in the base class.

https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxIssuesReportSensor.java#L215

Issue: Clang Static Analyzer: add support for full path/flow to a given issue #1707
PR: Add path/flow support for Clang Static Analyzer *.plist reports #1710

Think you can replace the "> 1" with "> 0" and fix the test.

[bloodlee]

That would be great! I will make this change.

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

[guwirth]

@romanek-adam do you remember why there is this > 1?
https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxIssuesReportSensor.java#L215

[bloodlee]

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 191 at r2 (raw file):

Previously, bloodlee (Bloodlee) wrote…

The paths in pclint report are relative ones to the base dir. Basically, it needs pclint check and sonar-scanner run in the same base dir.

I will double-check the API usage in other sensors. Thanks.

Hi @ivangalkin,

I checked the API usage in Valgrind sensor (CxxValgrindSensor.java:71). I don't quite understand the logic of Valgrind report but I think it's different from the case I am implementing.

In Valgrind, it's checking if the "file path" (I think) of the frame in the project context or not. If the location is unknown or it's not in the project context, re-map the "file path" to the one of the "last own frame".

The logic in PCLint sensor is slightly different. The supplemental message of PC-lint is to let the reviewer know how the primary issue was caught. Since PClint report generation and sonar-scanner can have different scope, I think no matter if the "file path" in the supplemental message is in project context or not, we put it in the secondary location or flow. Re-mapping the file path only happens when the file path in a supplemental message is different from the primary issue's path.

Not sure if I understand the case clearly, let me know.

Thanks.

[guwirth]

The logic in PCLint sensor is slightly different.

@bloodlee think there are two poinzs to take into account:

1. What happens if you add an flow item which is outside of the module (no source exists)? Does this work or do you get an exception.
2. What do you/the user see in the UI? If you click an flow issue with no source what happens?

[romanek-adam]

@romanek-adam do you remember why there is this > 1?
https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxIssuesReportSensor.java#L215

Yes, I do. I tried to document it in the code as a comment but... it's only useful to report a flow when it has more than one element, like on this screenshot: https://pbs.twimg.com/media/C-Z53ywXcAAhJzA.jpg:large . Here the flow has 4 elements. Otherwise when there is just one element in a given flow we would get just one step in a flow on SonarQube UI which is a bit misleading and redundant. We would see red squares with "1" in them in a few places on the UI. IMO such issues without a multi-step path are expected to be presented on SonarQube UI like this: https://www.ndepend.com/Doc/CI_SonarQube/BrowseNDependIssuesInSQUI.png (without these red squares indicating the number of steps on the flow/path).

(I took these screenshots from Google, with no particular preference, just to show the idea)

@guwirth does this answer your question?

[guwirth]

@romanek-adam tanks for your fast response.

• What I understood: The real value of the flow issues are these 1,2,3... in the code to give more details to the user. In the left hand panel I see the corresponding messages.
• What's still a question for me: If I add a message and then flow issue(s), there is the original issue (in the red box) and also this first flow item (in your case: 'visitTerminal() returns null) provides additional informaton? So I would not remove it?

The other discussion which is ongoing is: Does it make sense to add issues (locations) to the flow where no source code is available (outside of the project)? What do you think?

[romanek-adam]

@guwirth
ad 1. correct, the messages also appear on the source code pane, when you click a specific message from the list of steps on the left
ad 2. Clang Static Analyzer provides two types of outputs

• one which contains the full path to a given issue (like for an uninitialized variable - first when the variable is declared, then possibly some conditionals if statements, function calls etc., and finally the location when the uninitialized variable is actually used)
• another one which provides a specific location in the code, just a single line (like for dead stores, i.e. when a variable is declared and initialized on a given line and not used anywhere else) - in this case CSA provides a description like "Value stored to 'result' during its initialization is never read"; this description is presented on SonarQube UI in a red block both on the issues list on the left and in-line on the source code pane; there is no need to add flow steps, even just one because the report (the .plist) file does not provide any additional information; uff, quite hard to explain without a specific example to refer to

Regarding flows for issues where there is no source code is available - I'm not sure SonarQube would accept such issues/flows - I'd expect an exception to be thrown but maybe they are silently skipped. Or maybe I'm wrong and SonarQube is actually able to present such issues/flows somehow, even without source code(?) - this needs to be verified through experimentation.

[guwirth]

@romanek-adam thanks for your explanation.

Looking to #1720 (clang-tidy) there it is clear what is the warning and what is the flow data (labeled note). Wondering if this is not the same in clang sa reports? Could you provide a report (plist) with a flow? In our repository I found only this report: https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/cxx-sensors/src/test/resources/org/sonar/cxx/sensors/reports-project/clangsa-reports/clangsa-report.plist.

[bloodlee]

@guwirth, for your questions,

1. What happens if you add an flow item which is outside of the module (no source exists)? Does this work or do you get an exception.

It will work. If the supplemental message points to a different file, which the primary location points to, I will remap the file path.

2. What do you/the user see in the UI? If you click an flow issue with no source what happens?

If the flow item or secondary item points to different file, the sonarqube UI will exclude those items. Nothing can see on UI. That's also why I do the remapping.

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

[bloodlee]

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/pclint/CxxPCLintSensor.java, line 191 at r2 (raw file):

Previously, bloodlee (Bloodlee) wrote…

Hi @ivangalkin,

I checked the API usage in Valgrind sensor (CxxValgrindSensor.java:71). I don't quite understand the logic of Valgrind report but I think it's different from the case I am implementing.

In Valgrind, it's checking if the "file path" (I think) of the frame in the project context or not. If the location is unknown or it's not in the project context, re-map the "file path" to the one of the "last own frame".

The logic in PCLint sensor is slightly different. The supplemental message of PC-lint is to let the reviewer know how the primary issue was caught. Since PClint report generation and sonar-scanner can have different scope, I think no matter if the "file path" in the supplemental message is in project context or not, we put it in the secondary location or flow. Re-mapping the file path only happens when the file path in a supplemental message is different from the primary issue's path.

Not sure if I understand the case clearly, let me know.

Thanks.

Done.

[bloodlee]

@guwirth,

And please check the function CxxIssuesReportSensor.saveViolation, when saving an issue with secondary locations or flow items, if the file is outside of the module (not in the context), the location will be dropped. So the case described in question #1 will not happen.

Thanks.

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

[romanek-adam]

(...) In our repository I found only this report: https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/cxx-sensors/src/test/resources/org/sonar/cxx/sensors/reports-project/clangsa-reports/clangsa-report.plist.

Actually this report contains an issue with a two-element flow. I was using a helper script to pretty print the plist file so that I could examine the structure more easily, see https://gist.github.com/romanek-adam/70b4534a780e5cb624fd982b0b3fdc8f. The JSON formatted report can be found here: https://gist.github.com/romanek-adam/68e9bc3c62538ff15dff200a8365d2df

The code in CxxClangSASensor.java only parses path elements of kind 'event' which provide the exact location of a given flow step. The remaining elements of kind 'control' provide additional information which I wasn't able to map to SonarQube APIs easily.

In the report example two elements of type 'event' exist:

•            {
                "depth": 0,
                "kind": "event",
                "message": "'a' declared without an initial value",
                "location": {
                    "col": 5,
                    "file": 0,
                    "line": 31
                },
                "extended_message": "'a' declared without an initial value",
                "ranges": [
                    [
                        {
                            "col": 5,
                            "file": 0,
                            "line": 31
                        },
                        {
                            "col": 9,
                            "file": 0,
                            "line": 31
                        }
                    ]
                ]
            },
  

•            {
                "depth": 0,
                "kind": "event",
                "message": "Branch condition evaluates to a garbage value",
                "location": {
                    "col": 8,
                    "file": 0,
                    "line": 32
                },
                "extended_message": "Branch condition evaluates to a garbage value",
                "ranges": [
                    [
                        {
                            "col": 8,
                            "file": 0,
                            "line": 32
                        },
                        {
                            "col": 8,
                            "file": 0,
                            "line": 32
                        }
                    ]
                ]
            }
  

This is enough to form a flow.

[bloodlee]

@guwirth,

Do you prefer to use flow or secondary locations?

Thanks

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

[guwirth]

Do you prefer to use flow or secondary locations?

@bloodlee If you want to add a flow you should use the flow method. I'm still wondering why addLocation is working? Did you verify this in the UI? I still had not the time to verify the differences between flow and additional location (I mean not our implementation in the plugin, I mean the SQ API differences). Or is it at the end the same?

[bloodlee]

Q API differences). Or is it at the end the same?

Yes. it has been verified in UI and it's working well. But I still prefer to use flow, which makes more sense. Even the flow has only one location, it shows how the issue is caught. Secondary locations sound confusing.

About the difference between flow and secondary location, my understanding is secondary locations will be converted into flows and each flow has only one location. Please check my update in CxxPCLintSensorTest.java:L204.

Actually, it bypasses the guard (>1) in CxxReportSensor.java, which also means there is no such limitation (one flow should have more than one location) in SQ.

I will make some update and use API to add flow.

Thanks.

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

[bloodlee]

BTW, on the UI, I couldn't feel differences on behavior on flow and secondary locations.

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @guwirth and @ivangalkin)

[bloodlee]

Sorry about the UT failure. I will fix it soon.

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @bloodlee, @guwirth, and @ivangalkin)

[guwirth]

@bloodlee thanks for providing this. For me it's ok I would merge it. In general there are some questions open with addLocation and addFlow. But I think we should align it as part of #1743.

[bloodlee]

Thanks for your approval, @guwirth. I am glad to contribute my work to this project.

I will also keep an eye on #1743.

Nice weekend!

Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @bloodlee, @guwirth, and @ivangalkin)