[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a reviewer: martong.
NoQ added a comment.
Herald added a subscriber: rnkovacs.

First of all, the static analyzer doesn't warn on everything at once; it stops the analysis after a fatal error (such as UB) is found, so it doesn't explore the execution path further. This is the reason why some tests look like they don't warn, but they will if you comment out the previous tests in the same function.

  // bad (short is aligned to 2).
  short* s6 = nullptr;
  ::new (s6) long;
  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's6' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]

Null pointer is obviously well-aligned, so this is a false positive / incorrect test (though for a syntactic check that focuses on a single statement it's a perfectly normal test).

  // bad (just check for pointer to pointer).
  short **s7;
  ::new (*s7) long;
  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's7' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]

There's another checker that warns here: `warning: Dereference of undefined pointer value`. So the analysis is interrupted before `new` is reached.

  // bad (buffer is aligned to 'short' instead of 'long').
  alignas(short) unsigned char buffer2[sizeof(long)];
  ::new (buffer2) long;
  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'buffer2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]

Hmm, i think we don't have an insufficient alignment check yet, only an insufficient storage check. Same goes for all of the `test3()`. These shouldn't be too hard to add so that, at least, to pass the missing tests.

  short* test4_f1()
  {
      return nullptr;
  }
  
  void test4()
  {
      ::new (test4_f1()) long;
      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]

Again, the static analyzer is inter-procedural so it knows that it's a null pointer.

  short* (*p1)();
  p1 = test4_f1;
  ::new (p1()) long;
  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'p1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]

We can even see through your obfuscation via function pointer! It's still null.

  struct S
  {
      short a;
  };
  
  // bad (not enough space).
  const unsigned N = 32;
  alignas(S) unsigned char buffer1[sizeof(S) * N];
  ::new (buffer1) S[N];
  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 64 bytes are not enough for array allocation which requires 64 bytes [cert-mem54-cpp]
  
  // maybe ok but we need to warn.
  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)];
  ::new (buffer2) S[N];
  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: possibly not enough 68 bytes for array allocation which requires 64 bytes. current overhead requires the size of 4 bytes [cert-mem54-cpp]

Ah yes, placement array-new, we're still missing it but it shouldn't be hard to add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229