[PATCH] D118050: [analyzer] Different address spaces cannot overlap

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

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:718
 
+static uint64_t getLocBitwidth(ProgramStateRef State, Loc loc) {
+  uint64_t LocBitwidth = 0;
----------------
Why don't you simply call `loc->getType()` end delegate the heavy lifting to that? 
If you have a meaningful type, you could acquire the size of that using the ASTContext exactly how you did.

After doing so, this would reduce the complexity of checking this, so the assertion could be placed into the body of `evalBinOpLL()`.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:723-725
+  default:
+    llvm_unreachable("Unhandled loc subkind in getLocBitwidth");
+    break;
----------------
Remove this default case. Simply `return 0;` from the end of the function.
Also, I would recommend `return`-ing from the handled cases instead of mutating a variable.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:738
+    if (!Ty.isNull())
+      LocBitwidth = Ctx.getTypeSize(Ty);
+  } break;
----------------
What if `Loc` is a `void*`. What would be the size of that one?


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:746
+
+static bool compareLocBitWidth(ProgramStateRef State, Loc RhsLoc, Loc LhsLoc) {
+  uint64_t RhsBitwidth = getLocBitwidth(State, RhsLoc);
----------------
`assertEqualBitwidths` would be probably more appropriate - even if this is a //best-effort// assertion.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:766
+  // and the bitwidths of the address spaces are different.
+  // See LIT case clang/test/Analysis/cstring-checker-addressspace.c
+  compareLocBitWidth(state, rhs, lhs);
----------------
Please have something like this in the assertion. That way we could immediately see why this is an issue.


================
Comment at: clang/test/Analysis/cstring-checker-addressspace.c:17
+
+DEVICE void *memcpy(DEVICE void *, const void *, unsigned long);
+
----------------
Remove this line, it's redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118050