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