[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)

Wed Jun 5 08:59:21 PDT 2024

NagyDonat wrote:

I analyzed several open source project with this check to observe the effects of my commit and I was (pleasantly?) surprised to see that it detected some ugly errors (despite the fact that the inputs are stable open source projects...).

The following table compares the "old" state before this commit and the "new" state where this commit is active and the freshly added option `WarnOnSizeOfPointer` is set to true (instead of the default "false"):
| Project | New Reports | Resolved Reports | Notes | 
|---------|-------------|------------------|-------|
| memcached | No reports | No reports | – 
| tmux | No reports | [23 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_2.6_old_sizeofexpressions&newcheck=tmux_2.6_new_sizeofexpressions&diff-type=Resolved) | reports seem to be FPs, including several ones that [use `qsort` in a clear and straightforward way](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_old_sizeofexpressions&newcheck=tmux_2.6_new_sizeofexpressions&diff-type=Resolved&report-id=5493278&report-hash=e1dd82bffcf68169ff8fe7181ca44f16&report-filepath=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Ftmux%2Fwindow-buffer.c)
| curl | [3 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions&newcheck=curl_curl-7_66_0_new_sizeofexpressions&diff-type=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions&newcheck=curl_curl-7_66_0_new_sizeofexpressions&diff-type=Resolved) | new reports are TPs (all reporting incorrect use of the same data structure), resolved one is FP
| twin | No reports | No reports | –
| vim | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=vim_v8.2.1920_old_sizeofexpressions&newcheck=vim_v8.2.1920_new_sizeofexpressions&diff-type=New) | No reports | true positive 
| openssl | [33 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions&newcheck=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions&diff-type=New) | [32 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions&newcheck=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions&diff-type=Resolved) | ...
| sqlite | [19 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions&newcheck=sqlite_version-3.33.0_new_sizeofexpressions&diff-type=New) | [8 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions&newcheck=sqlite_version-3.33.0_new_sizeofexpressions&diff-type=Resolved) | among the new results there are many FPs ([(1)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions&newcheck=sqlite_version-3.33.0_new_sizeofexpressions&diff-type=New&report-id=5493379&report-hash=f411835e93b1711c2889d4bef2889db9&report-filepath=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fshell.c), [(2)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions&newcheck=sqlite_version-3.33.0_new_sizeofexpressions&diff-type=New&report-id=5493385&report-hash=d9e3d0a984913130c821b7c18c2cc8d2&report-filepath=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fsqlite3.c)) that do things like `char **mem; realloc(mem, numElements*sizeof(mem[0]))`; the resolved reports are FPs and they reappear among the new reports with a different message
| ffmpeg | [31 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions&newcheck=ffmpeg_n4.3.1_new_sizeofexpressions&diff-type=New) | [118 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions&newcheck=ffmpeg_n4.3.1_new_sizeofexpressions&diff-type=Resolved) | ...
| postgres | [2 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions&newcheck=postgres_REL_13_0_new_sizeofexpressions&diff-type=New) | [7 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions&newcheck=postgres_REL_13_0_new_sizeofexpressions&diff-type=Resolved) | resolved reports are mostly FPs, two of them (one FP + one "technically works, but stupid" code) reappear as "new" reports with the new message
| tinyxml2 | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_old_sizeofexpressions&newcheck=tinyxml2_8.0.0_new_sizeofexpressions&diff-type=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_old_sizeofexpressions&newcheck=tinyxml2_8.0.0_new_sizeofexpressions&diff-type=Resolved)  | same FP is reported with the new message
| libwebm | No reports | No reports | –
| xerces | [16 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_old_sizeofexpressions&newcheck=xerces_v3.2.3_new_sizeofexpressions&diff-type=New) | [15 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_old_sizeofexpressions&newcheck=xerces_v3.2.3_new_sizeofexpressions&diff-type=Resolved) 
| bitcoin | [4 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_old_sizeofexpressions&newcheck=bitcoin_v0.20.1_new_sizeofexpressions&diff-type=New) | [3 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_old_sizeofexpressions&newcheck=bitcoin_v0.20.1_new_sizeofexpressions&diff-type=Resolved) 
| protobuf | [9 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=protobuf_v3.13.0_old_sizeofexpressions&newcheck=protobuf_v3.13.0_new_sizeofexpressions&diff-type=New) | [5 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=protobuf_v3.13.0_old_sizeofexpressions&newcheck=protobuf_v3.13.0_new_sizeofexpressions&diff-type=Resolved) 
| qtbase | [43 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_old_sizeofexpressions&newcheck=qtbase_v6.2.0_new_sizeofexpressions&diff-type=New) | [33 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_old_sizeofexpressions&newcheck=qtbase_v6.2.0_new_sizeofexpressions&diff-type=Resolved) 
| contour | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=contour_v0.2.0.173_old_sizeofexpressions&newcheck=contour_v0.2.0.173_new_sizeofexpressions&diff-type=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=contour_v0.2.0.173_old_sizeofexpressions&newcheck=contour_v0.2.0.173_new_sizeofexpressions&diff-type=Resolved) 
| openrct2 | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openrct2_v0.4.8_old_sizeofexpressions&newcheck=openrct2_v0.4.8_new_sizeofexpressions&diff-type=New) | No reports 
| llvm-project | [37 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=llvm-project_llvmorg-12.0.0_old_sizeofexpressions&newcheck=llvm-project_llvmorg-12.0.0_new_sizeofexpressions&diff-type=New) | [38 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=llvm-project_llvmorg-12.0.0_old_sizeofexpressions&newcheck=llvm-project_llvmorg-12.0.0_new_sizeofexpressions&diff-type=Resolved) 

The "Resolved" reports are mostly discarded by the new "do not report `sizeof(*pp)` where `pp` is a pointer-to-pointer" heuristic (which would be enabled even if `WarnOnSizeOfPointer` was disabled); on the other hand, the "New" reports are mostly specific to the new, more aggressive mode `WarnOnSizeOfPointer` and would not appear with the (proposed) default settings of this check.

Moreover, it seems that the diffing done by CodeChecker handles the old message ("suspicious usage of 'sizeof(A*)'; pointer to aggregate") and the new messages (or at least "suspicious usage of 'sizeof(A*)' on pointer-to-aggregate type; did you mean 'sizeof(A)'? ") as different reports, so there are several reports that appear on both sides of this diff table.

NOTE: I started to review and summarize the reports, but I wasn't able to finish it. I'll continue this tomorrow and edit this comment to add the summaries for openssl, ffmpeg and the second half of the table.

https://github.com/llvm/llvm-project/pull/94356