[clang] [CIR] Add object offset calculation for basic AA (PR #219047)

[Erich Keane via cfe-commits]

================
@@ -47,114 +138,96 @@ mlir::Value CIRBasicAliasAnalysis::getUnderlyingObject(mlir::Value val) {
       break;
     }
 
-    // Pointer stride: only strip through when we can prove the access stays
-    // within the bounds of the underlying allocation.
+    // A stride moves the pointer by `stride * sizeof(pointee)` bytes.
     if (auto strideOp = mlir::dyn_cast<cir::PtrStrideOp>(defOp)) {
-      auto constOp = strideOp.getStride().getDefiningOp<cir::ConstantOp>();
-      if (constOp) {
-        if (auto intAttr = mlir::dyn_cast<cir::IntAttr>(constOp.getValue())) {
-          APInt stride = intAttr.getValue();
-
-          // Zero stride is trivially in-bounds.
-          if (stride.isZero()) {
-            LDBG() << "Walking past zero-strided PtrStrideOp";
-            val = strideOp.getBase();
-            continue;
-          }
-        }
-      }
-      // Dynamic stride or unverifiable bounds — stop here conservatively.
-      LDBG() << "Non-zero or dynamic PtrStrideOp, stopping";
-      break;
+      LDBG() << "Walking past PtrStrideOp";
+      addToOffset(offset,
+                  scaleOffset(getConstantIndex(strideOp.getStride()),
+                              getTypeSizeInBytes(strideOp.getElementType(),
+                                                 dataLayout)));
+      val = strideOp.getBase();
+      continue;
     }
 
-    // Handle special cases for zero-offset sub-object accesses.
-    if (auto op = mlir::dyn_cast<cir::GetMemberOp>(defOp)) {
-      if (op.getIndex() == 0) {
-        LDBG() << "GetMemberOp[0], following to underlying object";
-        val = op.getAddr();
-        continue;
-      } else {
-        LDBG() << "GetMemberOp, non-zero index, stopping";
-        break;
-      }
+    // A record member sits at a fixed offset given by the record layout.
+    if (auto memberOp = mlir::dyn_cast<cir::GetMemberOp>(defOp)) {
+      LDBG() << "Walking past GetMemberOp";
+      auto recordTy =
+          mlir::cast<cir::RecordType>(memberOp.getAddrTy().getPointee());
+      std::optional<int64_t> memberOffset;
+      if (!recordTy.isIncomplete())
+        memberOffset =
+            recordTy.getElementOffset(dataLayout, memberOp.getIndex());
+      addToOffset(offset, memberOffset);
+      val = memberOp.getAddr();
+      continue;
     }
-    if (auto op = mlir::dyn_cast<cir::GetElementOp>(defOp)) {
-      cir::IntAttr index;
-      if (auto constOp = op.getIndex().getDefiningOp<cir::ConstantOp>())
-        index = mlir::dyn_cast<cir::IntAttr>(constOp.getValue());
-      if (index && index.getValue().isZero()) {
-        LDBG() << "GetElementOp[0], following to underlying object";
-        val = op.getBase();
-        continue;
-      }
-      LDBG() << "GetElementOp, non-zero or dynamic index, stopping";
-      break;
+
+    // An array element sits at `index * sizeof(element)` bytes into the array.
+    if (auto elementOp = mlir::dyn_cast<cir::GetElementOp>(defOp)) {
+      LDBG() << "Walking past GetElementOp";
+      addToOffset(offset,
+                  scaleOffset(getConstantIndex(elementOp.getIndex()),
+                              getTypeSizeInBytes(elementOp.getElementType(),
+                                                 dataLayout)));
+      val = elementOp.getBase();
+      continue;
     }
-    if (auto op = mlir::dyn_cast<cir::BaseClassAddrOp>(defOp)) {
-      // A zero byte offset means the base subobject starts at the same address
-      // as the derived object.
-      if (op.getOffset().isZero()) {
-        LDBG() << "BaseClassAddrOp[0], following to underlying object";
-        val = op.getDerivedAddr();
-        continue;
-      }
-      LDBG() << "BaseClassAddrOp, non-zero offset, stopping";
-      break;
+
+    // A base class subobject starts the given number of bytes into the derived
+    // object.
+    if (auto baseOp = mlir::dyn_cast<cir::BaseClassAddrOp>(defOp)) {
+      LDBG() << "Walking past BaseClassAddrOp";
+      addToOffset(offset, baseOp.getOffset().tryZExtValue());
----------------
erichkeane wrote:

> I just assumed there were target architectures that supported valid zero-addresses based on the fact that we have handling for it.

Maybe? I DO know we are often pretty pedantic about "but what about mystery arch that we don't support?!" in many places, but its also entirely/equally possible I am not aware of one we use.  Perhaps some sort of 'bare metal'/freestanding implementation? 

> Perhaps what I intended to do about the case where we aren't assuming non-null wasn't clear. I understand that the operation needs it, and it will lower to IR that checks for null and returns null with no offset when the input is null. However, I wanted to ignore that possibility for purposes of alias analysis. My reasoning is that if the input pointer is null (and therefore the result is null), any dereference of that null pointer would be UB, and so it wouldn't matter if we gave an incorrect alias analysis result for that access.

On the face, that seems reasonable.
 
> To be more explicit. This is what I'm saying:
> 
> ```
> void foo() { bar(nullptr); }
> int bar(Derived *p) {
>   Base2 *p2 = static_cast<Base2*>(p); // return null
>   int *pi = (int*)p; // also null
>   pi++; // pi is now 4
>   *pi = 1; // dereference 4 --> UB
>   return p2->y; // dereferences null --> UB
> }
> ```

That all appears accurate.

> For this case, we would report `MustAlias` for the access at `*pi = 1` and the access at `return p2->y`. Even though these are different pointers and their accesses wouldn't overlap if they were defined behavior (the address zero is valid case), but the `static_cast` would be giving a wrong answer in the address zero is valid case, and in the address zero is not valid case, both accesses are UB so it doesn't matter that we said they alias.

TBH, if we're doing that `if` chain, we're already breaking the `zero is a valid pointer` case.  The fact that we don't apply the offset means we've already broken it.  However, if you check classic-codegen, it inserts the null check ONLY based on the expression (see `CodeGenFunction::ShouldNullCheckClassCastValue`).  `ImplicitCastExpr` and `This` are excluded BECAUSE it is UB to have these expressions be null already.  The other case (`UncheckedDerivedToBase`) is ALSO a case where the Sema has determined 'null is impossible', not 'we have good reason to allow 0'.

SO I don't see (unless i'm missing something you're seeing?) anywhere we are acknowledging these '0 is a valid pointer' archs? THAT SAID, if they WERE supported, we WOULD need a valid value for `null`, and LLVM would have to support it as a `null` constant. (That is, `0 is a valid address` means you have to have SOMETHING to be `null`, and `0`!=`null`).  So LLVM would have to do this comparison, and, as a bit of a tautology, `myval eq null-constant` would ALWAYS be testing for an 'invalid' address.

> The alternative is we return `MayAlias` for any pointer with a BaseClassAddrOp that doesn't assume non-null in its pointer use-def chain. So for a case like this...
> 
> ```
> void foo() { bar(nullptr); }
> int bar(Derived *p) {
>   Base2 *p2 = static_cast<Base2*>(p); // return null
>   if (p2 == null)
>     return -1;
>   p->x = 0; // Writes 4 bytes at p, offset zero
>   return p2->y; // Reads 4 bytes at p, offset four
> }
> ```
> 
> ...we'd return `MayAlias`, even though we could say `NoAlias` if ignored the possibility of the static_cast returning null. For this trivial case that wouldn't matter, but I'm sure you can see how it could in other cases.

This section/example I'm not really getting? But my understanding of the alias-analysis is limited.


https://github.com/llvm/llvm-project/pull/219047