Page MenuHomePhabricator

[AArch64] Use '//' as comment string for MSVC assembly
ClosedPublic

Authored by mstorsjo on Feb 8 2021, 5:46 AM.

Details

Summary

As the actual MSVC toolset doesn't use the GAS-style assembly that Clang/LLVM produces and consumes, there's no reference for what string to use for e.g. comments when building with a MSVC triple.

This frees up the use of semicolon as separator string, just like was done for GNU targets in 23413195649d0cf6f3860ae8b5fb115b35032075. (Previously, both the separator and comment strings were set to the same, a semicolon.)

Compiler-rt extensively uses separator chars in its assembly, and that assembly should be buildable with clang-cl for MSVC too.

Diff Detail

Event Timeline

mstorsjo created this revision.Feb 8 2021, 5:46 AM
mstorsjo requested review of this revision.Feb 8 2021, 5:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 5:46 AM

Looks good to me. It might be prudent to switch comment string from ";" to "//" for 32-bit ARM COFF as well, but that should be a separate change anyway.

tambre accepted this revision.Feb 8 2021, 8:11 AM

Thanks! Maybe worth a changelog entry?

This revision is now accepted and ready to land.Feb 8 2021, 8:11 AM
rnk accepted this revision.Feb 8 2021, 9:39 AM

I guess the actual rules for the armasm tool that MSVC uses are documented here:
https://developer.arm.com/documentation/dui0802/b/Via-File-Syntax/Via-file-syntax

Lines beginning with a semicolon (;) or a hash (#) character as the first nonwhitespace character are comment lines. If a semicolon or hash character appears anywhere else in a line, it is not treated as the start of a comment. For example:
-o objectname.axf ;this is not a comment
A comment ends at the end of a line, or at the end of the file. There are no multi-line comments, and there are no part-line comments.

But we want to support part-line comments, so I guess we have to use //.

lgtm

Thanks! Maybe worth a changelog entry?

I think that's a bit overkill TBH

In D96259#2548965, @rnk wrote:

I guess the actual rules for the armasm tool that MSVC uses are documented here:
https://developer.arm.com/documentation/dui0802/b/Via-File-Syntax/Via-file-syntax

Lines beginning with a semicolon (;) or a hash (#) character as the first nonwhitespace character are comment lines. If a semicolon or hash character appears anywhere else in a line, it is not treated as the start of a comment. For example:
-o objectname.axf ;this is not a comment
A comment ends at the end of a line, or at the end of the file. There are no multi-line comments, and there are no part-line comments.

But we want to support part-line comments, so I guess we have to use //.

Yes - and the armasm syntax is pretty much orthogonal here anyway.

In the same way on x86, clang-cl doesn't interpret .S files as if they were in masm format as that's something totally different (we have llvm-ml for that), and when instructed to output asm, it outputs gas-like syntax (although maybe it should produce masm-like assembly for consistency with msvc?). So in both cases, masm and armasm syntax are entirely different from what's put into or interpreted from gas-like .S assembly.

This revision was landed with ongoing or failed builds.Feb 8 2021, 12:30 PM
This revision was automatically updated to reflect the committed changes.

@rnk @tstellar Are you ok with backporting this to 12.x (after cooking in main for a couple days), to fix the issue that is brought up in https://reviews.llvm.org/rGb2851aea80e5a8f0cfd6c3c5a56a6b00fb28c6b6?

@rnk @tstellar Are you ok with backporting this to 12.x (after cooking in main for a couple days), to fix the issue that is brought up in https://reviews.llvm.org/rGb2851aea80e5a8f0cfd6c3c5a56a6b00fb28c6b6?

This has been in main for a couple days now and I haven't heard back about it. Anyone want to give a thumbs up for backporting to 12.x, @rnk @tstellar?

Can you file a bug for the backport request?

Can you file a bug for the backport request?

(Adding a reference here as well) I guess https://bugs.llvm.org/show_bug.cgi?id=49075 will do for that.