[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:7138
+      if (C->isNullValue() && HasNoUndefAttr &&
+          Ret->getFunction()->hasRetAttribute(Attribute::NonNull)) {
+        return true;
----------------
nikic wrote:
> nikic wrote:
> > DianQK wrote:
> > > DianQK wrote:
> > > > nikic wrote:
> > > > > DianQK wrote:
> > > > > > nikic wrote:
> > > > > > > DianQK wrote:
> > > > > > > > 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.
> > > > > > > Not sure I understand. Once the return type is changed to void there is no longer a noundef attribute (or return instruction operand, for that matter).
> > > > > > Yes, I think so. This `DeadArgumentElimination` pass only handles internal functions, we can add a non-internal function check. It should be safe because any pass should not change the attributes and types of the non-internal function. But I'm not sure yet if this will change under LTO.
> > > > > Sorry, I still don't get what the DeadArgumentElimination pass has to do with this. Before DAE runs, we have a `noundef` return value and if this optimization triggers, presumably an `undef` value is being returned from it on some path. This is UB even if the return value is actually unused and can be eliminated by DAE.
> > > > > 
> > > > > Can you share a more complete IR sample?
> > > > You can try this IR, this content can be very complex and I haven't made a simplified example yet.
> > > > 
> > > > The function is `define internal noundef zeroext i1 @_ZN12_GLOBAL__N_118ASTSelectionFinder12TraverseStmtEPN5clang4StmtE` from https://github.com/llvm/llvm-project/blob/main/clang/lib/Tooling/Refactoring/ASTSelection.cpp#L112.
> > > > 
> > > > > To make the content simpler, I manually deleted https://github.com/llvm/llvm-project/blob/main/clang/lib/Tooling/Refactoring/ASTSelection.cpp#L122-L128.
> > > > 
> > > > {F26591071}
> > > The DAE changes the function to `define internal fastcc void @_ZN12_GLOBAL__N_118ASTSelectionFinder12TraverseStmtEPN5clang4StmtE`, also it changes the return instruction. 
> > > The return value of this function no longer requires noundef, or more precisely, no return value (a `void`).
> > > Now it looks like these two optimizations are in conflict, one requiring the return value noundef and one removing noundef.
> > > 
> > > Sorry I haven't found a more precise explanation.
> > > The current local `buildbot_bootstrap_asan.sh` test looks fine.
> > Thanks for the IR. I believe I understand the problem now. IPSCCP replaces return values with undef, but does not drop the noundef attribute. I'll work on a patch...
> Patch: https://reviews.llvm.org/D144461
Wow, thank you for giving the right solution! 
I tested `test/Refactor/Extract/ObjCProperty.m`, which was fine.

> Maybe next I should learn what all the LLVM passes are doing.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144319