[ELF] Allow uppercase section directives

[sidneym]

Summary

It was noticed that llvm-mc doesn't allow uppercase section directives. I don't see any obvious reason why they shouldn't be allowed.

Since this has never been the case for llvm I'm not sure how important this is but gnu-as isn't case sensative AFAIK.

[grimar]

Seems I do not understand the current behavior.

GNU as shows the following errors for this test:

test.s:6: Error: unknown pseudo-op: `.data.rel.ro'
test.s:9: Error: unknown pseudo-op: `.data.rel'
test.s:12: Error: unknown pseudo-op: `.eh_frame'
test.s:15: Error: unknown pseudo-op: `.rodata'
test.s:18: Error: unknown pseudo-op: `.tbss'
test.s:21: Error: unknown pseudo-op: `.tdata'
test.s:27: Error: unknown pseudo-op: `.data.rel.ro'
test.s:30: Error: unknown pseudo-op: `.data.rel'
test.s:33: Error: unknown pseudo-op: `.eh_frame'
test.s:36: Error: unknown pseudo-op: `.rodata'
test.s:39: Error: unknown pseudo-op: `.tbss'
test.s:42: Error: unknown pseudo-op: `.tdata'

Looking at the documentation of directives allowed (https://sourceware.org/binutils/docs/as/Pseudo-Ops.html#Pseudo-Ops)
It says: "The names are case insensitive for most targets, and usually written in lower case."
But it mentions only Data and Text directives. Though we do not check them in this test (but we support them).

So seems we are already different here. But does it make sence to support upper-case? I do not know.
Perhaps it is OK for the compatibility (with the spec), but seems we have no users, so probably we shouldn't?
I'd like to hear what other think.

[sidneym]

In D76328#1928559, @grimar wrote:

Seems I do not understand the current behavior.

GNU as shows the following errors for this test:

test.s:6: Error: unknown pseudo-op: `.data.rel.ro'
test.s:9: Error: unknown pseudo-op: `.data.rel'
test.s:12: Error: unknown pseudo-op: `.eh_frame'
test.s:15: Error: unknown pseudo-op: `.rodata'
test.s:18: Error: unknown pseudo-op: `.tbss'
test.s:21: Error: unknown pseudo-op: `.tdata'
test.s:27: Error: unknown pseudo-op: `.data.rel.ro'
test.s:30: Error: unknown pseudo-op: `.data.rel'
test.s:33: Error: unknown pseudo-op: `.eh_frame'
test.s:36: Error: unknown pseudo-op: `.rodata'
test.s:39: Error: unknown pseudo-op: `.tbss'
test.s:42: Error: unknown pseudo-op: `.tdata'

I think GNU might want a leading .section.

Looking at the documentation of directives allowed (https://sourceware.org/binutils/docs/as/Pseudo-Ops.html#Pseudo-Ops)
It says: "The names are case insensitive for most targets, and usually written in lower case."
But it mentions only `Data` and `Text` directives. Though we do not check them in this test (but we support them).

So seems we are already different here. But does it make sence to support upper-case? I do not know.
Perhaps it is OK for the compatibility (with the spec), but seems we have no users, so probably we shouldn't?
I'd like to hear what other think.

That is fair, I ran accross some code that was all uppercase and the .TEXT section was actually the error that surfaced. I will update the testcase to include all of the ops if we want to add this patch.

[MaskRay]

I am mostly with the following opinions.

http://lists.llvm.org/pipermail/llvm-dev/2020-February/138955.html @JonChesterfield: This doesn't sound right. GNU binutils have a large quantity of legacy cruft, not least the redundancy between tools like readelf and objdump which are capable of doing the same task in exchange for different command line arguments.

@joerg: So given that we got around with this for years, how much use do non-lower case assembler pseudops actually see?

Can't the projects using uppercase directives be fixed?

I don't see any obvious reason why they shouldn't be allowed.

Most people agree this is legacy cruft. There is few if any project which must use this syntax. I think we need a very strong reason to support uppercase directives, rather than the converse.

(https://bugs.llvm.org/show_bug.cgi?id=39527 was really a bad example. The author committed one change, despite the request both on the differential and on the bugzilla, they continued making another change with no explanation (bugzilla,differential,or git commit description).)

[MaskRay]

In D76328#1928559, @grimar wrote:

Seems I do not understand the current behavior.

GNU as shows the following errors for this test:

test.s:6: Error: unknown pseudo-op: `.data.rel.ro'
test.s:9: Error: unknown pseudo-op: `.data.rel'
test.s:12: Error: unknown pseudo-op: `.eh_frame'
test.s:15: Error: unknown pseudo-op: `.rodata'
test.s:18: Error: unknown pseudo-op: `.tbss'
test.s:21: Error: unknown pseudo-op: `.tdata'
test.s:27: Error: unknown pseudo-op: `.data.rel.ro'
test.s:30: Error: unknown pseudo-op: `.data.rel'
test.s:33: Error: unknown pseudo-op: `.eh_frame'
test.s:36: Error: unknown pseudo-op: `.rodata'
test.s:39: Error: unknown pseudo-op: `.tbss'
test.s:42: Error: unknown pseudo-op: `.tdata'

Looking at the documentation of directives allowed (https://sourceware.org/binutils/docs/as/Pseudo-Ops.html#Pseudo-Ops)
It says: "The names are case insensitive for most targets, and usually written in lower case."
But it mentions only Data and Text directives. Though we do not check them in this test (but we support them).

So seems we are already different here. But does it make sence to support upper-case? I do not know.
Perhaps it is OK for the compatibility (with the spec), but seems we have no users, so probably we shouldn't?
I'd like to hear what other think.

The extra recognized special sections can be considered extensions. If they don't seem to cause error-prone behaviors (other than portability issues), I think we probably don't need to write more code just to reject them.

[sidneym]

OK, I will drop it. Thanks

llvm/lib/MC/MCParser/AsmParser.cpp

if (TPDirectiveReturn && StartTokLoc != getTok().getLoc())
       return true;
     // Return if we did some parsing or believe we succeeded.
     if (!TPDirectiveReturn || StartTokLoc != getTok().getLoc())
       return false;
 
     // Next, check the extension directive map to see if any extension has
     // registered itself to parse this directive.
     std::pair<MCAsmParserExtension *, DirectiveHandler> Handler =
-        ExtensionDirectiveMap.lookup(IDVal);
+        ExtensionDirectiveMap.lookup(IDVal.lower());
     if (Handler.first)
       return (*Handler.second)(Handler.first, IDVal, IDLoc);
 
     // Finally, if no one else is interested in this directive, it must be
     // generic and familiar to this class.
     switch (DirKind) {
     default:
       break;

llvm/test/MC/ELF/elf_directive_section.s

 # CHECK: .rodata
 
   .tbss
 # CHECK: .tbss
 
   .tdata
 # CHECK: .tdata
 
+  .BSS
+# CHECK: .bss
+
+  .DATA.REL.RO
+# CHECK: .data.rel.ro
+
+  .DATA.REL
+# CHECK: .data.rel
+
+  .EH_FRAME
+# CHECK: .eh_frame
+
+  .RODATA
+# CHECK: .rodata
+
+  .TBSS
+# CHECK: .tbss
+
+  .TDATA
+# CHECK: .tdata
+