Page MenuHomePhabricator

[clang-tidy] Add incorrect-pointer-cast checker
Needs ReviewPublic

Authored by steakhal on Jul 3 2018, 12:24 AM.

Details

Summary

This checker warns for cases when pointer is cast and the pointed to type is incompatible with allocated memory area type.
This may lead to access memory based on invalid memory layout.

Warn for cases when the pointed to type is wider than the allocated type.
For example char vs integer, long vs char etc.
Also warn for cases when the pointed to type layout is different from the allocated type layout, like different structs, integer vs float/double, different signedness.

Allows pointer casts if the pointed to struct type is "part" of the allocated type.
Which means the allocated type contains the pointed to type member by member.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Eugene.Zelenko added inline comments.Jul 3 2018, 8:16 AM
clang-tidy/misc/IncorrectPointerCastCheck.cpp
35 ↗(On Diff #153866)

const auto * could be used instead.

clang-tidy/misc/IncorrectPointerCastCheck.h
19 ↗(On Diff #153866)

Please make sentence same as in Release Notes.

docs/clang-tidy/checks/misc-incorrect-pointer-cast.rst
6 ↗(On Diff #153866)

Please make this sentence same as in Release Notes.

alexfh added a comment.Jul 4 2018, 9:53 AM

Some patterns are covered by compiler diagnostics: https://godbolt.org/g/HvsjnP. Is there any benefit in re-implementing them?

clang-tidy/misc/IncorrectPointerCastCheck.cpp
60 ↗(On Diff #153866)

The style of the message differs from that of the other checks (and Clang warnings). Messages are not complete sentences and as such should not use capitalization, trailing periods etc. Also I would avoid imperatives and exclamation marks. The message should use neutral tone and state what exactly is wrong with the code and why.

test/clang-tidy/misc-incorrect-pointer-cast.cpp
109 ↗(On Diff #153866)

"No newline at end of file". Please fix.

Eugene.Zelenko added inline comments.Oct 12 2018, 6:42 PM
clang-tidy/misc/IncorrectPointerCastCheck.cpp
80 ↗(On Diff #153866)

Somehow, misc namespace is closed twice.

docs/ReleaseNotes.rst
60 ↗(On Diff #153866)

Will be good idea to rebase from trunk and use alphabetical order.

63 ↗(On Diff #153866)

Warns. Please also use as much as possible from 80 characters.

docs/clang-tidy/checks/misc-incorrect-pointer-cast.rst
10 ↗(On Diff #153866)

Looks like two paragraphs are almost same. Will be good idea to rephrase. Please make sure that lines are no longer then 80 characters.

hgabii updated this revision to Diff 170360.Oct 21 2018, 3:32 PM

Warning messages changed.
Tests updated.
Comments changed based on the recommendations.
Documentation refactored and reformatted.

Warning messages changed.
Tests updated.
Comments changed based on the recommendations.
Documentation refactored and reformatted.

Could you mark comments "done" where appropriate?

hgabii marked 9 inline comments as done.EditedNov 20 2018, 12:25 PM

I resolved all comments.
(Sorry, I did not recognize that the done state is updating only when I submit a new comment)

docs/ReleaseNotes.rst
60 ↗(On Diff #153866)

It is rebased. I could not see what should be in an alphabetical order. This is inserted to the top of the document by the add_new_check.py.

Warn for cases when the pointed to type is wider than the allocated type.
What does that mean? Is this talking about the size of the allocation, or the alignment?

Are you aware of the previous attempts, namely D33826?
Please incorporate the tests from there.

Please add tests with casts (that should normally be diagnosed) from types with explicitly specified alignment, that silence the warning.

Eugene.Zelenko added inline comments.Nov 20 2018, 1:00 PM
docs/ReleaseNotes.rst
60 ↗(On Diff #153866)

List of new check should be sorted alphabetically. Unfortunately add_new_check.py doesn't do this, so manual edit is necessary.

gerazo added a subscriber: gerazo.Nov 26 2018, 7:52 AM

In my opinion, after migrating relevant test cases from D33826, this is ready.

hgabii updated this revision to Diff 177810.Dec 11 2018, 6:26 PM
hgabii marked an inline comment as done.
Eugene.Zelenko added inline comments.Dec 11 2018, 6:30 PM
docs/ReleaseNotes.rst
139 ↗(On Diff #177810)

Please wrap up to 80 characters. Same in documentation.

docs/clang-tidy/checks/misc-incorrect-pointer-cast.rst
8 ↗(On Diff #177810)

Integer -> int. Please highlight types with ``.

Check for different size and alignment as well.
Tests migrated from D33826.
Warning in case of reinterpret_cast, but options is added to be able to turn it off.

hgabii updated this revision to Diff 177812.Dec 11 2018, 6:38 PM

Highlight type names in documentation.

hgabii marked 4 inline comments as done.Dec 11 2018, 6:40 PM
hgabii added inline comments.
docs/ReleaseNotes.rst
139 ↗(On Diff #177810)

Lines added by me are not longer than 80 characters.
It is also true for the documentation.

docs/clang-tidy/checks/misc-incorrect-pointer-cast.rst
8 ↗(On Diff #177810)

Done.

@hgabii Are you planning on finishing this? If not, I'd happily commandeer if not.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 1:52 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
hgabii marked 2 inline comments as done.Mar 7 2019, 2:31 PM

@steakhal I think it is basically done. If you have any recommendations what needs to be changed, then I welcome your contribution. I can also fix new comments, but I have limited time for this.

Szelethus added a subscriber: Szelethus.

Is this patch good to go? :)

alexfh requested changes to this revision.Mar 11 2019, 10:47 AM

As per the very first comment, "bugprone" will be a more suitable category for this check. The rename_check.py script should be able to help here.

This revision now requires changes to proceed.Mar 11 2019, 10:47 AM

And one more comment that hasn't yet been addressed: "Some patterns are covered by compiler diagnostics: https://godbolt.org/g/HvsjnP. Is there any benefit in re-implementing them?"

And one more: please update file headers. The license has changed since.

steakhal commandeered this revision.Jun 3 2019, 7:15 AM
steakhal added a reviewer: hgabii.

If you don't mind I will finish the leftover work. This will still be committed under your name.

steakhal updated this revision to Diff 202717.Jun 3 2019, 7:29 AM

Unfortunately the changes that I've made are not available in a diff because I've moved to the monorepo version.
Although, you can see the changes in detail on my llvm-project github fork.

There were only minor changes.

  • Updated the license, as requested
  • Moved the checker to the 'bugprone' category from 'misc'
  • Fixed bug: now using getAsRecordDecl instead of getAsCXXRecordDecl

There is still an open question whether we should relay on the warnings of the -Wcast-align option, but I'm not convinced.

Some basic thoughts

clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp
64

this reads weirdly, should be
"new type is wider (%2) than the original type (%3)"

70–71

"*more* strictly aligned" perhaps?
Similarly, specify the actual values.

89

`break;'?

96

Would be better to at least point to first two incompatible fields

99–108

What does this mean?
How can signedness affect memory layout?

clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h
19

is cast*ed*
s/pointed to/new/

20

How do you know anything about the actual allocated memory area type.
You probably want to talk about the original type before cast.

Also, this kinda feels like this shouldn't be a single check.

  1. I'm very much looking forward the alignment part of this, the reinterpret_cast<>() specifically. It would be good to have a switch to also diagnose casts from void* here. But indeed, if it comes from c-style cast, -Wcast-align will already have diagnosed it.
  2. Then there is 'cast increases type width' check, makes sense i suppose.
  3. 'struct layout mismatch', makes sense.
  4. I don't understand the 'different signdness' part.

The problem with the -Wcast-align is that it will only fire for C-style bitcast expressions, not for reinterpret_cast ones. example
Does anyone know why is that behavior?

The problem with the -Wcast-align is that it will only fire for C-style bitcast expressions, not for reinterpret_cast ones. example
Does anyone know why is that behavior?

Because reinterpret_cast is by definition allowed to perform these casts, so it is assumed that no warning should be issued.
This part of the check i look forward to.

steakhal marked 8 inline comments as done.Jun 4 2019, 4:53 AM
steakhal added inline comments.
clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp
70–71

Which metrics should I use? Bits or char units? For now I will stick to bits.

99–108

You are right, signess difference is explicitly allowed in bitcasts. It has nothing to do with the actual memory layout. I thought it might be useful for detecting those cases too. It should be in a different checker in the future.
For now I've removed the related code from this checker.

clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h
19

Casted fixed now.

What do you think about destination type and original type instead of new type?

20

You are right, fixed.

steakhal updated this revision to Diff 202913.Jun 4 2019, 5:11 AM
steakhal marked 4 inline comments as done.
  • Removed different signess related parts.
  • Don't warn for the casts which are already covered by '-Wcast-align' check.
  • Improved the diagnostic messages:
    • Now adds notes for the first incompatible members of structs.
    • Added alignment, size and type information to most of the warning messages.

The problem with the -Wcast-align is that it will only fire for C-style bitcast expressions, not for reinterpret_cast ones. example
Does anyone know why is that behavior?

Because reinterpret_cast is by definition allowed to perform these casts, so it is assumed that no warning should be issued.
This part of the check i look forward to.

I still don't understand why should -Wcast-align issue warning only for c-style casts. What is the difference in the given example? What are the related paragraphs in the standard?
I assume in the example we violated the basic.lval/11 paragraph, so neither of those pointers are safely dereferencable.

In case we are casting from char* we don't know what is the dynamic type of the object (same with std::byte, void * etc.). From my point of view the style of the cast is not important in this case, only the fact that both are performing bitcasts.
Have I missed something?

What do you think, what should I improve in this checker?
Your remarks, @lebedev.ri, were really valuable.

Do you have some time @Szelethus to check this change?
Your experience and comments would help a lot to finish this.

aaron.ballman added inline comments.Aug 2 2019, 5:47 AM
clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp
41

Should be renamed to meet coding style guidelines (but don't reuse CastExpr please, as that's a type name).

42

Please drop top-level const on non-pointer/reference automatics.

47–50

Can these checks be hoisted into the AST matcher?

56

This one too.

60

Missing full stop at the end of the comment.

65

I'd drop the "lead to" from the diagnostic text.

70–71

I tend to think in terms of bytes rather than bits as that's the allocation unit for storage. Might be worth adding "bytes" into the diagnostic as well, just to be very clear.

74

Missing full stop. This comment suggests that we should not be diagnosing in C mode because it's a duplicate diagnostic, but you're gating it on a config flag rather than then language mode. Is that intentional?

79

Same comments here as above regarding "lead to" and bytes vs bits.

88

Comments should be full sentences with proper capitalization and punctuation.

104

Should this be looking at the canonical types as opposed to whatever types are written? e.g., will this decide that a field of type int and one of type int32_t are different (assuming that int32_t is a typedef to int)?

What if the fields have the same type, but have different bit widths? e.g., int i : 2; vs int i : 4;?

Should this consider qualifiers, like _Atomic() for differences, or is that already covered by the type comparison?

Should structure packing or alignment attributes impact this?

What if the fields are the same but the layout is still different? e.g., struct S { int i; }; vs 'struct S { virtual ~S() = default; int i; };` ?

What if one record is a struct and the other is a union, but their fields are the same? e.g., struct S { int i; double d; }; vs union U { int i; double d; }; ?

111

Do not use auto here as the type is not spelled out in the initialization.

115

Drop "lead to" (same below). I'm not certain "invalid layout" is a good way to phrase it -- I think "incompatible layout" works better.

clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst
6

Warns for cases when a pointer is cast

6–11

Can you re-flow this paragraph?

11

I don't think signedness is covered by the check (nor should it be, IMO).

13

I think this could be reworded better in terms of common initial sequence (http://eel.is/c++draft/class.mem#def:common_initial_sequence), WDYT?

17

It is highly recommended

whith -> with

25

to do not warn -> to not warn
(also, remove the extraneous whitespace after warn).

reinterpret cast -> reinterpret_cast (and add backticks)

30

Underlining looks too short here.