[ARM] Set a non-conflicting comment character for assembly in MSVC mode

[mstorsjo]

Summary

Currently, for ARMCOFFMCAsmInfoMicrosoft, no comment character is set, thus the default, '#', is used.

The hash character doesn't work as comment character in ARM assembly, since '#' is used for immediate values.

The comment character is set to ';', which is the comment character used by MS armasm.exe. (The microsoft armasm.exe uses a different directive syntax than what LLVM currently supports though, similar to ARM's armasm.)

This allows inline assembly with immediate constants to be built (and brings the assembly output from clang -S closer to being possible to assemble).

A test is added that verifies that ';' is correctly interpreted as comments in this mode, and verifies that assembling code that includes literal constants with a '#' works.

[rengolin]

I'm surprised this was never caught before... @compnerd, @jroelofs, that looks ok to me, but I'd rather hear your opinions. :)

[jroelofs]

I don't know much about Microsoft ASM. That being said... testcase?

[rengolin]

Well, @ is the comment string on Darwin and ELF, so it's possible that MS went with the default.

If it did, I don't understand how we didn't notice this before. Did we never test asm files on Windows? Or maybe only used Cygwin for asm files?

[compnerd]

The MS ASM support is nearly non-existent. Ive generally used the IAS, so I never printed the ASM for Microsoft mode. It makes sense that there are some silly bugs in the printing here, as Ive only focused on the ABI level compatibility. Ideally, we would produce assembly that is consumable by armasm here. The prefixed are set to the armasm style, and I think I prefer that the comment string also match (i.e. is set to ';').

[mstorsjo]

Changed the comment char from @ to ;, to match MS armasm.

[mstorsjo]

As for a testcase - can you suggest where and how to add one, if it is necessary for this fix?

[compnerd]

Yes, a test is necessary. test/MC/ARM/Windows would be the logical place for that. We should ensure that we get the right comment leader.

[mstorsjo]

Added a test for assembling code in armv7-windows mode, testing both '#' and ';'. I didn't add a test for something that would output assembly including comments (my attempt with "llvm-mc -filetype asm" didn't include any comments at all), but this test should at least cover the essentials here.

[mstorsjo]

test/MC/ARM/Windows/literals-comments.s

20 ↗

(On Diff #65695)

Once D22776 is solved, we should be able to add IMAGE_SCN_MEM_16BIT to the attributes checked here as well.

Or should I rather use a test with llvm-objdump -d, to check the encoding of the instructions? (The main point of the test is just that the assembly of the file shouldn't fail.)

[rengolin]

test/MC/ARM/Windows/literals-comments.s

20 ↗

(On Diff #65695)

I wouldn't think you'd need llvm-readobj at all. But would be good to make it clear what is testing, so I'd add a clear comment line:

function:
  ; this is a comment
  mov r0, #42 ; this # was not
  bx lr

[mstorsjo]

Simplified the test, as suggested

[rengolin]

LGTM. Thanks! Do you have commit access?

[mstorsjo]

In D22775#497486, @rengolin wrote:

Do you have commit access?

No, I don't, so feel free to commit it yourself.

[rengolin]

Committed in r276859

lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

 
 void ARMCOFFMCAsmInfoMicrosoft::anchor() { }
 
 ARMCOFFMCAsmInfoMicrosoft::ARMCOFFMCAsmInfoMicrosoft() {
   AlignmentIsInBytes = false;
 
   PrivateGlobalPrefix = "$M";
   PrivateLabelPrefix = "$M";
+  CommentString = "@";
 }
 
 void ARMCOFFMCAsmInfoGNU::anchor() { }
 
 ARMCOFFMCAsmInfoGNU::ARMCOFFMCAsmInfoGNU() {
   AlignmentIsInBytes = false;
   HasSingleParameterDotFile = true;