This is an archive of the discontinued LLVM Phabricator instance.

[MC] Fix PR45805: infinite recursion in assembler
ClosedPublic

Authored by thopre on May 7 2020, 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.May 7 2020, 6:22 AM
thopre updated this revision to Diff 262635.May 7 2020, 6:26 AM

Default initialize InsertionSuccess

thopre added a comment.May 7 2020, 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.May 11 2020, 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)May 11 2020, 11:25 AM
MaskRay added inline comments.May 11 2020, 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.May 12 2020, 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.May 12 2020, 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.May 12 2020, 9:27 AM

Initialize IsBeingLaidOut member variable.

thopre updated this revision to Diff 263706.May 13 2020, 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.May 21 2020, 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.

thopre added a comment.Jun 9 2020, 6:33 AM

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

I'll try to get to this today.

Ping?

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

I'll try to get to this today.

Ping @echristo ?

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

I'll try to get to this today.

Ping @echristo ?

Ping?

echristo accepted this revision.Jun 24 2020, 11:02 AM

One inline code comment otherwise lgtm.

llvm/lib/MC/MCFragment.cpp
61

A bit of vertical space here and bring the comment before the variable declaration here?

This revision is now accepted and ready to land.Jun 24 2020, 11:02 AM
thopre updated this revision to Diff 273356.Jun 25 2020, 7:41 AM
thopre marked an inline comment as done.

Address review comment

This revision was automatically updated to reflect the committed changes.