[analyzer] Add flags to log_parser.py #3433
Conversation
waqas-mazhar
requested review from
bruntib and
csordasmarton
as code owners
waqas-mazhar
force-pushed
the
log-parser-update
branch
from
fb1c91e to
c4a20ee
Compare
csordasmarton
added
the
analyzer 📈
| '-fgcse-lm', | ||
| '-fhoist-adjacent-loads', | ||
| '-findirect-inlining', | ||
| '-finline-limit', | ||
| '-finline-local-initialisers', | ||
| '-fipa-sra', | ||
| '-fmacro-prefix-map', |
There was a problem hiding this comment.
From clang 10.0.0 this flag is already supported: https://reviews.llvm.org/D49466. So maybe it is not a good idea to skip this flag in CodeChecker but to update your clang version in this case. What do you think @bruntib?
There was a problem hiding this comment.
Unfortunately, currently it is an "unwritten rule" that we support all versions of the analyzers. This is the concept of our checker labels too: we list all checkers ever existed in clang even with their old names if they were renamed. We have been following the same rule with this log parser: we skip flags if any clang version doesn't support them, but we have never removed any of these from the list just because Clang started to support it.
I agree that this is not a good approach. But I think this will not be solved nowadays. We were planning to create a 3pp tool that handles these compiler flags so that tool can also be used in other projects like CodeCompass. @gamesh411 started its development. I think this logic with different clang versions could be implemented in this tool if we get there in the future. As part of that project we could also review if any of these skipped flags are still unsupported by the latest clang.
There was a problem hiding this comment.
I am keeping this flag in the changeset unless unanimously advised otherwise.
| '-fno-extended-identifiers', | ||
| '-fno-jump-table', | ||
| '-fno-keep-static-consts', | ||
| '-fno-reorder-functions', |
There was a problem hiding this comment.
Note: on the Phabricator there is a patch for clang to support this flag but as far as I see it is not merged yet and still under review: https://reviews.llvm.org/D34796.
There was a problem hiding this comment.
Thanks for your response @csordasmarton. I have removed this flag altogether because it is already covered by the flag in the next line -f(no-)?reorder-functions
waqas-mazhar
force-pushed
the
log-parser-update
branch
from
c4a20ee to
e2dffd7
Compare
| @@ -220,7 +223,7 @@ | |||
| '-[DIUF]', | |||
| '-idirafter', | |||
| '-isystem', | |||
| '-macros', | |||
| '-imacros', | |||
There was a problem hiding this comment.
This flag is fixed by #3428 and it is merged. It is very interesting that github doesn't recognise this as a merge conflict but shows this as a change. Can you please rebase your branch to hide this change from this commit? Otherwise this patch looks good to me.
There was a problem hiding this comment.
Rebased and commit message corrected.
Add -mfp16-format, -fmacro-prefix-map and -fno-defer-pop to IGNORED_OPTIONS_GCC as they are not recognized by clang.
waqas-mazhar
force-pushed
the
log-parser-update
branch
from
e2dffd7 to
43c7e9e
Compare
Add -mfp16-format, -fmacro-prefix-map and -fno-defer-pop
to IGNORED_OPTIONS_GCC as they are not recognized by clang.