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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97982