[PATCH] D67545: [clang-tidy] Added DefaultOperatorNewCheck.

Tue Sep 17 07:10:29 PDT 2019

aaron.ballman added a comment.

Thank you for working on this check!

In D67545#1672106 <https://reviews.llvm.org/D67545#1672106>, @balazske wrote:

> 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.

================
Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:53
+    CheckFactories.registerCheck<DefaultOperatorNewCheck>(
+        "cert-default-operator-new");
     CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
----------------
balazske wrote:
> The checker should be renamed to `cert-mem57-cpp` to comply with the others.
Yes, please. It should also be moved under a `// MEM` comment instead of `OOP` (and kept in alphabetical order).

================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:26
+
+  if (NewExpr->getNumPlacementArgs() > 0)
+    return;
----------------
balazske wrote:
> martong wrote:
> > 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?
> 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.
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.

================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:41
+  unsigned SpecifiedAlignment = D->getMaxAlignment();
+  unsigned DefaultAlignment = Context.getTargetInfo().getCharAlign();
+  if (!SpecifiedAlignment)
----------------
balazske wrote:
> martong wrote:
> > 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.
> I wanted to use `Context.getTargetInfo().getNewAlign()` here, is it correct? (Or `getNewAlign()/getCharWidth()`?)
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).

================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:49
+  if (HasDefaultOperatorNew && OverAligned)
+    diag(NewExpr->getBeginLoc(), "using default 'operator new' with over-aligned type %0 may result in undefined behavior")
+      << D;
----------------
balazske wrote:
> martong wrote:
> > I think it would be useful to add the value of the fundamental alignment to the diagnostics too.
> 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//."
I'd go with: `allocation function returns a pointer with alignment %0 but the over-aligned type being allocated requires alignment %1`

================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.h:18
+
+/// FIXME: Write a short description.
+///
----------------
FIXME needs fixing.

================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:79
+
+  FIXME: add release notes.
+
----------------
You should fix this fixme.

================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-default-operator-new.rst:6
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
----------------
This one too -- you should model it after the other CERT check documentation, with a link back to the CERT check.

================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:107
    cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp>
-   clang-analyzer-core.CallAndMessage
-   clang-analyzer-core.DivideZero
+   clang-analyzer-core.CallAndMessage (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.CallAndMessage>
+   clang-analyzer-core.DivideZero (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.DivideZero>
----------------
balazske wrote:
> martong wrote:
> > Why do we have these changes? Seems to be unrelated.
> I used the `add_new_check.py` script, do not know why these changes appeared. (Probably issue with the diff or rebase?)
Something got messed up -- you should back out these changes.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67545/new/

https://reviews.llvm.org/D67545