[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

663

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.

667

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

663

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

667

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

if (PrevLoc.getPointer() + CurSize != getTok().getLoc().getPointer())
       break;
   }
   if (Size == 0)
     return true;
 
   return false;
 }
 
+static std::string getSectionFlagString(unsigned flags) {
+  return (Twine(((flags & ELF::SHF_ALLOC) != 0) ? "a" : "") +
+          Twine(((flags & ELF::SHF_EXCLUDE) != 0) ? "e" : "") +
+          Twine(((flags & ELF::SHF_EXECINSTR) != 0) ? "x" : "") +
+          Twine(((flags & ELF::SHF_WRITE) != 0) ? "w" : "") +
+          Twine(((flags & ELF::SHF_LINK_ORDER) != 0) ? "o" : "") +
+          Twine(((flags & ELF::SHF_MERGE) != 0) ? "M" : "") +
+          Twine(((flags & ELF::SHF_STRINGS) != 0) ? "S" : "") +
+          Twine(((flags & ELF::SHF_TLS) != 0) ? "T" : "") +
+          Twine(((flags & ELF::XCORE_SHF_CP_SECTION) != 0) ? "c" : "") +
+          Twine(((flags & ELF::XCORE_SHF_DP_SECTION) != 0) ? "d" : "") +
+          Twine(((flags & ELF::SHF_ARM_PURECODE) != 0) ? "y" : "") +
+          Twine(((flags & ELF::SHF_HEX_GPREL) != 0) ? "s" : "") +
+          Twine(((flags & ELF::SHF_GROUP) != 0) ? "G" : ""))
+      .str();
+}
+
 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) {
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 = getSectionFlagString(ExpectedFlags);
+    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)
         Warning(loc, "DWARF2 only supports one section per compilation unit");
 

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