The threadsafety.py addon now flags MT-Unsafe symbols and functions.#5086
Conversation
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
how is the test file supposed to be used and what is the expected output.
I suggest that you add a test execution in the .github/workflows/CI-unixish.yml file.
We have this testing in that file:
...
# TODO: move to scriptcheck.yml so these are tested with all Python versions?
- name: Test addons
run: |
./cppcheck --addon=threadsafety addons/test/threadsafety
./cppcheck --addon=threadsafety --std=c++03 addons/test/threadsafety
...
I don't know it does not seem to enforce that the expected warnings are printed. Imho we should add --error-exitlevel=1 --inline-suppr to the commands and then in the test files such comments should be added where warnings are expected:
--cppcheck-suppress threadsafety-the-warning-id
Since addons/test/threadsafety is a directory, the existing lines in CI-unixish.yml do exercise my new testfile.
With --error-exitcode=1 that looks right - I haven't tried it in the CI yet. |
yes please add --error-exitcode=1 for all the testfiles and add proper cppcheck-suppress comments in the files. |
|
I have added --error-exitcode=1 --inline-suppr I will look at your other suggestions and see whether I can implement them. |
In C++11 and later, initialization of static objects is thread safe. hmm so somehow we would like to test that there is no warning in local_static.cpp normally. Only if --std=c++03 we want to have warnings. |
andrew-aitchison
force-pushed
the
MT_unsafe
branch
from
0be8ace to
6b9fac4
Compare
good it was closed by mistake not by intention :-) |
tools/MT-Unsafe.py generates a list of these MT-Unsafe symbols for this addon, by reading the "Attributes" sections of man pages, and addons/test/threadsafety/MT-Unsafe.cpp exercises the threadsafety.py addon.
…he number of arguments. Fix it. Originally changed to appease quake8 line length warning.
--error-exitcode=1 --inline-suppr so that they are quiet if the result is as expected, but the return code indicates if something unexpected happens.
with comments that cppcheck understands, so that it can be silent about them.
…at cppcheck understands, so that it can be silent about them.
…onst.cpp
correctly test the threadsafety addon.
- they were copied over from the y2038 addon.
Move argument parsing after function definitions, and describe each function. Appeases flake8.
andrew-aitchison
force-pushed
the
MT_unsafe
branch
from
f3c8790 to
18306e4
Compare
|
On Mon, 29 May 2023, Daniel Marjamäki wrote:
@danmar commented on this pull request.
> + # if arg.startswith('-'):
+ # continue
+
+ if not args.quiet:
+ print('Checking ' + dumpfile + '...')
+
+ # load XML from .dump file
+ data = cppcheckdata.CppcheckData(dumpfile)
+ # data = cppcheckdata.CppcheckData(args)
+
+ # Convert dump file path to source file in format generated by
+ # cppcheck. For example after the following call:
+ # cppcheck ./src/my-src.c --dump
+ # We got 'src/my-src.c' value for 'file' field in cppcheckdata.
+ srcfile = dumpfile.rstrip('.dump')
+ srcfile = os.path.expanduser(srcfile)
hmm.. this conversion does not seem to work if a cppcheck build dir is used.
Hmm. The code is the same as addons/y2038.py
In a couple of other projects I have been using the
cppcheck --suppress=missingInclude:: --template=cppcheck1
--cppcheck-build-dir=.cppcheck
--addon=threadsafety.py
--addon=y2038.py <...>
without noticing any issues with the conversion.
I *do* get reports of problems from these addons.
What do you see ?
os.path.expanduser(srcfile) could obviously depend upon the platform:
I am using Ubuntu. I don't use the gui, and I use the old template since
the new one doesn't suit my color vision.
… The top of a dump file should look something like this:
```
<?xml version="1.0"?>
<dumps language="c">
<platform name="native" char_bit="8" short_bit="16" int_bit="32" long_bit="64" long_long_bit="64" pointer_bit="64"/>
<rawtokens>
<file index="0" name="1.c"/>
```
Spontanously I think it's better to read `srcfile` from the
`<file ..>` element that has `index="0"`.
How you access this information in the easiest way from cppcheckdata.py
I don't know off the top of my head.
--
Andrew C. Aitchison Kendal, UK
***@***.***
|
The intention of the code is that srcfile will be src/my-src.c But when the dump file is created in .cppcheck, the srcfile will more likely be something like .cppcheck/my-src.c You only use it to print some "Checking.. " message as far as I see but that message will be wrong.. |
|
On Wed, 31 May 2023, Daniel Marjamäki wrote:
> Hmm. The code is the same as addons/y2038.py
cppcheck --suppress=missingInclude:: --template=cppcheck1
--cppcheck-build-dir=.cppcheck
--addon=threadsafety.py
--addon=y2038.py
src/my-src.c
The intention of the code is that srcfile will be src/my-src.c
But when the dump file is created in .cppcheck, the srcfile will more likely be something like .cppcheck/my-src.c
You only use it to print some "Checking.. " message as far as I see but that message will be wrong..
Looking at the source code I would expect that too,
but the paths displayed in the output are the ones we want ... :
# cppcheck --cppcheck-build-dir=qct/.cppcheck --addon=y2038.py qct/qctdataset.cpp
Checking qct/qctdataset.cpp ...
qct/qctdataset.cpp:3074:31: warning: ctime_r is Y2038-unsafe [y2038-unsafe-call]
ctime_r(&poDS->nOrigFileTime, timebuf), "");
^
…--
Andrew C. Aitchison Kendal, UK
***@***.***
|
The "Checking qct/qctdataset.cpp is written by Cppcheck so it does not come from the addon at all. The "qct/qctdataset.cpp:3074:31: warning: ctime_r is Y2038-unsafe [y2038-unsafe-call]" comes from the addon but I believe it takes the token filename not srcfile |
There is something wrong earlier: I copied the code from y2038.py on the assumption that it was correct, without understanding it; |
|
well I merge this and then try to clean it up.. |
ok well the problem with that is a permission problem. Cppcheck tries to create the file /usr/include/c++/12/stdatomic.h.dump but likely you don't have proper permissions for that (good!). Cppcheck should have written a better error message when there was permission error. Also I have the feeling it's a bad idea to create the dump files in the source folder I am thinking about creating temporary files instead.. I wrote a point about that in https://trac.cppcheck.net/ticket/11750 |
|
For information: #5132 |
2 participants
At least on linux, many man pages have an "Attributes" section which lists MT-Unsafe (multi-thread unsafe) symbols
in a standard format.
This pull request extends the existing addon threadsafety.py to warn when these symbols are used.
It includes the tool tools/MT-Unsafe.py to re-generate the list of symbols by reading the (troff) man pages,
and a cpp test file which uses some symbols which these man pages list as safe or unsafe.