[ARM] Uses "Sun Style" syntax for section switching

[jcai19]

Summary

Support "Sun Style" syntax for section switching ("#alloc,#write" etc).
https://bugs.llvm.org/show_bug.cgi?id=43759

[peter.smith]

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.

/// This is true if this target uses "Sun Style" syntax for section switching
/// ("#alloc,#write" etc) instead of the normal ELF syntax (,"a,w") in
/// .section directives.  Defaults to false.
bool SunStyleELFSectionSwitchSyntax = false;

So at the moment, I'm not convinced that doing this just for Arm and leaving the existing comment in MCAsmInfo.h in place is the right thing to do. If this is being done to be compatible with GNU, and for a good reason, then it should work on all targets that it works in GNU as and not just Arm.

[jcai19]

This comment was removed by jcai19.

[jcai19]

Update comments.

[jcai19]

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.

I agree, and it seems GNU assembler supports both on arm so I have updated the comments on MCAsmInfo.h.

/// This is true if this target uses "Sun Style" syntax for section switching
/// ("#alloc,#write" etc) instead of the normal ELF syntax (,"a,w") in
/// .section directives.  Defaults to false.
bool SunStyleELFSectionSwitchSyntax = false;

So at the moment, I'm not convinced that doing this just for Arm and leaving the existing comment in MCAsmInfo.h in place is the right thing to do. If this is being done to be compatible with GNU, and for a good reason, then it should work on all targets that it works in GNU as and not just Arm.

