[polly] 6983741 - [Polly] Fix use-after-free.
Michael Kruse via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 26 11:17:34 PDT 2020
Thanks for the suggestions. Implemented
in c971b53b22a5cd43b54bf4773fe3c59ea1b805fb
and 6538fff37245921a0983d94c08af7e6cc120b3a9.
Michael
Am Mo., 24. Aug. 2020 um 12:42 Uhr schrieb David Blaikie <dblaikie at gmail.com
>:
>
>
> 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
>>
>
--
Tardyzentrismus verboten!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200826/33919ae7/attachment-0001.html>
More information about the llvm-commits
mailing list