[PATCH] D144319: [SimplifyCFG] Check if the return instruction causes undefined behavior

[DianQK via Phabricator via llvm-commits]

DianQK added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:7124
       if (GEP->getPointerOperand() == I) {
         if (!GEP->isInBounds() || !GEP->hasAllZeroIndices())
           PtrValueMayBeModified = true;
----------------
nikic wrote:
> As a side note, I'm pretty sure this was supposed to read `!GEP->isInBounds() && !GEP->hasAllZeroIndices()`, the current check doesn't make a lot of sense.
After addressing the current problem, I cloud to try to refactor this part.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:7138
+      if (C->isNullValue() && HasNoUndefAttr &&
+          Ret->getFunction()->hasRetAttribute(Attribute::NonNull)) {
+        return true;
----------------
nikic wrote:
> Not sure whether that's the cause of the sanitizer failures, but I just realized that we're missing a check for `!PtrValueMayBeModified` here -- this is not a provenance based fold, so it must be exactly the null pointer.
I found the `DeadArgumentElimination` pass will change `define internal noundef zeroext i1 @...SelectionFinder12TraverseStmt...` to `define fastcc void @...SelectionFinder12TraverseStmt...`. Then the `if (isa<UndefValue>(C) && HasNoUndefAttr) return true;` statement is no longer a sound decision.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:7135
+        return true;
+      // Return null to a nonnnull return value is undefined.
+      if (C->isNullValue() &&
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > if the result value is not used - is it?
> > 
> > check line 7172, it requires nonnnull+noundef for similar case.
> (please ignore, was in draft)
I checked the `test/Refactor/Extract/ObjCProperty.m` case. The `DeadArgumentElimination` pass will change `define internal noundef zeroext i1 @...SelectionFinder12TraverseStmt...` to `define fastcc void @...SelectionFinder12TraverseStmt...`.
So if no return value is used, returning undefined values does not appear to be undefined behavior.

Now, finding a sound solution was a bit difficult for me, but I will address it in this direction.

(More study time may be required.)

Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144319