Support arbitrary numbers of fragments between the symbols of kind
MCDataFragment. Getting the size of MCFillFragments seems to be less
straightforward https://llvm.org/doxygen/MCAssembler_8cpp_source.html#l00287.

In D69411#1742896, @jyknight wrote:

Making this work only on ".if" is IMO a non-starter, at least without understanding why. So I looked into what broke with llvm/test/MC/MachO/reloc-diff.s.

Basically, with LLVM's current representation, assuming that consecutive fragments are never moved with respect to each-other is invalid. In both ELF and Mach-O. In ELF, we have the ability to use numbered subsections that can insert new fragments between existing fragments. In Mach-O, fragments can effectively be turned into subsections with the 'subsections_via_symbols' directive. Committing this patch as-is would both be ugly and wrong, since we'd allow computing nonsense offsets -- even if we only restrict it to ".if".

I think it's a mistake that it works like that, and I'm going to spend a little bit of time to see if it'll be easy to fix this representational mistake in LLVM (in a separate patch), so that we have fragment lists which _are_ guaranteed to be kept exactly in the order they appear.

For this review, I have two requests:

1. Undo all the ".if"-only hacks. (understanding that it will cause some tests to fail, for now).
2. Fix the code to support arbitrary numbers of fragments between the symbols, of kinds MCFillFragment and MCDataFragment (and maybe others if they are fixed size -- I haven't looked through all of the kinds). Probably best would be to introduce a helper function to calculate the delta between two arbitrary symbols, given an optional layout (and either return an answer or a failure indication). This should make AttemptToFoldSymbolOffsetDifference significantly more straightforward.
   
   I believe these examples should work, and will after making the latter change: ` 1: .arch armv7a nop .arch armv4 nop .arch armv7a nop .if . - 1b != 0 .word 0x12345678 .endif
   
   2: nop .zero 0x10000 nop .if . - 2b == 4 .word 0x12345678 .endif `

In D69411#1742501, @nickdesaulniers wrote:

In D69411#1742030, @peter.smith wrote:

Fix a typo.

Fix test cases.

Thanks for this patch!

Remove the code redundant to D70062.

Handle cases at https://bugs.llvm.org/show_bug.cgi?id=41825.

In D69411#1735172, @peter.smith wrote:

Thanks for the update. I'll wait a few days to see what comments we get. If there are none then I guess there aren't any strong objections.

To clarify the remarks about resolving .if at layout time. I think that there are two major obstacles.

• MC assembles instructions once, if the condition in the .if is not satisfied the contents of the block aren't even parsed. For example, the following will assemble if the .if condition fails, but will fail to parse if the .if condition passes. We'd need to change MC to either reparse (effectively making it a multipass assembler), or to parse and remember all parts. This is doable but it isn't a small change. ` .text .if 0 You aren't parsing me are you? .else nop .endif `
• The second problem, and not one unique to llvm-mc, is that it is possible to write a program that doesn't converge, something like (not testable as it needs relaxation and late evaluation of .if): ` label: beq after In thumb 2 this 2-byte branch will be relaxed to a 4-byte branch if out of range. .if . - label == 2 .space 1024 * 1024 sufficient to make after out of range .endif after: nop ` In pass 1, beq is 2-bytes in size, the .if passes, beq is then relaxed to 4-bytes, which would make the .if fail, which then makes beq 2-bytes ... The interaction with relaxation could be fixed by making all size increases permanent. However I think it is possible to write multiple .if conditions that conflict. In Arm's old 2 - pass assembler (pass -1 find all sizes so layout is known, pass-2 encode instructions knowing layout), we had an error message when the equivalent of .if evaluated differently in subsequent passes. In summary, I think it would be a major change to MC to support something like late evaluation of .if in a reliable way.

In D69411#1732215, @peter.smith wrote:

I think that this needs some wider review for a couple of reasons. It looks like this is heading towards a special case evaluation just for .if. I'm not comfortable going too much further in the approval process as this is exceeding my knowledge of MC, and I'd like see some supporting opinions on whether this is the right thing to do. Will be worth adding some more reviewers, particularly someone familiar with Mach-O. You may need an RFC to the llvm-dev mailing list to draw some attention.

Add test cases.

Address some concerns.

Update tests.

Update test cases.

Update test cases and move their location.

Fix format.

In D69411#1725486, @nickdesaulniers wrote:

There are also some test cases for other ISA's in https://bugs.llvm.org/show_bug.cgi?id=41825.

In D69411#1725298, @peter.smith wrote:

When I run check-llvm with this patch applied I get 7 failures:

LLVM :: DebugInfo/Mips/delay-slot.ll
LLVM :: DebugInfo/Mips/dsr-fixed-objects.ll
LLVM :: DebugInfo/Mips/dsr-non-fixed-objects.ll
LLVM :: DebugInfo/Mips/fn-call-line.ll
LLVM :: MC/AsmParser/directive_fill_2.s
LLVM :: MC/MachO/reloc-diff.s
LLVM :: MC/X86/expand-var.s

This is applied to master revision 12c9ffd108345f643df98dfa8653af1a4311ed86 and the tests don't fail with master. Is it possible your most recent changes have broken something? I'm testing a release build of LLVM with clang, assertions enabled.

In D69411#1725261, @peter.smith wrote:

I'd like to see some more test cases for the error cases. Off the top of my head I can think of these cases:

// Align fragment in between MCDataFragments
9997: nop ;
      .align 4
      nop
.if . - 9997b == 4 ;

// Fill (I think) fragment in between MCDataFragments
9997: nop ;
      .space 4
      nop
.if . - 9997b == 4 ;

// Relaxable (in Thumb) fragment in between MCDataFragments
9997: nop ;
      b external
      nop
.if . - 9997b == 4 ;

// Constant pool between MCDataFragments,
9997:
      ldr r0,=0x12345678 ;
      .ltorg
      nop  
.if . - 9997b == 4 ;

There is also something called a MCCompactEncodingInstFragment that would cause this to fail, but I think that only exists as part of NaCl bundle locking, which I think isn't too much of a concern.

Made sure the patch pass check-clang and check-llvm.

Linux has been patched by to use normal section switching syntax https://lkml.org/lkml/2019/10/30/807. Abandon this LLVM patch as we should be fine without it.

In D69411#1725298, @peter.smith wrote:

When I run check-llvm with this patch applied I get 7 failures:

LLVM :: DebugInfo/Mips/delay-slot.ll
LLVM :: DebugInfo/Mips/dsr-fixed-objects.ll
LLVM :: DebugInfo/Mips/dsr-non-fixed-objects.ll
LLVM :: DebugInfo/Mips/fn-call-line.ll
LLVM :: MC/AsmParser/directive_fill_2.s
LLVM :: MC/MachO/reloc-diff.s
LLVM :: MC/X86/expand-var.s

This is applied to master revision 12c9ffd108345f643df98dfa8653af1a4311ed86 and the tests don't fail with master. Is it possible your most recent changes have broken something? I'm testing a release build of LLVM with clang, assertions enabled.

Update the test case.

Update the patch based on comments.

Update commit log.

Add a test case.

Fix wrong patch.

Add a test case.

@nickdesaulniers Maybe we should fix kernel instead? The fix will be cleaner that way.

Right now once we toggle SunStyleELFSectionSwitchSyntax on an architecture, all the section switching assembly is emitted on sun-style syntax, which is the reason why some test cases fail as they assume the regular syntax. To deal with this issue, I introduced a new variable that would allow the architecture to parse sun-style assembly while not emitting it.

In D69296#1721893, @thakis wrote:

Sorry about the breakage. I have reverted the commit. Will reland once I fix it.

Thanks much for the revert!

Going forward, when reverting things, please say in the revert commit message _why_ you're reverting (e.g. "breaks bots, see original review", or "breaks CodeGen/cfstring3.c on most bots", or similar) :)

In D69296#1721844, @thakis wrote:

This breaks tests everywhere, see eg http://45.33.8.238/linux/2607/step_6.txt

Please take a look and revert if it takes a while to fix.

And please always watch http://lab.llvm.org:8011/console for a bit after landing.

Update commit message.

Formatted.

Update comments.

In D69296#1717731, @peter.smith wrote:

I can see that this would allow "Sun Style" syntax for section switching that seems to be permitted for the Arm target. The question is why, and why just Arm? Looking at the GNU as code it looks like:

• This syntax was supported for Solaris, it only has tests for Solaris
• The syntax isn't gated by target, but it doesn't work on any target that uses # as the comment character (x86), it fails with a cryptic error message, this suggests it works on Arm and AArch64 by accident rather than by intention.
• The comment in MCAsmInfo.h states "instead of the normal ELF syntax", looking at the implementation it seems like both can be used at once as there isn't a parsing difference, however this might not be by design.

Update comments.

In D68764#1708513, @nickdesaulniers wrote:

Thanks for the patch and for following up on code review!

For autogenerated comment log.

Fix a typo

Update based on comments.

Update test cases.

Fix typos.

Expand test cases and rename test file.

Fix a typo.

In D68764#1703095, @MaskRay wrote:

acceet offset

typo

Fix bugs.

Remove typos.

Thank you for the fix! LGTM. Please give some time to @nickdesaulniers for comments.