[PATCH] D128535: [analyzer] Improve loads from reinterpret-cast fields

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

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1897
+  QualType Ty = SubReg->getValueType();
+  if (BaseTy->isScalarType() && Ty->isScalarType()) {
+    if (Ctx.getTypeSizeInChars(BaseTy) >= Ctx.getTypeSizeInChars(Ty)) {
----------------
BTW I'm not sure why this check is here. I would expect this to work eve without this.
OOh, I think I know why. `getTypeSizeInChars` can only be called on //scalars//?


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2014-2017
+  // FIXME: This is a hack, and doesn't do anything really intelligent yet.
+  // FIXME: This hack is analogous with the one present in
+  // `getBindingForElement`, maybe we should handle both in
+  // `getBindingForFieldOrElementCommon`.
----------------
martong wrote:
> Could you please elaborate why this is a hack? What should be done and where to solve it properly? (Disclaimer, I did not look into `getBindingForFieldOrElementCommon`.)
I'm not sure what Jordan was actually referring to. IMO hardcoding to look for a highly specific scenario (a typed memregion, and a symbol to be exact) could be thought about implementing a 'hack'. IMO this fits in the current ecosystem, so for me it doesn't look like a hack; hence IMO this is the way of implementing this.

To make it clear, I simply hoisted the nested `dyn_cast`s and checks into a function, while preserved the 'hack' comment at the callsite; so it's not added by me.

One additional note. We relay on the type information embedded into the memregion - which we know might not always be present and even accurate. That could be another reason why this should be considered as a 'hack'. IDK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128535