This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck.
ClosedPublic

Authored by etienneb on Apr 6 2016, 6:38 PM.

Details

Summary

This is the same kind of bug than D18238.

Fix crashes caused by deferencing null pointer when declarations parsing may be delayed.
The body of the declarations may be null.

The crashes were observed with a Windows build of clang-tidy and the following command-line.

command-line switches: -fms-compatibility-version=19 -fms-compatibility

Diff Detail

Event Timeline

etienneb updated this revision to Diff 52877.Apr 6 2016, 6:38 PM
etienneb retitled this revision from to [clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck..
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.

That we've had to fix this twice now tells me that our collective memory is forgetful :).

We need to collect the community wisdom for clang-tidy gotchas somewhere in the docs...

There are probably other instances of this error.
It's not an instance that got incorrectly fixed.

I may review other checkers to see if I can find more of them.

For now, I'm fixing them when they make clang-tidy crash over chromium code (with a clang-tidy windows build).

The fact that 'hasBody()' may be true and 'getBody()' can return NULL is a little bit strange to me.

etienneb updated this object.Apr 6 2016, 7:53 PM
kimgr added a subscriber: kimgr.Apr 7 2016, 12:59 AM

A test case would be nice here. You can repro on all systems by passing -fdelayed-template-parsing explicitly.

alexfh edited edge metadata.Apr 7 2016, 2:02 AM

Please add a test file with -fms-compatibility or -fdelayed-template-parsing.

alexfh requested changes to this revision.Apr 7 2016, 2:02 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 7 2016, 2:02 AM
etienneb updated this revision to Diff 52915.Apr 7 2016, 7:28 AM
etienneb edited edge metadata.

add unittests.

alexfh accepted this revision.Apr 7 2016, 7:41 AM
alexfh edited edge metadata.

LG

This revision is now accepted and ready to land.Apr 7 2016, 7:41 AM
etienneb closed this revision.Apr 7 2016, 8:03 AM
kimgr added a comment.Apr 7 2016, 11:05 AM

The RUN line looks weird to me, but I'm no lit expert...

test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
1

Are the double -- necessary?

etienneb marked an inline comment as done.Apr 7 2016, 11:12 AM
etienneb added inline comments.
test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
1

Yes, this is fine. See other *delayed.cpp test.

Example:

// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t -- -- -fdelayed-template-parsing

kimgr added inline comments.Apr 7 2016, 11:26 AM
test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
1

Oh, I see now that it's a peculiarity of check_clang_tidy.py. Sorry for the noise!