Page MenuHomePhabricator

[MC] Add MCSubtargetInfo to MCAlignFragment [NFC]
Needs ReviewPublic

Authored by peter.smith on Apr 23 2018, 7:23 AM.

Details

Summary

In preparation for passing the MCSubtargetInfo (STI) through to writeNops so that it can use the STI in operation at the time, we need to record the STI in operation when a MCAlignFragment may write nops as padding. The STI is currently unused, a further patch will pass it through to writeNops.

There are many places that can create an MCAlignFragment, in most cases we can find out the STI in operation at the time. In a few places this isn't possible as we are in initialisation or finalisation, or are emitting constant pools. When possible I've tried to find the most appropriate existing fragment to obtain the STI from, when none is available use the per module STI.

For constant pools we don't actually need to use EmitCodeAlign as the constant pools are data anyway so falling through into it via an executable NOP is no better than falling through into data padding.

This is part 3 of 4 of a patch series to pass MCSubtargetInfo to writeNops so that the ARM and X86 AsmBackends can remove the per-module STI that is used to decide which nop encodings can be used.
1 [MC] Move bundling and MCSubtargetInfo to MCEncodedFragment [NFC]
2 [MC] Add MCSubtargetInfo to MCPaddingFragment [NFC]
3 [MC] Add MCSubtargetInfo to MCAlignFragment [NFC]
4 [MC] Use local MCSubtargetInfo in writeNops

This patch depends on D44928.

Diff Detail

Event Timeline

peter.smith created this revision.Apr 23 2018, 7:23 AM

Rebased [NFC]

asb added a comment.Aug 15 2018, 6:51 AM

You will also need to patch at least clang tools/driver/cc1as_main.cpp due to the change to InitSections.

Mechanically the changes seem fine. There's one aspect I'd appreciate further comment on: having to perform getLastSubtargetInfo seems pragmatic but feels ugly. Are there cases where this might be incorrect? Should there be a FIXME indicating that some superior solution for ensuring access to an appropate MCSubtargetInfo should be implemented?

In D45961#1200623, @asb wrote:

You will also need to patch at least clang tools/driver/cc1as_main.cpp due to the change to InitSections.

Mechanically the changes seem fine. There's one aspect I'd appreciate further comment on: having to perform getLastSubtargetInfo seems pragmatic but feels ugly. Are there cases where this might be incorrect? Should there be a FIXME indicating that some superior solution for ensuring access to an appropate MCSubtargetInfo should be implemented?

Thanks for the review. I think that in the context that it is used getLastSubtargetInfo() is at, however that doesn't mean that it could potentially be used in a different context in the future.

  • For a section with ConstantPools we are guaranteed to have at least one MCFragment with a valid MCSubtarget as there must exist a fragment with an instruction that needs a constant pool entry. In theory given that constant-pools are not instructions it shouldn't actually matter what the encoding of the NOP is because there will most likely be a crash if control flow gets to the constant pool.
  • For the Mips RoundSectionSizes option then if the last fragment has instructions we can use its MCSubtarget for the NOP and if not then we can use the per translation unit MCSubtarget. Moreover Mips doesn't even use the Subtarget when determining the NOP encoding anyway.

With this in mind it may be better to change the code so that getLastSubtargetInfo() is no longer required.

  • In ConstantPools.cpp::emitEntries() don't use EmitCodeAlignment(), use EmitValueToAlignment() instead. As mentioned above valid code shouldn't ever fall through the padding into the constant pool.
  • In Mips we can just use the global STI as there is only one NOP encoding anyway.

I don't think that there are many alternative implementations unless we make the getSubtargetInfo() return a pointer rather than a reference. I'm not sure if that would be a change worth making.

If it sounds ok to you, I can post another patch without getLastSubtargetInfo()?

One possible option might be to fix the surrounding code so that getLastSubtargetInfo() isn't needed.

asb added a comment.Aug 16 2018, 6:38 AM

I like the idea of rejigging this so getLastSubtargetInfo isn't needed.

Using EmitValueToAlignment in constant pools seems more correct to me anyway - or at least, I can't think of a disadvantage.

peter.smith edited the summary of this revision. (Show Details)

I've uploaded a new version that doesn't use getLastSubtargetInfo(). Thanks for the warning about clang tools/driver/cc1as_main.cpp I've not seen anywhere else in the project that uses InitSections(). The code there will become:

Str.get()->InitSections(Opts.NoExecStack, *STI);

As the STI has been computed earlier. I'm happy to post another review if people prefer? Otherwise I could just make the change immediately after where this to be committed.

Uploaded rebased diff, no functional changes.