Page MenuHomePhabricator

[clang-tidy] Added DefaultOperatorNewCheck.
ClosedPublic

Authored by balazske on Sep 13 2019, 4:00 AM.

Details

Summary

Added new checker 'cert-default-operator-new' that checks for
CERT rule MEM57-CPP. Simple version.

Diff Detail

Event Timeline

balazske created this revision.Sep 13 2019, 4:00 AM

Some thoughts:

  1. Docs missing
  2. How does this play with C++17 aligned new? Assuming compiler/library support for that isn't broken ('bye, OSX!', ?), i'm not sure why it would be UB for C++17, see https://godbolt.org/z/kwxRbu vs https://godbolt.org/z/om-bR2
  3. I'm not sure what's getCharAlign() has to do with anything here? You want to check the alignment requirement against the __STDCPP_DEFAULT_NEW_ALIGNMENT__ (i don't recall how to get it inside of clang) i would think?
martong added inline comments.
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
26 ↗(On Diff #220073)

Perhaps we should add in the docs that placement new is not supported. Or add a fixme here.
Anyway, I feel this code simply could work with placement new as well. What is the reason you disabled it for placement new?

41 ↗(On Diff #220073)

This might not be what we want... getCharAlign() theoretically could return even with 1, I think, though it would be a very strange architecture.
Perhaps we should use getSuitableAlign() instead?

Also, I think we should call the variable as FundamentalAlignment. Fundamental and default alignment can differ, I guess.

49 ↗(On Diff #220073)

I think it would be useful to add the value of the fundamental alignment to the diagnostics too.

clang-tools-extra/docs/clang-tidy/checks/list.rst
110–112

Why do we have these changes? Seems to be unrelated.

balazske marked 3 inline comments as done.Sep 16 2019, 11:53 PM

C++17 makes things more difficult because the align is probably handled by operator new, probably not, depending on the defined allocation functions. This can be observed only with a non clang-tidy checker (we could compute the used alignment?). Probably the CERT rule is from the time before C++17. It looks like that by default the alignment is handled correctly in C++17 so the whole check is out of scope for that language version.

clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
26 ↗(On Diff #220073)

Placement new provides already a memory location that is specified by the user (or some custom allocation function) so this is not a default case for which the warning should be provided.

41 ↗(On Diff #220073)

I wanted to use Context.getTargetInfo().getNewAlign() here, is it correct? (Or getNewAlign()/getCharWidth()?)

clang-tools-extra/docs/clang-tidy/checks/list.rst
110–112

I used the add_new_check.py script, do not know why these changes appeared. (Probably issue with the diff or rebase?)

balazske marked an inline comment as done.Sep 17 2019, 1:08 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
53

The checker should be renamed to cert-mem57-cpp to comply with the others.

balazske marked an inline comment as done.Sep 17 2019, 2:03 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
49 ↗(On Diff #220073)

Should be this the correct error message? "Allocation with standard new: too long alignment for a user type." Or "Allocation with standard new: too long alignment (user_align) for a user type, default is default_align."

Thank you for working on this check!

C++17 makes things more difficult because the align is probably handled by operator new, probably not, depending on the defined allocation functions.

Correct.

This can be observed only with a non clang-tidy checker (we could compute the used alignment?).

I'm less certain of this. We should be able to look up which operator new overload was selected for the new expression and check to see if it has an alignment parameter. If it does, I think it's reasonable to assume that the function behaves correctly wrt alignment.

Probably the CERT rule is from the time before C++17. It looks like that by default the alignment is handled correctly in C++17 so the whole check is out of scope for that language version.

Yeah, the CERT C++ rules were written for C++14: https://wiki.sei.cmu.edu/confluence/display/cplusplus/Scope

The check may be out of scope for C++17, I've not analyzed it for that. There are times when new with align is not called (http://eel.is/c++draft/cpp.predefined#1.6) and user overloads to consider as well, but I think those should be trying to call the aligned new version when given an over-aligned type.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
53

Yes, please. It should also be moved under a // MEM comment instead of OOP (and kept in alphabetical order).

clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
26 ↗(On Diff #220073)

That would correspond to MEM54-CPP: https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM54-CPP.+Provide+placement+new+with+properly+aligned+pointers+to+sufficient+storage+capacity

I think this should be hoisted into a local AST matcher rather than be part of the check() call. Something like unless(isPlacementNew()) when registering the check.

41 ↗(On Diff #220073)

I think getNewAlign() is the correct thing to use here. If the alignment requirements of the allocated type are greater than what getNewAlign() returns, there are problems (unless calling the C++17 aligned allocation operator).

49 ↗(On Diff #220073)

I'd go with: allocation function returns a pointer with alignment %0 but the over-aligned type being allocated requires alignment %1

clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.h
18 ↗(On Diff #220073)

FIXME needs fixing.

clang-tools-extra/docs/ReleaseNotes.rst
85

You should fix this fixme.

clang-tools-extra/docs/clang-tidy/checks/cert-default-operator-new.rst
6 ↗(On Diff #220073)

This one too -- you should model it after the other CERT check documentation, with a link back to the CERT check.

clang-tools-extra/docs/clang-tidy/checks/list.rst
110–112

Something got messed up -- you should back out these changes.

balazske marked an inline comment as done.Sep 17 2019, 7:58 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
53

I will move it, but should I change the order of the commented sections? These are currently not in alphabetic order.

aaron.ballman added inline comments.Sep 17 2019, 8:05 AM
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
53

Not for this patch, but that could be done in a separate patch (as an NFC change).

balazske updated this revision to Diff 220625.Sep 18 2019, 2:01 AM

Rebase and update according to comments.
C++17 related changes not implemented yet (possible check for the called allocation function).

balazske updated this revision to Diff 220628.Sep 18 2019, 2:13 AM

Fixed the test, fixed problems in list.rst.

balazske marked 6 inline comments as done.Sep 18 2019, 2:17 AM
martong added inline comments.Sep 18 2019, 5:10 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
38 ↗(On Diff #220628)

Would it make sense to early return here if the language dialect is >= C++17 ?

51 ↗(On Diff #220628)

What is the difference between "default" and "fundamental" alignment? Are they the same? Can they differ in any architecture?

https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM57-CPP.+Avoid+using+default+operator+new+for+over-aligned+types
Here there is no wording of "default alignment" only "fundamental alignment" is mentioned. Based on this I'd call this as FundamentalAligment.

martong added inline comments.Sep 18 2019, 5:40 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

What is the difference between "default" and "fundamental" alignment? Are they the same?

fundamental alignment of any type is the alignment of std::max_align_t. I.e. alignof(std::max_align_t).
See C++17 6.11.2.

On the other hand, default alignment is the value in __STDCPP_DEFAULT_NEW_ALIGNMENT__ which may be predefined with fnew-alignment
See https://www.bfilipek.com/2019/08/newnew-align.html

These values can differ: https://wandbox.org/permlink/yIwjiNMw9KyXEQan

Thus, I think we should use the fundamental alignment here, not the default alignment.
So, getNewAlign() does not seem right to me.
@aaron.ballman What do you think?

lebedev.ri added inline comments.Sep 18 2019, 5:48 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

Thus, I think we should use the fundamental alignment here, not the default alignment.

I have the exact opposite view.
If as per getNewAlign() the alignment would be okay, why should we not trust it?

aaron.ballman added inline comments.Sep 18 2019, 5:49 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

The comment on getNewAlign() is:

/// Return the largest alignment for which a suitably-sized allocation with
/// '::operator new(size_t)' is guaranteed to produce a correctly-aligned
/// pointer.

I read that as saying any alignment larger than what is returned by getNewAlign() must call the over-aligned operator new variant in C++17 if available. So if the actual call target doesn't have an alignment specifier, it's probably getting the alignment wrong and would be worth diagnosing on.

martong added inline comments.Sep 18 2019, 6:03 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

I have the exact opposite view.
If as per getNewAlign() the alignment would be okay, why should we not trust it?

That could lead to a false positive diagnostic if -fnew-alignment=8 and alignas(16) , because alignof(max_align_t) is still 16.

See the definidion of getNewAlign() which will return with 8 in this case I suppose:

unsigned getNewAlign() const {
  return NewAlign ? NewAlign : std::max(LongDoubleAlign, LongLongAlign);
}
martong added inline comments.Sep 18 2019, 6:16 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

So if the actual call target doesn't have an alignment specifier, it's probably getting the alignment wrong and would be worth diagnosing on.

I agree, but then we are implementing a checker which is different from the description given in cert-mem57.
So it is not a CERT checker anymore, perhaps we should rename then.
There is no mention of STDCPP_DEFAULT_NEW_ALIGNMENT in https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM57-CPP.+Avoid+using+default+operator+new+for+over-aligned+types
It clearly references the "fundamental alignement".

aaron.ballman added inline comments.Sep 18 2019, 6:18 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

Why do you believe that to be a false positive? That seems like exactly the behavior we'd want -- if the user says that their allocation function guarantees a particular max alignment by using -fnew-alignment, we should honor that.

martong added inline comments.Sep 18 2019, 6:26 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

Why do you believe that to be a false positive? That seems like exactly the behavior we'd want -- if the user says that their allocation function guarantees a particular max alignment by using -fnew-alignment, we should honor that.

Okay, giving it more thought, that makes perfect sense.
Anyway, thanks for trying to understand my concerns :)

aaron.ballman added inline comments.Sep 18 2019, 6:27 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

That's because the CERT rule was written to target C++14 and earlier, which did not have __STDCPP_DEFAULT_NEW_ALIGNMENT__.

We can solve this in one of two ways: don't enable the check in C++17 mode, or do the right thing in C++17 mode. I think we should do the right thing, which is to check which overload is selected (if the aligned overload is selected, we don't diagnose because it's doing the right thing for the user) and compare against getNewAlign() (if the alignment requested is stricter than what we can guarantee through getNewAlign() and we've verified we're not calling an aligned overload, there is a real chance the pointer value will be incorrectly aligned). To me, that meets the spirit of what the CERT rule is trying to convey while still being useful in C++17.

aaron.ballman added inline comments.Sep 18 2019, 6:31 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

Okay, giving it more thought, that makes perfect sense.
Anyway, thanks for trying to understand my concerns :)

Thank you for the good discussion on them!

balazske marked an inline comment as done.Sep 18 2019, 7:43 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

I am not sure about if really any warning is needed in C++17. Because if the non-aligned operator new is selected it is already an user-provided one (the system-provided allocation function should take the alignment argument). And for user-defined operator new we do not want any warning (even if it looks like wrong but may be it is not).

aaron.ballman added inline comments.Sep 18 2019, 8:16 AM
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

I think this makes sense. If the user had replaced the global operator new but not the aligned one, the aligned one would still be found by overload resolution when it matters. So it seems fine to ignore C++17 for the check for now.

lebedev.ri marked an inline comment as done.Sep 18 2019, 8:21 AM
lebedev.ri added inline comments.
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp
51 ↗(On Diff #220628)

Tangentially-related rant:
The operator new/malloc&friend alignment assumptions question isn't new,
it was brought up on the list several times, and i think it will be approached once more.
Right now clang, even though it knows about those alignments, it does not annotate
them (operator new, ...) with alignment assumption, thus both pessimizing middle-end
(knowing that some pointer has some big alignment is useful!),
and "disabling" the appropriate UBSan check.

lebedev.ri added inline comments.Sep 18 2019, 3:26 PM
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
68

I find DefaultOperatorNewCheck to be insufficiently explanative.
Maybe DefaultOperatorNewAlignmentCheck ?

balazske updated this revision to Diff 222161.Sep 27 2019, 6:21 AM
  • Rename to DefaultOperatorNewAlignmentCheck.
aaron.ballman added inline comments.Sep 30 2019, 12:48 PM
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
67–69

The MEM section should come before MSC.

clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.cpp
25

Didn't we decide the check should not diagnose in C++17 or later?

30–31

I think this should be hoisted into a local AST matcher rather than be part of the check() call. Something like unless(isPlacementNew()) when registering the check.

48

Please drop top-level const on anything that's not a pointer or a reference (that's not a style we typically follow).

60

Commented-out code?

lebedev.ri requested changes to this revision.Oct 16 2019, 11:13 AM

(outstanding unaddressed review notes)

This revision now requires changes to proceed.Oct 16 2019, 11:13 AM
balazske updated this revision to Diff 225625.Oct 18 2019, 7:54 AM
  • Fixes from review comments, added C++ version test.
balazske marked 4 inline comments as done.Oct 18 2019, 7:59 AM
aaron.ballman accepted this revision.Oct 23 2019, 6:54 AM

LGTM aside from a minor issue.

clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.cpp
31–32

This should be in the registerMatchers() function so that we don't even bother trying to match anything. You don't need to do the || either; if 2a is enabled, that automatically enables all previous modes as well.

balazske updated this revision to Diff 226206.Oct 24 2019, 1:20 AM
  • Improved check for C++17.

Ping. It should be accepted before I can land it.

lebedev.ri resigned from this revision.Mon, Nov 18, 3:26 AM

Ping. It should be accepted before I can land it.

@aaron.ballman already accepted.

This revision is now accepted and ready to land.Mon, Nov 18, 3:26 AM

Ping. It should be accepted before I can land it.

@aaron.ballman already accepted.

And I'm happy to re-affirm my acceptance. :-D

This revision was automatically updated to reflect the committed changes.