This is part of the effort to build Linux kernel with integrated assembler for 32-bit Arm (https://github.com/ClangBuiltLinux/linux/issues/744). Some assembly files are written with this syntax in the kernel, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mm/proc-v7.S?h=v5.4-rc4. I only ran into this issue on Arm so far and by grepping for #alloc I only found such syntax used for arm, sparc (SunStyleELFSectionSwitchSyntax already set to true), m68k and nds32 use such syntax. I am not sure if anyone is actually building m68k or nds32 with integrated assembler so that is why I only tried to turn it on for Arm.

[peter.smith]

Thanks for the context. I managed to find: https://lists.gt.net/linux/kernel/352032?do=post_view_threaded which asked about the difference in section syntax used in the kernel. The view at the time 2003 was that the forms where equivalent but there wasn't a firm preference. I've found a few other instances in other projects, although I've not got a large corpus of code to search through, the one's I've found are: Xen and CMSIS for low level driver code.

I think that this qualifies the use of the Solaris form as rare, but not insubstantial. On that basis I'm inclined to approve, does anyone else have any strong counter arguments?

llvm/include/llvm/MC/MCAsmInfo.h

218 ↗

(On Diff #226096)

nit: I'd use "alongside" rather than "along" here.

llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

78 ↗

(On Diff #226096)

nit: one blank line would match the style of the rest of the file.

80 ↗

(On Diff #226096)

Although SunStyleELFSectionSwitchSyntax comes under the section Data Emission Directives, I think that is a mistake in MCAsmInfo, as a Data Emission Directive is something like .word.

I suggest a comment like:
// The GNU assembler supports Sun style section switching for Arm targets, and it is used in projects like the Linux kernel.

[jcai19]

Update comments.

[peter.smith]

Thanks for the update. Looks good to me. Please can you apply the formatting change to the comment (80 columns) prior to commit.

llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

79 ↗

(On Diff #226287)

Can you break this comment into two lines so that it doesn't go over 80 columns? (https://llvm.org/docs/CodingStandards.html#source-code-width)

[MaskRay]

llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

79 ↗

(On Diff #226287)

@jcai19 You can run git diff -U0 --no-color 'HEAD^' | ~/llvm-project/clang/tools/clang-format/clang-format-diff.py -i -p1 to format the comment.

[nickdesaulniers]

llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

79 ↗

(On Diff #226287)

Or in vim:

shift+v to highlight visual lines of comments, then gqgq to rewrap lines. May require set cc=80.

[jcai19]

Formatted.

[jcai19]

llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

79 ↗

(On Diff #226287)

Thanks all for the comments.

[jcai19]

Update commit message.

[thakis]

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.

[jcai19]

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.

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

[thakis]

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) :)

[jcai19]

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) :)

Sounds good.

[jcai19]

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.

[jcai19]

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

[jcai19]

Add a test case.

[jcai19]

Accidentally uploaded a patch for different issue. Fixed it.

[MaskRay]

The original patch looks fine because the SunStyleELFSectionSwitchSyntax functionality is already there. Now we start to add IsCompatibleWithSunStyleELFSectionSwitchSyntax... Have we communicated the Solaris assembler's .section syntax problem to the Linux community?

[nickdesaulniers]

In D69296#1725741, @MaskRay wrote:

The original patch looks fine because the SunStyleELFSectionSwitchSyntax functionality is already there. Now we start to add IsCompatibleWithSunStyleELFSectionSwitchSyntax... Have we communicated the Solaris assembler's .section syntax problem to the Linux community?

https://github.com/ClangBuiltLinux/linux/issues/744#issuecomment-547633348

[jcai19]

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.

[nickdesaulniers]

In D69296#1727454, @jcai19 wrote:

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.

s/has been patched/has had a patch posted for review but not yet accepted or merged/

[MaskRay]

In D69296#1727454, @jcai19 wrote:

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.

From my viewpoint of reducing maintenance burden, thanks! I am not clear about the Linux kernel development model but I think we cannot say "Linux has been patched" yet because the patch sent by nickdesaulniers has not been merged into the mainline kernel :) (Though I think it is promising.)

llvm/lib/MC/MCExpr.cpp

static void AttemptToFoldSymbolOffsetDifference(
     const MCAssembler *Asm, const MCAsmLayout *Layout,
     const SectionAddrMap *Addrs, bool InSet, const MCSymbolRefExpr *&A,
     const MCSymbolRefExpr *&B, int64_t &Addend) {
   if (!A || !B)
     return;
 
   const MCSymbol &SA = A->getSymbol();
   const MCSymbol &SB = B->getSymbol();
+  const MCFragment *FragA = SA.getFragment();
+  const MCFragment *FragB = SB.getFragment();
 
   if (SA.isUndefined() || SB.isUndefined())
     return;
 
   if (!Asm->getWriter().isSymbolRefDifferenceFullyResolved(*Asm, A, B, InSet))
     return;
 
-  if (SA.getFragment() == SB.getFragment() && !SA.isVariable() &&
-      !SA.isUnset() && !SB.isVariable() && !SB.isUnset()) {
+  if (FragA == FragB && !SA.isVariable() && !SA.isUnset() && !SB.isVariable() &&
+      !SB.isUnset()) {
+
     Addend += (SA.getOffset() - SB.getOffset());
 
     // Pointers to Thumb symbols need to have their low-bit set to allow
     // for interworking.
     if (Asm->isThumbFunc(&SA))
       Addend |= 1;
 
     // If symbol is labeled as micromips, we set low-bit to ensure
     // correct offset in .gcc_except_table
     if (Asm->getBackend().isMicroMips(&SA))
       Addend |= 1;
 
     // Clear the symbol expr pointers to indicate we have folded these
     // operands.
     A = B = nullptr;
     return;
   }
 
-  if (!Layout)
-    return;
+  const MCSection &SecA = *FragA->getParent();
+  const MCSection &SecB = *FragB->getParent();
 
-  const MCSection &SecA = *SA.getFragment()->getParent();
-  const MCSection &SecB = *SB.getFragment()->getParent();
+  if ((&SecA != &SecB) && !Addrs)
+    return;
 
+     // Pointers to Thumb symbols need to have their low-bit set to allow
    if ((&SecA != &SecB) && !Addrs)
      return;
 
+   if (SecB.getFragmentList().getNextNode(*FragB) == FragA &&
+       dyn_cast<MCDataFragment>(FragA) && dyn_cast<MCDataFragment>(FragB)) {
+     Addend +=
+         (SA.getOffset() + (cast<MCDataFragment>(FragB))->getContents().size() -
+          SB.getOffset());
+
+    // Pointers to Thumb symbols need to have their low-bit set to allow
+    // for interworking.
+    if (Asm->isThumbFunc(&SA))
+      Addend |= 1;
+
+    // If symbol is labeled as micromips, we set low-bit to ensure
+    // correct offset in .gcc_except_table
+    if (Asm->getBackend().isMicroMips(&SA))
+      Addend |= 1;
+
+    // Clear the symbol expr pointers to indicate we have folded these
+    // operands.
+    A = B = nullptr;
+    return;
+  }
+
+  if (!Layout)
+    return;
+
   // Eagerly evaluate.
   Addend += Layout->getSymbolOffset(A->getSymbol()) -
             Layout->getSymbolOffset(B->getSymbol());
   if (Addrs && (&SecA != &SecB))
     Addend += (Addrs->lookup(&SecA) - Addrs->lookup(&SecB));
 
   // Pointers to Thumb symbols need to have their low-bit set to allow
   // for interworking.

llvm/test/MC/AsmParser/directive_if_with_dot_symbol.s

+@ RUN: llvm-mc -filetype=obj -triple armv7a-linux-gnueabihf %s -o -
+
+nop
+.arch_extension sec
+9997: nop ; .if . - 9997b == 2 ; nop ; .endif ;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: error: expected absolute expression