From 6c67ee0f58323fe39db84635ef8cea629c2214b4 Mon Sep 17 00:00:00 2001 From: Thomas Preud'homme Date: Tue, 5 May 2020 19:11:04 +0100 Subject: [PATCH] [MC] Fix PR45805: infinite recursion in assembler Give up folding an expression if the fragment of one of the operands would require laying out a fragment already being laid out. This prevents hitting an infinite recursion when a fill size expression refers to a later fragment since computing the offset of that fragment would require laying out the fill fragment and thus computing its size expression. Reviewed By: echristo Differential Revision: https://reviews.llvm.org/D79570 --- llvm/include/llvm/MC/MCAsmLayout.h | 4 ++++ llvm/include/llvm/MC/MCFragment.h | 3 +++ llvm/lib/MC/MCAssembler.cpp | 4 ++++ llvm/lib/MC/MCExpr.cpp | 15 +++++++++---- llvm/lib/MC/MCFragment.cpp | 21 ++++++++++++++++++- .../MC/AsmParser/layout-interdependency.s | 10 +++++++++ 6 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 llvm/test/MC/AsmParser/layout-interdependency.s diff --git a/llvm/include/llvm/MC/MCAsmLayout.h b/llvm/include/llvm/MC/MCAsmLayout.h index 45ac96f0b81e..f95af210a28f 100644 --- a/llvm/include/llvm/MC/MCAsmLayout.h +++ b/llvm/include/llvm/MC/MCAsmLayout.h @@ -49,6 +49,10 @@ public: /// Get the assembler object this is a layout for. MCAssembler &getAssembler() const { return Assembler; } + /// \returns whether the offset of fragment \p F can be obtained via + /// getFragmentOffset. + bool canGetFragmentOffset(const MCFragment *F) const; + /// Invalidate the fragments starting with F because it has been /// resized. The fragment's size should have already been updated, but /// its bundle padding will be recomputed. diff --git a/llvm/include/llvm/MC/MCFragment.h b/llvm/include/llvm/MC/MCFragment.h index 4c8a895592ef..fb7166e82c09 100644 --- a/llvm/include/llvm/MC/MCFragment.h +++ b/llvm/include/llvm/MC/MCFragment.h @@ -65,6 +65,9 @@ private: FragmentType Kind; + /// Whether fragment is being laid out. + bool IsBeingLaidOut; + protected: bool HasInstructions; diff --git a/llvm/lib/MC/MCAssembler.cpp b/llvm/lib/MC/MCAssembler.cpp index 47b25c5a299b..c1a39cada7e2 100644 --- a/llvm/lib/MC/MCAssembler.cpp +++ b/llvm/lib/MC/MCAssembler.cpp @@ -397,6 +397,9 @@ void MCAsmLayout::layoutFragment(MCFragment *F) { assert((!Prev || isFragmentValid(Prev)) && "Attempt to compute fragment before its predecessor!"); + assert(!F->IsBeingLaidOut && "Already being laid out!"); + F->IsBeingLaidOut = true; + ++stats::FragmentLayouts; // Compute fragment offset and size. @@ -404,6 +407,7 @@ void MCAsmLayout::layoutFragment(MCFragment *F) { F->Offset = Prev->Offset + getAssembler().computeFragmentSize(*this, *Prev); else F->Offset = 0; + F->IsBeingLaidOut = false; LastValidFragment[F->getParent()] = F; // If bundling is enabled and this fragment has instructions in it, it has to diff --git a/llvm/lib/MC/MCExpr.cpp b/llvm/lib/MC/MCExpr.cpp index 429531165585..fdb83aeef5eb 100644 --- a/llvm/lib/MC/MCExpr.cpp +++ b/llvm/lib/MC/MCExpr.cpp @@ -547,8 +547,10 @@ static void AttemptToFoldSymbolOffsetDifference( if (!Asm->getWriter().isSymbolRefDifferenceFullyResolved(*Asm, A, B, InSet)) return; - if (SA.getFragment() == SB.getFragment() && !SA.isVariable() && - !SA.isUnset() && !SB.isVariable() && !SB.isUnset()) { + MCFragment *FA = SA.getFragment(); + MCFragment *FB = SB.getFragment(); + if (FA == FB && !SA.isVariable() && !SA.isUnset() && !SB.isVariable() && + !SB.isUnset()) { Addend += (SA.getOffset() - SB.getOffset()); // Pointers to Thumb symbols need to have their low-bit set to allow @@ -570,12 +572,17 @@ static void AttemptToFoldSymbolOffsetDifference( if (!Layout) return; - const MCSection &SecA = *SA.getFragment()->getParent(); - const MCSection &SecB = *SB.getFragment()->getParent(); + const MCSection &SecA = *FA->getParent(); + const MCSection &SecB = *FB->getParent(); if ((&SecA != &SecB) && !Addrs) return; + // One of the symbol involved is part of a fragment being laid out. Quit now + // to avoid a self loop. + if (!Layout->canGetFragmentOffset(FA) || !Layout->canGetFragmentOffset(FB)) + return; + // Eagerly evaluate. Addend += Layout->getSymbolOffset(A->getSymbol()) - Layout->getSymbolOffset(B->getSymbol()); diff --git a/llvm/lib/MC/MCFragment.cpp b/llvm/lib/MC/MCFragment.cpp index 5252c3f14672..8e90e07a4dbf 100644 --- a/llvm/lib/MC/MCFragment.cpp +++ b/llvm/lib/MC/MCFragment.cpp @@ -48,6 +48,25 @@ bool MCAsmLayout::isFragmentValid(const MCFragment *F) const { return F->getLayoutOrder() <= LastValid->getLayoutOrder(); } +bool MCAsmLayout::canGetFragmentOffset(const MCFragment *F) const { + MCSection *Sec = F->getParent(); + MCSection::iterator I; + if (MCFragment *LastValid = LastValidFragment[Sec]) { + // Fragment already valid, offset is available. + if (F->getLayoutOrder() <= LastValid->getLayoutOrder()) + return true; + I = ++MCSection::iterator(LastValid); + } else + I = Sec->begin(); + + // A fragment ordered before F is currently being laid out. + const MCFragment *FirstInvalidFragment = &*I; + if (FirstInvalidFragment->IsBeingLaidOut) + return false; + + return true; +} + void MCAsmLayout::invalidateFragmentsFrom(MCFragment *F) { // If this fragment wasn't already valid, we don't need to do anything. if (!isFragmentValid(F)) @@ -235,7 +254,7 @@ void ilist_alloc_traits::deleteNode(MCFragment *V) { V->destroy(); } MCFragment::MCFragment(FragmentType Kind, bool HasInstructions, MCSection *Parent) : Parent(Parent), Atom(nullptr), Offset(~UINT64_C(0)), LayoutOrder(0), - Kind(Kind), HasInstructions(HasInstructions) { + Kind(Kind), IsBeingLaidOut(false), HasInstructions(HasInstructions) { if (Parent && !isa(*this)) Parent->getFragmentList().push_back(this); } diff --git a/llvm/test/MC/AsmParser/layout-interdependency.s b/llvm/test/MC/AsmParser/layout-interdependency.s new file mode 100644 index 000000000000..6e275e00d9ec --- /dev/null +++ b/llvm/test/MC/AsmParser/layout-interdependency.s @@ -0,0 +1,10 @@ +# RUN: not llvm-mc --filetype=obj %s -o /dev/null 2>&1 | FileCheck %s + +fct_end: + +# CHECK: layout-interdependency.s:[[#@LINE+1]]:7: error: expected assembly-time absolute expression +.fill (data_start - fct_end), 1, 42 +# CHECK: layout-interdependency.s:[[#@LINE+1]]:7: error: expected assembly-time absolute expression +.fill (fct_end - data_start), 1, 42 + +data_start: