<div dir="ltr">Thanks for the suggestions. Implemented in c971b53b22a5cd43b54bf4773fe3c59ea1b805fb and 6538fff37245921a0983d94c08af7e6cc120b3a9.<div><br></div><div>Michael</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Am Mo., 24. Aug. 2020 um 12:42 Uhr schrieb David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Aug 22, 2020 at 8:22 AM Michael Kruse via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Author: Michael Kruse<br>
Date: 2020-08-22T10:10:49-05:00<br>
New Revision: 6983741eaa84e76802feca9145e39293cc6d15b4<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/6983741eaa84e76802feca9145e39293cc6d15b4" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/6983741eaa84e76802feca9145e39293cc6d15b4</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/6983741eaa84e76802feca9145e39293cc6d15b4.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/6983741eaa84e76802feca9145e39293cc6d15b4.diff</a><br>
<br>
LOG: [Polly] Fix use-after-free.<br>
<br>
VirtualUse of type UseKind::Inter expects the definition of a<br>
llvm::Value to be represented in another statement. In the bug report<br>
that statement has been removed due to its domain being empty.<br>
Scop::InstStmtMap for the llvm::Value's defintion still pointed to the<br>
removed statement, which resulted in the use-after-free.<br>
<br>
The defintion statement was removed by Simplify because it was<br>
considered to not be reachable by other uses; trivially because it is<br>
never executed due to its empty domain. However, no such thing happend<br>
to the using statement using the value altough its domain is also empty.<br>
<br>
Fix by always removing statements with empty domains in Simplify since<br>
these are not properly analyzable. A UseKind::Inter should always have a<br>
statement with its defintion due to LLVM's SSA form.<br>
Scop::removeStmtNotInDomainMap() also removes statements with empty<br>
domains but does so without considering the context as used by<br>
Simplify's analyzes.<br>
<br>
In another angle, InstStmtMap pointing to removed statements should not<br>
happen either and ForwardOpTree would have bailed out if the llvm::Value<br>
definition was not represented by a statement. This will be corrected in<br>
a followup-commit.<br>
<br>
This fixes <a href="http://llvm.org/PR47098" rel="noreferrer" target="_blank">llvm.org/PR47098</a><br>
<br>
Added: <br>
polly/test/Simplify/func-b320a7.ll<br>
<br>
Modified: <br>
polly/include/polly/ScopInfo.h<br>
polly/lib/Transform/Simplify.cpp<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/polly/include/polly/ScopInfo.h b/polly/include/polly/ScopInfo.h<br>
index 7d99a38086d9..b6fcddc6379a 100644<br>
--- a/polly/include/polly/ScopInfo.h<br>
+++ b/polly/include/polly/ScopInfo.h<br>
@@ -1949,18 +1949,6 @@ class Scop {<br>
void addScopStmt(Region *R, StringRef Name, Loop *SurroundingLoop,<br>
std::vector<Instruction *> EntryBlockInstructions);<br>
<br>
- /// Remove statements from the list of scop statements.<br>
- ///<br>
- /// @param ShouldDelete A function that returns true if the statement passed<br>
- /// to it should be deleted.<br>
- /// @param AfterHoisting If true, also remove from data access lists.<br>
- /// These lists are filled during<br>
- /// ScopBuilder::buildAccessRelations. Therefore, if this<br>
- /// method is called before buildAccessRelations, false<br>
- /// must be passed.<br>
- void removeStmts(std::function<bool(ScopStmt &)> ShouldDelete,<br>
- bool AfterHoisting = true);<br>
-<br>
/// Removes @p Stmt from the StmtMap.<br>
void removeFromStmtMap(ScopStmt &Stmt);<br>
<br>
@@ -2321,6 +2309,19 @@ class Scop {<br>
MinMaxAliasGroups.back().first = MinMaxAccessesReadWrite;<br>
MinMaxAliasGroups.back().second = MinMaxAccessesReadOnly;<br>
}<br>
+<br>
+ /// Remove statements from the list of scop statements.<br>
+ ///<br>
+ /// @param ShouldDelete A function that returns true if the statement passed<br>
+ /// to it should be deleted.<br>
+ /// @param AfterHoisting If true, also remove from data access lists.<br>
+ /// These lists are filled during<br>
+ /// ScopBuilder::buildAccessRelations. Therefore, if this<br>
+ /// method is called before buildAccessRelations, false<br>
+ /// must be passed.<br>
+ void removeStmts(std::function<bool(ScopStmt &)> ShouldDelete,<br>
+ bool AfterHoisting = true);<br></blockquote><div><br>Unrelated to this patch (or a pre-existing condition at least) but assuming removeStmts doesn't capture "ShouldDelete" (only uses it within the scope of the function call) - it could use llvm::function_ref instead, which can avoid some overhead.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
/// Get an isl string representing the context.<br>
std::string getContextStr() const;<br>
<br>
<br>
diff --git a/polly/lib/Transform/Simplify.cpp b/polly/lib/Transform/Simplify.cpp<br>
index ec9df1846ec5..f3b8bf83efe5 100644<br>
--- a/polly/lib/Transform/Simplify.cpp<br>
+++ b/polly/lib/Transform/Simplify.cpp<br>
@@ -41,6 +41,8 @@ static int const SimplifyMaxDisjuncts = 4;<br>
TWO_STATISTICS(ScopsProcessed, "Number of SCoPs processed");<br>
TWO_STATISTICS(ScopsModified, "Number of SCoPs simplified");<br>
<br>
+TWO_STATISTICS(TotalEmptyDomainsRemoved,<br>
+ "Number of statement with empty domains removed in any SCoP");<br>
TWO_STATISTICS(TotalOverwritesRemoved, "Number of removed overwritten writes");<br>
TWO_STATISTICS(TotalWritesCoalesced, "Number of writes coalesced with another");<br>
TWO_STATISTICS(TotalRedundantWritesRemoved,<br>
@@ -124,6 +126,9 @@ class Simplify : public ScopPass {<br>
/// The last/current SCoP that is/has been processed.<br>
Scop *S;<br>
<br>
+ /// Number of statements with empty domains removed from the SCoP.<br>
+ int EmptyDomainsRemoved = 0;<br>
+<br>
/// Number of writes that are overwritten anyway.<br>
int OverwritesRemoved = 0;<br>
<br>
@@ -147,10 +152,36 @@ class Simplify : public ScopPass {<br>
<br>
/// Return whether at least one simplification has been applied.<br>
bool isModified() const {<br>
- return OverwritesRemoved > 0 || WritesCoalesced > 0 ||<br>
- RedundantWritesRemoved > 0 || EmptyPartialAccessesRemoved > 0 ||<br>
- DeadAccessesRemoved > 0 || DeadInstructionsRemoved > 0 ||<br>
- StmtsRemoved > 0;<br>
+ return EmptyDomainsRemoved > 0 || OverwritesRemoved > 0 ||<br>
+ WritesCoalesced > 0 || RedundantWritesRemoved > 0 ||<br>
+ EmptyPartialAccessesRemoved > 0 || DeadAccessesRemoved > 0 ||<br>
+ DeadInstructionsRemoved > 0 || StmtsRemoved > 0;<br>
+ }<br>
+<br>
+ /// Remove statements that are never executed due to their domains being<br>
+ /// empty.<br>
+ ///<br>
+ /// In contrast to Scop::simplifySCoP, this removes based on the SCoP's<br>
+ /// effective domain, i.e. including the SCoP's context as used by some other<br>
+ /// simplification methods in this pass. This is necessary because the<br>
+ /// analysis on empty domains is unreliable, e.g. remove a scalar value<br>
+ /// definition MemoryAccesses, but not its use.<br>
+ void removeEmptyDomainStmts() {<br>
+ size_t NumStmtsBefore = S->getSize();<br>
+<br>
+ auto ShouldDelete = [](ScopStmt &Stmt) -> bool {<br>
+ auto EffectiveDomain =<br>
+ Stmt.getDomain().intersect_params(Stmt.getParent()->getContext());<br>
+ return EffectiveDomain.is_empty();<br>
+ };<br>
+ S->removeStmts(ShouldDelete);<br></blockquote><div><br>FWIW, I'd probably put the lambda directly inside the "removeStmts" call - named lambdas are always a bit more "interesting" (where else is it called, how does it capture, etc) than ones that are used only within the scope of a single expression.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ assert(NumStmtsBefore >= S->getSize());<br>
+ EmptyDomainsRemoved = NumStmtsBefore - S->getSize();<br>
+ LLVM_DEBUG(dbgs() << "Removed " << EmptyDomainsRemoved << " (of "<br>
+ << NumStmtsBefore<br>
+ << ") statements with empty domains \n");<br>
+ TotalEmptyDomainsRemoved[CallNo] += EmptyDomainsRemoved;<br>
}<br>
<br>
/// Remove writes that are overwritten unconditionally later in the same<br>
@@ -587,6 +618,8 @@ class Simplify : public ScopPass {<br>
/// Print simplification statistics to @p OS.<br>
void printStatistics(llvm::raw_ostream &OS, int Indent = 0) const {<br>
OS.indent(Indent) << "Statistics {\n";<br>
+ OS.indent(Indent + 4) << "Empty domains removed: " << EmptyDomainsRemoved<br>
+ << '\n';<br>
OS.indent(Indent + 4) << "Overwrites removed: " << OverwritesRemoved<br>
<< '\n';<br>
OS.indent(Indent + 4) << "Partial writes coalesced: " << WritesCoalesced<br>
@@ -633,6 +666,9 @@ class Simplify : public ScopPass {<br>
this->S = &S;<br>
ScopsProcessed[CallNo]++;<br>
<br>
+ LLVM_DEBUG(dbgs() << "Removing statements that are never executed...\n");<br>
+ removeEmptyDomainStmts();<br>
+<br>
LLVM_DEBUG(dbgs() << "Removing partial writes that never happen...\n");<br>
removeEmptyPartialAccesses();<br>
<br>
@@ -683,6 +719,7 @@ class Simplify : public ScopPass {<br>
virtual void releaseMemory() override {<br>
S = nullptr;<br>
<br>
+ EmptyDomainsRemoved = 0;<br>
OverwritesRemoved = 0;<br>
WritesCoalesced = 0;<br>
RedundantWritesRemoved = 0;<br>
<br>
diff --git a/polly/test/Simplify/func-b320a7.ll b/polly/test/Simplify/func-b320a7.ll<br>
new file mode 100644<br>
index 000000000000..22f6a5ac98f1<br>
--- /dev/null<br>
+++ b/polly/test/Simplify/func-b320a7.ll<br>
@@ -0,0 +1,82 @@<br>
+; RUN: opt %loadPolly -polly-simplify -polly-optree -analyze < %s | FileCheck %s -match-full-lines<br>
+<br>
+; <a href="http://llvm.org/PR47098" rel="noreferrer" target="_blank">llvm.org/PR47098</a><br>
+; Use-after-free by reference to Stmt remaining in InstStmtMap after removing it has been removed by Scop::simplifyScop.<br>
+; Removal happened in -polly-simplify, Reference was using in -polly-optree.<br>
+; Check that Simplify removes the definition of %0 as well of its use.<br>
+<br>
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"<br>
+target triple = "x86_64-pc-windows-msvc19.26.28806"<br>
+<br>
+@"?var_27@@3JA" = external dso_local local_unnamed_addr global i32, align 4<br>
+@"?var_28@@3HA" = external dso_local local_unnamed_addr global i32, align 4<br>
+<br>
+; Function Attrs: nofree norecurse nounwind uwtable<br>
+define dso_local void @"?test@@YAXHEQEAY3M@1BI@BJ@H@Z"(i32 %a, i8 %b, [12 x [2 x [24 x [25 x i32]]]]* nocapture readonly %c) local_unnamed_addr {<br>
+entry:<br>
+ br label %entry.split<br>
+<br>
+entry.split: ; preds = %entry<br>
+ %sext = shl i32 %a, 16<br>
+ %conv4 = ashr exact i32 %sext, 16<br>
+ %sub = add nsw i32 %conv4, -25941<br>
+ %cmp535 = icmp sgt i32 %sext, 1700069376<br>
+ br i1 %cmp535, label %<a href="http://for.cond8.preheader.lr.ph" rel="noreferrer" target="_blank">for.cond8.preheader.lr.ph</a>, label %for.cond.cleanup.critedge<br>
+<br>
+<a href="http://for.cond8.preheader.lr.ph" rel="noreferrer" target="_blank">for.cond8.preheader.lr.ph</a>: ; preds = %entry.split<br>
+ %conv10 = zext i8 %b to i64<br>
+ %sub11 = add nsw i64 %conv10, -129<br>
+ %cmp1232.not = icmp eq i64 %sub11, 0<br>
+ br i1 %cmp1232.not, label %for.cond8.preheader, label %<a href="http://for.cond8.preheader.us" rel="noreferrer" target="_blank">for.cond8.preheader.us</a><br>
+<br>
+<a href="http://for.cond8.preheader.us" rel="noreferrer" target="_blank">for.cond8.preheader.us</a>: ; preds = %<a href="http://for.cond8.preheader.lr.ph" rel="noreferrer" target="_blank">for.cond8.preheader.lr.ph</a>, %<a href="http://for.cond8.for.cond.cleanup13_crit_edge.us" rel="noreferrer" target="_blank">for.cond8.for.cond.cleanup13_crit_edge.us</a><br>
+ %<a href="http://e.036.us" rel="noreferrer" target="_blank">e.036.us</a> = phi i16 [ %<a href="http://add.us" rel="noreferrer" target="_blank">add.us</a>, %<a href="http://for.cond8.for.cond.cleanup13_crit_edge.us" rel="noreferrer" target="_blank">for.cond8.for.cond.cleanup13_crit_edge.us</a> ], [ 0, %<a href="http://for.cond8.preheader.lr.ph" rel="noreferrer" target="_blank">for.cond8.preheader.lr.ph</a> ]<br>
+ %<a href="http://idxprom.us" rel="noreferrer" target="_blank">idxprom.us</a> = sext i16 %<a href="http://e.036.us" rel="noreferrer" target="_blank">e.036.us</a> to i64<br>
+ br label %<a href="http://for.body14.us" rel="noreferrer" target="_blank">for.body14.us</a><br>
+<br>
+<a href="http://for.body14.us" rel="noreferrer" target="_blank">for.body14.us</a>: ; preds = %<a href="http://for.cond8.preheader.us" rel="noreferrer" target="_blank">for.cond8.preheader.us</a>, %<a href="http://for.body14.us" rel="noreferrer" target="_blank">for.body14.us</a><br>
+ %indvars.iv = phi i64 [ 0, %<a href="http://for.cond8.preheader.us" rel="noreferrer" target="_blank">for.cond8.preheader.us</a> ], [ %indvars.iv.next, %<a href="http://for.body14.us" rel="noreferrer" target="_blank">for.body14.us</a> ]<br>
+ %<a href="http://arrayidx19.us" rel="noreferrer" target="_blank">arrayidx19.us</a> = getelementptr inbounds [12 x [2 x [24 x [25 x i32]]]], [12 x [2 x [24 x [25 x i32]]]]* %c, i64 6, i64 2, i64 1, i64 %<a href="http://idxprom.us" rel="noreferrer" target="_blank">idxprom.us</a>, i64 %indvars.iv<br>
+ %0 = load i32, i32* %<a href="http://arrayidx19.us" rel="noreferrer" target="_blank">arrayidx19.us</a>, align 4, !tbaa !3<br>
+ store i32 0, i32* @"?var_28@@3HA", align 4, !tbaa !3<br>
+ %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1<br>
+ %exitcond.not = icmp eq i64 %indvars.iv.next, %sub11<br>
+ br i1 %exitcond.not, label %<a href="http://for.cond8.for.cond.cleanup13_crit_edge.us" rel="noreferrer" target="_blank">for.cond8.for.cond.cleanup13_crit_edge.us</a>, label %<a href="http://for.body14.us" rel="noreferrer" target="_blank">for.body14.us</a><br>
+<br>
+<a href="http://for.cond8.for.cond.cleanup13_crit_edge.us" rel="noreferrer" target="_blank">for.cond8.for.cond.cleanup13_crit_edge.us</a>: ; preds = %<a href="http://for.body14.us" rel="noreferrer" target="_blank">for.body14.us</a><br>
+ %<a href="http://add.us" rel="noreferrer" target="_blank">add.us</a> = add i16 %<a href="http://e.036.us" rel="noreferrer" target="_blank">e.036.us</a>, 4<br>
+ %<a href="http://conv2.us" rel="noreferrer" target="_blank">conv2.us</a> = sext i16 %<a href="http://add.us" rel="noreferrer" target="_blank">add.us</a> to i32<br>
+ %<a href="http://cmp5.us" rel="noreferrer" target="_blank">cmp5.us</a> = icmp sgt i32 %sub, %<a href="http://conv2.us" rel="noreferrer" target="_blank">conv2.us</a><br>
+ br i1 %<a href="http://cmp5.us" rel="noreferrer" target="_blank">cmp5.us</a>, label %<a href="http://for.cond8.preheader.us" rel="noreferrer" target="_blank">for.cond8.preheader.us</a>, label %for.cond.cleanup.critedge.loopexit38<br>
+<br>
+for.cond.cleanup.critedge.loopexit38: ; preds = %<a href="http://for.cond8.for.cond.cleanup13_crit_edge.us" rel="noreferrer" target="_blank">for.cond8.for.cond.cleanup13_crit_edge.us</a><br>
+ store i32 %0, i32* @"?var_27@@3JA", align 4, !tbaa !7<br>
+ br label %for.cond.cleanup.critedge<br>
+<br>
+for.cond.cleanup.critedge: ; preds = %for.cond8.preheader, %for.cond.cleanup.critedge.loopexit38, %entry.split<br>
+ ret void<br>
+<br>
+for.cond8.preheader: ; preds = %<a href="http://for.cond8.preheader.lr.ph" rel="noreferrer" target="_blank">for.cond8.preheader.lr.ph</a>, %for.cond8.preheader<br>
+ %e.036 = phi i16 [ %add, %for.cond8.preheader ], [ 0, %<a href="http://for.cond8.preheader.lr.ph" rel="noreferrer" target="_blank">for.cond8.preheader.lr.ph</a> ]<br>
+ %add = add i16 %e.036, 4<br>
+ %conv2 = sext i16 %add to i32<br>
+ %cmp5 = icmp sgt i32 %sub, %conv2<br>
+ br i1 %cmp5, label %for.cond8.preheader, label %for.cond.cleanup.critedge<br>
+}<br>
+<br>
+!3 = !{!4, !4, i64 0}<br>
+!4 = !{!"int", !5, i64 0}<br>
+!5 = !{!"omnipotent char", !6, i64 0}<br>
+!6 = !{!"Simple C++ TBAA"}<br>
+!7 = !{!8, !8, i64 0}<br>
+!8 = !{!"long", !5, i64 0}<br>
+<br>
+<br>
+; CHECK: Statistics {<br>
+; CHECK: Empty domains removed: 2<br>
+; CHECK: }<br>
+<br>
+; CHECK: After accesses {<br>
+; CHECK-NOT: Stmt_for_body14_us_a<br>
+; CHECK-NOT: Stmt_for_body14_us<br>
+; CHECK: }<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature">Tardyzentrismus verboten!</div>