[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I'll address, thanks for the comments.



================
Comment at: clang/docs/analyzer/checkers.rst:69-74
+Check for dereferences of null pointers. This checker specifically does
+not report null pointer dereferences for x86 and x86-64 targets when the
+address space is 256 (x86 GS Segment), 257 (x86 FS Segment), or 258 (x86 SS
+segment). See `X86/X86-64 Language Extensions
+<https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments>`__
+for reference.
----------------
steakhal wrote:
> IMO this should be in a separate paragraph. It's such a niche use-case.
I'll address.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:119
+  // and #258. Those address spaces are used when dereferencing address spaces
+  // relative to the GS, FS, and SS segments on x86/x86-64 targets.
+  // Dereferencing a null pointer in these address spaces is not defined
----------------
steakhal wrote:
> Shouldn't we check for this target?
> Add a test if we have `x86` and a non-`x86` target.
I wasn't sure it was needed, but easy enough to add I think. I'll address both. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:125-133
+  if (isTargetAddressSpace(E->getType().getAddressSpace())) {
+    switch (toTargetAddressSpace(E->getType().getAddressSpace())) {
+    case 256:
+    case 257:
+    case 258:
+      return true;
+    }
----------------
steakhal wrote:
> Even though `DetectAllNullDereferences=true`, `address_space(256)` would be suppressed, which seems to be a contradiction to me.
> 
> We should either pick a different name or implement a different semantic.
> I would vote for both, personally.
> 
> Then name should encode that it's purely about address-space attributes.
> The implementation should check if we have attributes or not, and bail out in the generic case. I would do something like this:
> 
> ```
> QualType Ty = E->getType();
> if (!Ty.hasAddressSpace())
>   return false;
> if (SuppressAddressSpaces)
>   return true;
> // On X86 addr spaces ...., thus ignored.
> return target == x86 && addressspace in {256,257,258};
> ```
I'll clean up the semantics. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841