[PATCH] D134320: [llvm] Teach whole program devirtualization about relative vtables

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:571
+    }
+  } else if (const auto *CE = dyn_cast<ConstantExpr>(stripCasts(I))) {
+    // If this constant can be reduced to the offset between a function and the
----------------
pcc wrote:
> leonardchan wrote:
> > tejohnson wrote:
> > > Do we also need to look through DSOLocalEquivalent in this function, as is done in getPointerAtOffset, or why not?
> > Not needed here since `IsConstantOffsetFromGlobal` already handles DSOLocalEquivalent to get the offset and global.
> This is just stripping `Trunc`, right? You could put this below this line instead of introducing `stripCasts`:
> ```
> if (CE->getOpcode() == Instruction::Trunc)
>   CE = CE->getOperand(0);
> ```
Yeah, removed.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:579
+      if (IsConstantOffsetFromGlobal(CE->getOperand(0), LHS, LHSOffset, DL) &&
+          IsConstantOffsetFromGlobal(CE->getOperand(1), RHS, RHSOffset, DL) &&
+          RHS == &OrigGV) {
----------------
pcc wrote:
> leonardchan wrote:
> > tejohnson wrote:
> > > Do we need to do any checking or handling of LHSOffset and RHSOffset?
> > In the context of `findFuncPointers`, I don't think so. LHS/RHSOffset just return the byte offset of either LHS/RHS from their respective globals (like via a GEP). `IsConstantOffsetFromGlobal` is just a nifty function for getting the underlying global in these cases. The relevant bits here are confirming RHS refers to the original vtable and we continue to track the function via LHS. 
> I think Teresa's concern was that if the offsets don't match this could end up being loaded as `function + 1` or something, which would not be callable. But I suppose we could just say that because calling e.g. `function + 1` is UB we can ignore that possibility and pretend that `function` was being called instead. So we don't need to check the offset. It may be worth adding a comment about that though.
Oh I see. Hmm specifically for RV, I think LHSOffset should effectively always be zero since the RHS should essentially be a pointer to the virtual function. Since this is a new addition to lto for vtables, I imagine it's better to be conservative here and add the LHSOffset check. Likewise for RHS, the offset should point to somewhere in the vtable, so we can also check here that the RHSOffset is at the very least within the vtable.


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:140
                                    Constant *TopLevelGlobal) {
+  if (auto *Equiv = dyn_cast<DSOLocalEquivalent>(I))
+    I = Equiv->getGlobalValue();
----------------
pcc wrote:
> I suppose it is the caller who knows whether it is appropriate to strip the `DSOLocalEquivalent`. But it seems that the existing callers should be fine with it so I don't mind adding this here.
> 
> It's somewhat suboptimal that this function handles absolute and relative pointers in the same function, but I suppose this is justifiable for the "UB" reasons that also justify ignoring the offset above. It may have been better to split out the "destructuring" part of this function from the pointer-identifying part (which could be done in two functions, one for absolute pointers and another for relative). That would make the semantics clearer at least. But you didn't add the relative pointer support to this function so I don't mind if we don't clean this up for now.
Added a TODO comment for this for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134320