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

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

steakhal added inline 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.
----------------
IMO this should be in a separate paragraph. It's such a niche use-case.


================
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
----------------
Shouldn't we check for this target?
Add a test if we have `x86` and a non-`x86` target.


================
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;
+    }
----------------
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};
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:331
+  auto *Chk = mgr.registerChecker<DereferenceChecker>();
+  // auto *Chk = mgr.getChecker<DereferenceChecker>();
+  Chk->DetectAllNullDereferences =
----------------
dead-code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841