Page MenuHomePhabricator

[AsmParser] Make generic directives and aliases case insensitive.
ClosedPublic

Authored by DavidSpickett on Jan 14 2020, 1:58 AM.

Details

Summary

GCC will accept any case for assembler directives.
For example ".abort" and ".ABORT" (even ".aBoRt")
are equivalent.

https://sourceware.org/binutils/docs/as/Pseudo-Ops.html#Pseudo-Ops
"The names are case insensitive for most targets,
and usually written in lower case."

Change llvm-mc to accept any case for generic directives
or aliases of those directives.

This for Bugzilla #39527.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jan 14 2020, 1:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 1:58 AM

Direct link to Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=39527

I chose to test one directive and one directive alias, instead of trying to create a file with one of every alias. Maybe it should have a few more than one of each though.

"The names are case insensitive for most targets, and usually written in lower case."

This suggests that there are exceptions, do you which targets and directives don't follow this?

llvm/lib/MC/MCParser/AsmParser.cpp
5323

Why is this change needed, when all the strings are already lower-case?

DavidSpickett marked an inline comment as done.Jan 15 2020, 1:06 AM
DavidSpickett added inline comments.
llvm/lib/MC/MCParser/AsmParser.cpp
5323

An attempt to make it harder to put mixed case in, though you could just DirectiveKindMap[<bla>] directly anyway.

I'll leave initializeDirective as it is and just add a comment at the top of the function saying that they should be lower case.

DavidSpickett marked an inline comment as not done.Jan 15 2020, 1:06 AM
DavidSpickett added a comment.EditedJan 15 2020, 2:02 AM

This suggests that there are exceptions, do you which targets and directives don't follow this?

In general, only ABORT is called out as allowed for COFF output, which is more of a compatibility note. aBoRt works fine too.

I went through the AS target documentation and found the following targets where directives were not all lower case:

  • ARC
  • MMIX (all upper case, though GCC accepts any)
  • V850

Along the way I realized that in LLVM each target has it's own <target>AsmParser::ParseDirective. Some of which use .lower() some don't.

So I think I'll reduce the scope of this to allow any case for the generic directives and aliases of those. (I don't see any alias that isn't all lower case)

That means that target specific directives are still case sensitive unless I go through each parser. Which I can do, just not sure whether it's the right way to go.

DavidSpickett retitled this revision from [AsmParser] Make directives case insensitive. to [AsmParser] Make generic directives and aliases case insensitive..
DavidSpickett edited the summary of this revision. (Show Details)

Added a comment to init directive map saying that they should all be lower case.
Switched to a generic x86 target and alias for the lit test, to match the other MC/AsmParser tests.

DavidSpickett marked 2 inline comments as done.Jan 17 2020, 1:39 AM
This revision is now accepted and ready to land.Jan 17 2020, 2:23 AM
This revision was automatically updated to reflect the committed changes.