[MC][ELF] Add section flags to diagnostic

[bcain]

Summary

Make the assembler diagnostic emit quoted section names and refer to
the expected flag bits by input character flag.

[MaskRay]

llvm/lib/MC/MCParser/ELFAsmParser.cpp

646

There are actually a lot more section flags, e.g. SHF_MERGE ('M') SHF_LINK_ORDER ('o') SHF_EXCLUDE ('e'). This patch will leave an empty string for an unknown flag.

650

A backquote (binutils uses it sometimes) is actually uncommon in LLVM diagnostics. Both unquoted and '-quoted section names are common.

[bcain]

llvm/lib/MC/MCParser/ELFAsmParser.cpp

646

Ok, I'll fix it by making it exhaustive.

650

IMO quoting it one way or another makes more sense than not. I can use single-quotes if that's more conventional.

[bcain]

Added a getSectionFlagString() function.
Changed section name delimiters in diagnostic to single-quotes

[bcain]

fix indentation in getSectionFlagString per git-clang-format

[bcain]

llvm/test/MC/ELF/section-flags-changed.s

6

When I ran this test, it failed. I'm not quite clear on why FileCheck doesn't match here, I'm sure I'm missing something simple.

I'll continue to investigate why this test fails.

[bcain]

Fixed the expressions in getSectionFlagString() - strings instead of chars, the nulls were the wrong choice.

Test passes now.

[bcain]

PIng.

[sidneym]

LGTM

[MaskRay]

llvm/lib/MC/MCParser/ELFAsmParser.cpp

290

It is incorrect to mix OS/processor specific flags here. Hard coding generic flags and their strings are also error-prone. I left this as a TODO because it is not very easy to reuse lib/MC/MCSectionELF.cpp#L53 code. I'll check more carefully how to reuse.

[bcain]

llvm/lib/MC/MCParser/ELFAsmParser.cpp

290

it is not very easy to reuse lib/MC/MCSectionELF.cpp#L53 code. I'll check more carefully how to reuse.

What If I just abstracted the portion of code in PrintSwitchToSection() relating to the flags to a method that takes unsigned Flags and returns the text as a string? That could be used here in ELFAsmParser but also in PrintSwitchToSection()?

llvm/lib/MC/MCParser/ELFAsmParser.cpp

static unsigned parseSectionFlags(StringRef flagsStr, bool *UseLastGroup) {
   unsigned flags = 0;
 
   // If a valid numerical value is set for the section flag, use it verbatim
   if (!flagsStr.getAsInteger(0, flags))
     return flags;
 
   for (char i : flagsStr) {
     switch (i) {
     case 'a':
       flags |= ELF::SHF_ALLOC;
       break;
     case 'e':
       flags |= ELF::SHF_EXCLUDE;
       break;
     case 'x':
       flags |= ELF::SHF_EXECINSTR;
       break;
EndStmt:
   }
 
   MCSectionELF *Section = getContext().getELFSection(
       SectionName, Type, Flags, Size, GroupName, UniqueID, LinkedToSym);
   getStreamer().SwitchSection(Section, Subsection);
   if (Section->getType() != Type)
     Error(loc, "changed section type for " + SectionName + ", expected: 0x" +
                    utohexstr(Section->getType()));
-  if (Section->getFlags() != Flags)
-    Error(loc, "changed section flags for " + SectionName + ", expected: 0x" +
-                   utohexstr(Section->getFlags()));
+  if (Section->getFlags() != Flags) {
+    const unsigned ExpectedFlags = Section->getFlags();
+    const std::string ExpectedFlagsText =
+        Twine(StringRef((ExpectedFlags & ELF::SHF_ALLOC) ? "a" : "") +
+              StringRef((ExpectedFlags & ELF::SHF_WRITE) ? "w" : "") +
+              StringRef((ExpectedFlags & ELF::SHF_EXECINSTR) ? "x" : ""))
+            .str();
+    Error(loc, "changed section flags for `" + SectionName + "', expected: \"" +
+                   ExpectedFlagsText + "\"");
+  }
   if (Section->getEntrySize() != Size)
     Error(loc, "changed section entsize for " + SectionName +
                    ", expected: " + Twine(Section->getEntrySize()));
 
   if (getContext().getGenDwarfForAssembly()) {
     bool InsertResult = getContext().addGenDwarfSection(Section);
     if (InsertResult) {
       if (getContext().getDwarfVersion() <= 2)

llvm/test/MC/ELF/section-flags-changed.s

 # RUN: not llvm-mc -triple=x86_64 %s -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
 
 foo:
 .section .foo,"ax",@progbits
 
-# CHECK: {{.*}}.s:[[# @LINE+1]]:1: error: changed section flags for .foo, expected: 0x6
+# CHECK: {{.*}}.s:[[# @LINE+1]]:1: error: changed section flags for `.foo', expected: "ax"
 .section .foo,"awx",@progbits
 
-# CHECK: {{.*}}.s:[[# @LINE+1]]:1: error: changed section flags for .foo, expected: 0x6
+# CHECK: {{.*}}.s:[[# @LINE+1]]:1: error: changed section flags for `.foo', expected: "ax"
 .pushsection .foo,"a",@progbits
 
 .pushsection .foo,"ax",@progbits