Page MenuHomePhabricator

[mips] [IAS] Implement the .asciiz directive.
ClosedPublic

Authored by tomatabacu on Feb 10 2015, 3:16 AM.

Details

Summary

This directive is exactly the same as .asciz, except it's only used by MIPS.
It is used to store null terminated strings in object files.

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 19657.Feb 10 2015, 3:16 AM
tomatabacu retitled this revision from to [mips] [IAS] Implement the .asciiz directive..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu added a subscriber: Unknown Object (MLST).

Rafael, I was wondering if you could help me with the implementation of this directive ?

The .asciiz assembler directive is the same as .asciz, except it's not accepted by all architectures (the only in-tree users are MIPS and XCore).
The problem with MIPS is that it should accept both .asciiz and .asciz (I don't know what XCore is supposed to do, since they haven't implemented an XCoreAsmParser).

I see three ways to implement .ascciiz:

  1. Add .asciiz to the DirectiveKindMap in AsmParser (which will make it available for all architectures).
  2. Make AsmParser::parseDirectiveAscii public and use it for parsing .asciiz in MipsAsmParser.cpp.
  3. Make AsmParser::parseEscapedString public and use it in a reimplementation of AsmParser::parseDirectiveAscii in MipsAsmParser.cpp.

In this patch, I have chosen the 3rd approach, as it seemed the least intrusive.

dsanders edited edge metadata.Feb 26 2015, 3:26 AM

I'm of the opinion that solution #1 is preferable, primarily for the simplicity of the implementation. I believe there are a couple other targets that define .asciiz as well as .asciz. As you say, this will make it available for all targets but I don't think that's a big problem. I actually think it's nice to have the inter-target consistency.

Documentation somewhere for .asciiz? I looked in binutils and couldn't find anything.

-eric

Documentation somewhere for .asciiz? I looked in binutils and couldn't find anything.

-eric

Unfortunately, it's undocumented. I think it's a remnant from some old MIPS assemblers.

I'm actually starting to have doubts about supporting .asciiz in LLVM.

I've grepped the Linux kernel and I've found only 2 uses of .asciiz, both in MIPS assembly,
while .asciz is used a lot more, in many different architectures, including MIPS (only once, but that means it's not a compatibility issue).
FreeBSD has a few more uses of .asciiz (about 8, all of them in MIPS assembly), but .asciz still outnumbers .asciiz.

It might be worth it to try to push Linux and FreeBSD to switch to .asciz for MIPS assembly, but, on the other hand, .asciiz is not difficult to support, just a bit ugly.

I haven't seen any documentation for it either. Just existing usage.

If it's really used as little as it seems then removing support for it from binutils and correcting Linux/FreeBSD/etc. makes sense to me. We'd need binutils to agree to drop support for it though. Otherwise, any patch to switch .asciiz to .asciz will find it difficult to get merged.

We'd need binutils to agree to drop support for it though. Otherwise, any patch to switch .asciiz to .asciz will find it difficult to get merged.

I don't think we *need* GAS to agree to drop support for .asciiz.

To me, this .asciiz situation feels like the VLAIS situation in Clang.
It's true that if GAS stopped supporting .asciiz, it would force everyone to switch, but I think we need to be convincing the developers to actively use and
support the IAS (and the whole Clang/LLVM toolchain), instead of convincing GAS to conform to our implementation.

In this regard, Linux is more problematic, as it seems to me like Clang/LLVM is used by a small subset of their dev community at the moment.

Also, I don't see why people would oppose switching to .asciz. GAS also supports .asciz for MIPS assembly, so there's no friction in switching.

We'd need binutils to agree to drop support for it though. Otherwise, any patch to switch .asciiz to .asciz will find it difficult to get merged.

I don't think we *need* GAS to agree to drop support for .asciiz.

To me, this .asciiz situation feels like the VLAIS situation in Clang.

The difference between the two is that lack of support for VLAIS is easy to justify on the ground that it requires a significant rework to the way all structure members are accessed. On the other hand, it's easy to look at the .asciiz directive and think "surely that's a trivial change, why don't you fix your assembler?".

In short, the battle is worth fighting for VLAIS but it's not worth it for .asciiz.

It's true that if GAS stopped supporting .asciiz, it would force everyone to switch, but I think we need to be convincing the developers to actively use and
support the IAS (and the whole Clang/LLVM toolchain), instead of convincing GAS to conform to our implementation.

In this regard, Linux is more problematic, as it seems to me like Clang/LLVM is used by a small subset of their dev community at the moment.

You won't be able to get everyone to agree to support both assemblers. It's hard enough to convince the developers within a single company to support multiple tools for the same purpose. You'll always have a subset that cares about building with GAS, as subset that cares about IAS, and maybe a subset that cares about both. Together, the three groups will move in the general direction of compatibility but it will break every so often.

Also, I don't see why people would oppose switching to .asciz. GAS also supports .asciz for MIPS assembly, so there's no friction in switching.

Me neither, but we've both witnessed first hand how difficult it is to get the other clang compatibility fixes merged into Linux. It will be a lot easier to support .asciiz in IAS. Not supporting .asciiz is an option, but the battle will be hard (and neverending) while GAS continues to support both directives.

tomatabacu updated this revision to Diff 23584.Apr 10 2015, 3:50 AM
tomatabacu edited edge metadata.

Added a way to create aliases to directives in the target-independent AsmParser.
Add .asciiz as an alias of .asciz in MipsAsmParser.

By doing this, we can reuse the existing implementation of .asciz without enabling .asciiz for all the other supported architectures.

dsanders accepted this revision.Apr 20 2015, 8:22 AM
dsanders edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 20 2015, 8:22 AM
echristo accepted this revision.Apr 20 2015, 2:15 PM
echristo added a reviewer: echristo.

Fine with me. Thanks!

-eric

tomatabacu closed this revision.Apr 21 2015, 4:53 AM