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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
