This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jcai19 on Oct 21 2019, 9:36 PM.

Details

Event Timeline

jcai19 created this revision.Oct 21 2019, 9:36 PM

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 updated this revision to Diff 226095.Oct 22 2019, 9:13 PM
This comment was removed by jcai19.
jcai19 updated this revision to Diff 226096.Oct 22 2019, 9:27 PM

Update comments.

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.

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

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

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

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

80

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 added a subscriber: MaskRay.Oct 23 2019, 9:51 AM
jcai19 updated this revision to Diff 226287.Oct 24 2019, 10:26 AM

Update comments.

jcai19 marked 2 inline comments as done.Oct 24 2019, 10:26 AM
peter.smith accepted this revision.Oct 25 2019, 1:58 AM

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

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)

This revision is now accepted and ready to land.Oct 25 2019, 1:58 AM
MaskRay added inline comments.Oct 25 2019, 9:49 AM
llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp
79

@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.

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

Or in vim:

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

jcai19 updated this revision to Diff 226460.Oct 25 2019, 10:55 AM

Formatted.

jcai19 marked 4 inline comments as done.Oct 25 2019, 10:56 AM
jcai19 added inline comments.
llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp
79

Thanks all for the comments.

jcai19 updated this revision to Diff 226476.Oct 25 2019, 12:01 PM
jcai19 marked an inline comment as done.

Update commit message.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 25 2019, 1:47 PM

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.

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.

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

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 reopened this revision.Oct 25 2019, 4:53 PM
This revision is now accepted and ready to land.Oct 25 2019, 4:53 PM
jcai19 updated this revision to Diff 226523.EditedOct 25 2019, 4:53 PM

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.

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

jcai19 updated this revision to Diff 226694.Oct 28 2019, 10:08 AM

Add a test case.

jcai19 updated this revision to Diff 226697.EditedOct 28 2019, 10:17 AM

Accidentally uploaded a patch for different issue. Fixed it.

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?

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 abandoned this revision.Oct 30 2019, 11:02 AM

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.

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/

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