[PATCH] D97982: [MC] Introduce NeverAlign fragment type

[Amir Ayupov via Phabricator via llvm-commits]

Amir added inline comments.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:389
+    const MCFragment *NF = F.getNextNode();
+    uint64_t Offset = Layout.getFragmentOffset(&NAF);
+    size_t NextFragSize = 0;
----------------
efriedma wrote:
> Amir wrote:
> > efriedma wrote:
> > > efriedma wrote:
> > > > efriedma wrote:
> > > > > Recomputing the size of the fragment here is suspicious.  For other fragments, computeFragmentSize is independent from the layout of the section.  If the size needs to be changed based on the layout, it's mutated by MCAssembler::layoutSectionOnce.
> > > > > 
> > > > > I realize FT_Org doesn't follow this rule, but that's not a great example to follow.  The following currently crashes:
> > > > > 
> > > > >     foo: .org bar
> > > > >     bar:
> > > > My biggest concern with the current approach is the potential for an infinite loop.  There isn't any protection against the following fragment's offset moving backwards, which could lead to infinitely oscillating fragment sizes.
> > > Thought a little more.
> > > 
> > > Probably the simplest way to ensure that computeFragmentSize() is sane is to establish the following rule: it should never examine fragments after the current fragment in the section.  If we logically need to examine any fragment after the current fragment, we need to do that using relaxation, inside MCAssembler::layoutSectionOnce.  This means we can compute the "current" layout using a single pass computeFragmentSize() calls.
> > > 
> > > Under that rule, FT_Align is fine because it only cares about its own offset.  The FT_Org code crashes because it tries to look at fragments after itself.  FT_NeverAlign is... marginal.  The size of an MCDataFragment is fixed, so that doesn't really count as looking forward.  But I suspect MCRelaxableFragment doesn't work right: if we do end up relaxing the fragment, I'm not sure we recompute everything we need to.
> > @efriedma: do you have a specific corner case in mind? Wouldn’t it be covered by a test case with MCRelaxableFragment following NA fragment?
> You'd want a testcase where there's an MCRelaxableFragment that gets relaxed after we computeFragmentSize() access.  Something like a forward jump more than 128 bytes.  Then make sure we chose the right size for the MCNeverAlignFragment.
> 
> It looks like in your regression test, the fragment after is always an MCDataFragment?
Let me experiment with the idea. The second test is a NeverAlign followed by MCRelaxableFragment (cmpl insn).
Actually we wanted to avoid computing the size using relaxation (as BoundaryAlign does) because it slows down the layout and appears unnecessary if we can handle the RelaxableFragment following NA.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97982/new/

https://reviews.llvm.org/D97982