This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mstorsjo on Jul 25 2016, 1:35 PM.

Details

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.

Diff Detail

Event Timeline

mstorsjo updated this revision to Diff 65410.Jul 25 2016, 1:35 PM
mstorsjo retitled this revision from to [ARM] Set a non-conflicting comment character for assembly in MSVC mode.
mstorsjo updated this object.
mstorsjo added a subscriber: llvm-commits.
rengolin added subscribers: jroelofs, compnerd.

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

jroelofs edited edge metadata.Jul 25 2016, 2:00 PM

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

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 edited edge metadata.Jul 25 2016, 6:28 PM

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 updated this revision to Diff 65486.Jul 26 2016, 3:26 AM
mstorsjo updated this object.
mstorsjo edited edge metadata.

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

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

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 updated this revision to Diff 65695.Jul 27 2016, 3:58 AM
mstorsjo updated this object.

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 added inline comments.Jul 27 2016, 4:01 AM
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 added inline comments.Jul 27 2016, 4:06 AM
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 updated this revision to Diff 65701.Jul 27 2016, 4:09 AM

Simplified the test, as suggested

rengolin accepted this revision.Jul 27 2016, 5:34 AM
rengolin added a reviewer: rengolin.

LGTM. Thanks! Do you have commit access?

This revision is now accepted and ready to land.Jul 27 2016, 5:34 AM

Do you have commit access?

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

rengolin closed this revision.Jul 27 2016, 5:45 AM

Committed in r276859