[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Oh, so we would canonicalize towards a signed extent type. I see. I think I'm okay with that.
However, I think this needs a little bit of polishing and testing when the region does not point to the beginning of an array or object, but rather inside an object, or like this:

  int nums[] = {1,2,3};
  char *p = (char*)&nums[1];
  
  clang_analyzer_dumpExtent(p);
  clang_analyzer_dumpElementCount(p);
  ++p;
  clang_analyzer_dumpExtent(p);
  clang_analyzer_dumpElementCount(p);
  ++p;
  int *q = (int*)p;
  clang_analyzer_dumpExtent(q);
  clang_analyzer_dumpElementCount(q);



================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:47-48
+                                                         const MemRegion *MR) {
+  if (!MR)
+    return UnknownVal();
+  MR = MR->StripCasts();
----------------
I'd prefer hoisting this check to the callsite.
Usually, we assume `MR` is non-null. This is already the case for a sibling API, `getDynamicExtent()`.
Let's keep these "overloads" behave the same.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:56-57
+  QualType Ty = TVR->getValueType();
+  if (const auto *ConstArrayTy =
+          dyn_cast_or_null<ConstantArrayType>(Ty->getAsArrayTypeUnsafe()))
+    return SVB.makeIntVal(ConstArrayTy->getSize(), false);
----------------
Funnily enough, one must use the `ASTContext::getAsConstantArrayType()` to achieve this.
I'm not sure why, but I was bitten by this.
It says something like this at the ASTContext:
```
  /// Type Query functions.  If the type is an instance of the specified class,
  /// return the Type pointer for the underlying maximally pretty type.  This
  /// is a member of ASTContext because this may need to do some amount of
  /// canonicalization, e.g. to move type qualifiers into the element type.
  const ArrayType *getAsArrayType(QualType T) const;
  const ConstantArrayType *getAsConstantArrayType(QualType T) const {
    return dyn_cast_or_null<ConstantArrayType>(getAsArrayType(T));
  }
```


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:58
+          dyn_cast_or_null<ConstantArrayType>(Ty->getAsArrayTypeUnsafe()))
+    return SVB.makeIntVal(ConstArrayTy->getSize(), false);
+
----------------
For bool literal arguments we usually use "named parameter passing", aka. `/*paramname=*/false` constructs.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:66-75
+  assert(!ElementSize.isZeroConstant() && "Non-zero element size expected");
+  if (Size.isUndef())
+    return UnknownVal();
+
+  SValBuilder &SVB = State->getStateManager().getSValBuilder();
+
+  auto ElementCount =
----------------
If `ElementSize` would be some symbol, constrained to null, we would pass the assertion, but still end up modeling a division by zero, resulting in `Undefined`, which then turned into `Unknown` - I see.

Looking at this, the assertion seems misleading as it won't protect the division.
That said, the early return on undef could be also dropped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158707