[PATCH] D84733: [Attributor] Check nonnull attribute violation in AAUndefinedBehavior

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This is cool :)

I thought about this again and I think it will expose the problematic case I mentioned on the call. The Attributor derives attributes, then replaces the value with undef because it is dead, then the thing might be folded to UB/unreachable. Let's directly require the `noundef` attribute as well. We need to split the AAUB behavior accordingly soon but I'm fine with checking it explicitly here first. That will require tests with and without `noundef`.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1988
+      // Check nonnull argument attribute violation for each callsite.
+      CallBase *CB = cast<CallBase>(&I);
+      const Function *Callee = CB->getCalledFunction();
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1993
+      unsigned ArgNum =
+          std::min(CB->getNumArgOperands(), (unsigned)Callee->arg_size());
+      for (unsigned idx = 0; idx < ArgNum; idx++) {
----------------
You don't need a callee, just use the call base.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1995
+      for (unsigned idx = 0; idx < ArgNum; idx++) {
+        Value *ArgVal = CB->getArgOperand(idx);
+        const auto &ValueSimplifyAA =
----------------
I somehow feel checking the attribute first is better.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2002
+        // If current argument is known to be simplified to null pointer,
+        // this callsite is considered UB.
+        if (!ValueSimplifyAA.isKnown() || !SimplifiedVal.hasValue())
----------------
Move this to the beginning to describe what is happening. Maybe elaborate a bit.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2015
+      return true;
+    };
+
----------------
Provide a comment before or at the beginning of this lambda explaining it a bit.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2024
                               /* CheckBBLivenessOnly */ true);
+    A.checkForAllCallLikeInstructions(InspectCallSiteForUB, *this);
     if (NoUBPrevSize != AssumedNoUBInsts.size() ||
----------------
baziotis wrote:
> Please add a third argument specifying the instructions to check, i.e. `{Instruction::CallSite}`. Also, a fourth argument `true` as a value, like the other too above.
No need to specify call site (which is not an opcode) because this is a specialized version of `checkForAllInstructions`. The CheckBBLivenessOnly can be provided I guess.


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

https://reviews.llvm.org/D84733