This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add bugprone-placement-new-target-type-mismatch check
Needs ReviewPublic

Authored by DennisL on Apr 2 2019, 10:01 AM.

Details

Summary

Finds placement-new calls where the pointer type of the adress mismatches the type of the created value

Diff Detail

Event Timeline

DennisL created this revision.Apr 2 2019, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 10:01 AM
Eugene.Zelenko retitled this revision from Add clang-tidy/checks/misc-placement-new-target-size check to [clang-tidy] Add misc-placement-new-target-size check.Apr 2 2019, 11:12 AM
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added inline comments.
clang-tidy/misc/PlacementNewTargetSizeCheck.cpp
11

Unnecessary empty line. Please run Clang-format after fixing.

24

Please use static instead of anonymous namespaces. See LLVM Coding Guidelines.

27

Please don't use auto where type could not be easily deduced.

29

Please don't use auto where type could not be easily deduced.

48

auto could be used here, because of cast.

51

Please don't use auto where type could not be easily deduced.

56

Please don't use auto where type could not be easily deduced.

63

auto could be used here, because of cast.

64

Please don't use auto where type could not be easily deduced.

66

Please run Clang-format.

74

Unnecessary empty line.

docs/ReleaseNotes.rst
135

Please add one sentence description. Should be same as in documentation.

docs/clang-tidy/checks/misc-placement-new-target-size.rst
6

Unnecessary empty line.

DennisL updated this revision to Diff 193449.Apr 3 2019, 2:19 AM
DennisL marked 13 inline comments as done.

Updated patch to address reviewer feedback

DennisL updated this revision to Diff 193454.Apr 3 2019, 2:28 AM

Remove debug output

DennisL updated this revision to Diff 193471.Apr 3 2019, 4:59 AM

Simplify logic

Eugene.Zelenko added inline comments.Apr 3 2019, 7:06 AM
docs/clang-tidy/checks/misc-placement-new-target-size.rst
6

Somehow empty line is still there.

DennisL updated this revision to Diff 193715.Apr 4 2019, 8:05 AM
DennisL marked an inline comment as done.
DennisL retitled this revision from [clang-tidy] Add misc-placement-new-target-size check to [clang-tidy] Add misc-placement-new-target-type-mismatch check.
DennisL edited the summary of this revision. (Show Details)

Addressed reviewer feedback and also simplified the implementation, renamed the test to be more concise, and added more test cases.

Eugene.Zelenko added inline comments.Apr 4 2019, 9:43 AM
docs/ReleaseNotes.rst
136

Please synchronize with documentation.

DennisL updated this revision to Diff 193839.Apr 5 2019, 12:16 AM
DennisL marked an inline comment as done.

Sync ReleaseNotes.rst with docs

alexfh added a comment.Apr 5 2019, 5:54 AM

Looks like this check would fit better into the bugprone module.

Looks like this check would fit better into the bugprone module.

Excellent suggestion. Thanks. Will follow up with an updated Diff.

DennisL updated this revision to Diff 194135.Apr 8 2019, 6:49 AM

The following has been updated:

  • moved check to bugprone instead of misc
  • more tests
  • rewritten check logic
DennisL updated this revision to Diff 194136.Apr 8 2019, 6:52 AM
DennisL retitled this revision from [clang-tidy] Add misc-placement-new-target-type-mismatch check to [clang-tidy] Add bugprone-placement-new-target-type-mismatch check.

Removed debug output

DennisL updated this revision to Diff 194137.Apr 8 2019, 6:54 AM
DennisL updated this revision to Diff 194283.Apr 9 2019, 3:23 AM
  • handle array to ptr decay
  • updated error msg
  • additional tests for arry to ptr decay

Hey Dennis,

my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand this to be checked.

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp
21 ↗(On Diff #194283)

Please write full sentences with punctuation in comments.

22 ↗(On Diff #194283)

placement-new? please make a new matcher (locally) to match that, because its is more specific.
You can use other checks as example, grep for "AST_MATCHER"

30 ↗(On Diff #194283)

Please ellide braces.

38 ↗(On Diff #194283)

braces, above the comment does not follow the correct-sentence style

42 ↗(On Diff #194283)

Is this universally true? What about the nothrow-overload, would that interfere?

45 ↗(On Diff #194283)

QualType is usually used a value-type, not const-ref. Please follow that for consistency.

50 ↗(On Diff #194283)

braces.

docs/clang-tidy/checks/bugprone-placement-new-target-type-mismatch.rst
7 ↗(On Diff #194283)

Please add examples to demonstrate good and bad code for the user.

test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp
5 ↗(On Diff #194283)

Please add a test for the nothrow overload.

86 ↗(On Diff #194283)

Please add test, where everything is hidden behind templates and only the type-substitution leads to the error.
A test including macros helps as well, to show they do not interfere (as should be the case with this check).

Eugene.Zelenko added inline comments.Apr 9 2019, 11:45 AM
clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp
38 ↗(On Diff #194283)

I would put nullptr as second argument for == operator.

DennisL marked 12 inline comments as done.Apr 10 2019, 6:35 AM

Hey Dennis,

my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand this to be checked.

Thanks for the great suggestions. Updated the diff according to the feedback. Also checked with cppcoreguidelines, hicpp as well as cert. Only cert has a related, yet different rule stating that calls to placement new shall be provided with properly aligned pointers. I'd say this should be a distinct check. Happy to work on it after this one.

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp
42 ↗(On Diff #194283)

Thanks, rewrote that check.

DennisL updated this revision to Diff 194505.Apr 10 2019, 6:39 AM
DennisL marked an inline comment as done.

Changed the following

  • addressed reviewer feedback
  • fetch the placement parameter as written
  • add further test cases as requested
  • stylistic changes
  • add nothrow parameter support
  • ignore matches with unresolved template parameters
  • add examples of bad and good code

Hey Dennis,

my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand this to be checked.

Thanks for the great suggestions. Updated the diff according to the feedback. Also checked with cppcoreguidelines, hicpp as well as cert. Only cert has a related, yet different rule stating that calls to placement new shall be provided with properly aligned pointers. I'd say this should be a distinct check. Happy to work on it after this one.

What is the rationale for separating those checks? It sort of feels like there is some overlap between alignment, size, and buffer type.

I'm not certain I agree with the way this check operates -- what problem is it trying to solve? For instance, this code would trigger on this check, but is perfectly valid code:

static_assert(sizeof(int) == sizeof(float));
static_assert(alignof(int) == alignof(float));
int buffer;
float *fp = new (&buffer) float;

The situations I can think of where the type mismatch is an issue would be for types with nontrivial special members, so there are times when this check makes sense, but perhaps it's too restrictive currently, but you may have a different problem in mind?

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp
20 ↗(On Diff #194505)

This should match on CXXNewExpr instead of Expr, then you won't need the dyn_cast below.

29 ↗(On Diff #194505)

This should probably use cxxNewExpr() instead of expr() so you match on something more specific.

38 ↗(On Diff #194505)

This assert message looks a bit incomplete. ;-)

43 ↗(On Diff #194505)

You can use const auto * here.

test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp
56 ↗(On Diff #194505)

While this code definitely has a bug from the buffer overflow, I don't think the diagnostic is valuable here. This is a very common pattern and I suspect this will yield a lot of false positives, unless the check starts taking alignment and buffer size into consideration.