[PATCH] D128056: [clang][dataflow] Singleton pointer values for null pointers.

[weiyi via Phabricator via cfe-commits]

wyt marked an inline comment as done.
wyt added a comment.

@xazax.hun

> Since you always want this function to create a null pointer value, I think it would be less error prone to ask for the location instead of an arbitrary value. Currently, a confused caller could put a non-null value into a table.

Replaced with a getOrCreate factory to encapsulate pointer value creation.

> I think `getAsString` is considered expensive. Could you use `QualType` directly as the key?

Done.

> I am wondering about the future plans regarding how pointers are represented.
> What will be the expected behavior when the analysis discovers that the pointer has a null value? E.g.:
>
>   if (p == nullptr)
>   {
>     ....
>   }
>
> Would we expect `p` in this case to have the same singleton value in the then block of the if statement?

This is an interesting idea.
IIUC, for non-boolean values, the core framework currently does not dissect the equality expression - p == nullptr would be represented by a symbolic boolean, but p and nullptr would not be entangled together. 
However, we are working on a pointer nullability analysis on top of the framework that should add information to interrelate two pointer values and their nullability information when we do a comparison between pointers.



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:146
+  ///  `PointeeType`.
+  void setNullPointerVal(QualType PointeeType, PointerValue &Val) {
+    assert(NullPointerVals.find(PointeeType.getAsString()) ==
----------------
xazax.hun wrote:
> Since you always want this function to create a null pointer value, I think it would be less error prone to ask for the location instead of an arbitrary value. Currently, a confused caller could put a non-null value into a table. 
> Since you always want this function to create a null pointer value, I think it would be less error prone to ask for the location instead of an arbitrary value. Currently, a confused caller could put a non-null value into a table. 




================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:149
+           NullPointerVals.end());
+    NullPointerVals[PointeeType.getAsString()] = &Val;
+  }
----------------
xazax.hun wrote:
> I think `getAsString` is considered expensive. Could you use `QualType` directly as the key?
> I think `getAsString` is considered expensive. Could you use `QualType` directly as the key?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128056