Page MenuHomePhabricator

[MC] Fix PR45805: infinite recursion in assembler
Needs ReviewPublic

Authored by thopre on Thu, May 7, 6:22 AM.

Details

Summary

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.

Diff Detail

Event Timeline

thopre created this revision.Thu, May 7, 6:22 AM
thopre updated this revision to Diff 262635.Thu, May 7, 6:26 AM

Default initialize InsertionSuccess

thopre added a comment.Thu, May 7, 8:13 AM

clang-tidy errors are:

llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:377:48: error: incomplete definition of type 'llvm::PointerLikeTypeTraits<const llvm::MCFragment *>' [clang-diagnostic-error]

return makeIterator(find_imp(ConstPtrTraits::getAsVoidPointer(Ptr)));
                                           ^

llvm-project/llvm/include/llvm/MC/MCAsmLayout.h:57:34: note: in instantiation of member function 'llvm::SmallPtrSetImpl<llvm::MCFragment *>::find' requested here

return LayingOutFragmentsSet.find(F) != LayingOutFragmentsSet.end();
                             ^

llvm-project/llvm/include/llvm/Support/PointerLikeTypeTraits.h:61:28: error: invalid application of 'alignof' to an incomplete type 'llvm::MCFragment' [clang-diagnostic-error]

detail::ConstantLog2<alignof(T)>::value;
                     ^

llvm-project/llvm/include/llvm/Support/PointerLikeTypeTraits.h:101:46: note: in instantiation of template class 'llvm::PointerLikeTypeTraits<llvm::MCFragment *>' requested here

static constexpr int NumLowBitsAvailable = NonConst::NumLowBitsAvailable;
                                           ^

llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:377:34: note: in instantiation of template class 'llvm::PointerLikeTypeTraits<const llvm::MCFragment *>' requested here

return makeIterator(find_imp(ConstPtrTraits::getAsVoidPointer(Ptr)));
                             ^

llvm-project/llvm/include/llvm/MC/MCAsmLayout.h:57:34: note: in instantiation of member function 'llvm::SmallPtrSetImpl<llvm::MCFragment *>::find' requested here

return LayingOutFragmentsSet.find(F) != LayingOutFragmentsSet.end();
                             ^

llvm-project/llvm/include/llvm/MC/MCAsmLayout.h:18:7: note: forward declaration of 'llvm::MCFragment'
class MCFragment;

^

Does that mean SmallPtrSet cannot be used for a pointer to incomplete type?

Thanks for the patch. This approach may be fine.

llvm/include/llvm/MC/MCAsmLayout.h
53

count

Why is called Validating instead of layouting?

llvm/lib/MC/MCAssembler.cpp
401
bool Inserted = LayingOutFragmentsSet.insert(F).second;
assert(Inserted && "Already being laid out!");
(void)Inserted;
llvm/lib/MC/MCExpr.cpp
581

Add a comment along the lines of: the enclosing fragment is being layouted. Avoid self loop.

llvm/test/MC/AsmParser/pr45805.s
1 ↗(On Diff #262635)

# is simpler

1 ↗(On Diff #262635)

binutils use prXXXX.s for regression tests. I find the names not very descriptive. How about layout-*.s? I currently haven't thought of a good name for the -* part.

7 ↗(On Diff #262635)

The test covers Layout->isFragmentValidating(FA) . You also need

.fill (fct_end - data_start), 1, 42

to test Layout->isFragmentValidating(FB)

10 ↗(On Diff #262635)

Delete the unneeded .long

thopre updated this revision to Diff 263225.Mon, May 11, 11:25 AM
thopre marked 8 inline comments as done.

Rework patch to catch to handle more complex layout interdependencies

llvm/include/llvm/MC/MCAsmLayout.h
53

To echo the isFragmentValid but yeah isFragmentLayouting is clearer.

thopre edited the summary of this revision. (Show Details)Mon, May 11, 11:25 AM
MaskRay added inline comments.Mon, May 11, 3:38 PM
llvm/include/llvm/MC/MCAsmLayout.h
35

I feel uneasy that we are adding another mutable member. Can mutable be avoided?

llvm/test/MC/AsmParser/layout-interdependency.s
7

Change the line number to [[#@LINE+1]]

9

Doesn't the second line have an error?

thopre updated this revision to Diff 263436.Tue, May 12, 7:38 AM

Record whether a fragment is being laid out in the fragment itself to avoid a mutable field in MCAsmLayout

thopre updated this revision to Diff 263437.Tue, May 12, 7:42 AM
thopre marked 3 inline comments as done.
  • Check for error message of second fill line
  • Check line number in error message based on CHECK directive's line number
thopre updated this revision to Diff 263457.Tue, May 12, 9:27 AM

Initialize IsBeingLaidOut member variable.

thopre updated this revision to Diff 263706.Wed, May 13, 7:11 AM

Remove no longer needed include

Ping?

The code and test look good to me now, but hope someone with MC experience can confirm.

thopre added a reviewer: rnk.Thu, May 21, 10:35 AM

Ping? Can anyone with MC experience review this? Am I missing someone as reviewer?

Ping? Can anyone with MC experience review this? Am I missing someone as reviewer?

I'll try to get to this today.