[MC] Handle discardable COFF sections in assembly

[rnk]

Summary

This fixes a dumpbin warning on objects produced by the MC assembler
when starting from text. All .debug$S sections are supposed to be marked
IMAGE_SCN_MEM_DISCARDABLE. The main, non-COMDAT .debug$S section had
this set, but any comdat ones were not being marked discardable because
there was no .section flag for it.

This change does two things:

• If the section name starts with .debug, implicitly mark the section as discardable. This means we do the same thing as gas on .s files with DWARF from GCC, which is important.

• Adds the 'D' flag to the .section directive on COFF to explicitly say a section is discardable. We only emit this flag if the section name does not start with .debug. This allows the user to explicitly tweak their section flags without worrying about magic section names.

The only thing you can't do in this scheme is to create a
non-discardable section with a name starting with ".debug", but
hopefully users don't need that.

[rnk]

• Update CV tests

[majnemer]

I wonder if we should just adopt the section name hack for improved compatibility with mingw. We'd give any section which starts with .debug IMAGE_SCN_MEM_DISCARDABLE

[rnk]

In D24582#543071, @majnemer wrote:

I wonder if we should just adopt the section name hack for improved compatibility with mingw. We'd give any section which starts with .debug IMAGE_SCN_MEM_DISCARDABLE

Maybe we should do both? I don't want to leave MCSectionCOFF with an incomplete .section serialization.

[majnemer]

In D24582#543082, @rnk wrote:

In D24582#543071, @majnemer wrote:

I wonder if we should just adopt the section name hack for improved compatibility with mingw. We'd give any section which starts with .debug IMAGE_SCN_MEM_DISCARDABLE

Maybe we should do both? I don't want to leave MCSectionCOFF with an incomplete .section serialization.

It won't work when we try to take our .s output to mingw though. Seeing as how this won't work in the general case, I'd suggest the following:

• Infer IMAGE_SCN_MEM_DISCARDABLE if the section starts with .debug
• Raise an error if we have a section with IMAGE_SCN_MEM_DISCARDABLE which does not start with .debug

[rnk]

In D24582#543125, @majnemer wrote:

In D24582#543082, @rnk wrote:

In D24582#543071, @majnemer wrote:

I wonder if we should just adopt the section name hack for improved compatibility with mingw. We'd give any section which starts with .debug IMAGE_SCN_MEM_DISCARDABLE

Maybe we should do both? I don't want to leave MCSectionCOFF with an incomplete .section serialization.

It won't work when we try to take our .s output to mingw though. Seeing as how this won't work in the general case, I'd suggest the following:

• Infer IMAGE_SCN_MEM_DISCARDABLE if the section starts with .debug
• Raise an error if we have a section with IMAGE_SCN_MEM_DISCARDABLE which does not start with .debug

How about I do all that, and instead of raising an error if there is a discardable section that doesn't start with .debug, I emit the 'D' flag?

[majnemer]

In D24582#543165, @rnk wrote:

In D24582#543125, @majnemer wrote:

In D24582#543082, @rnk wrote:

In D24582#543071, @majnemer wrote:

I wonder if we should just adopt the section name hack for improved compatibility with mingw. We'd give any section which starts with .debug IMAGE_SCN_MEM_DISCARDABLE

Maybe we should do both? I don't want to leave MCSectionCOFF with an incomplete .section serialization.

It won't work when we try to take our .s output to mingw though. Seeing as how this won't work in the general case, I'd suggest the following:

• Infer IMAGE_SCN_MEM_DISCARDABLE if the section starts with .debug
• Raise an error if we have a section with IMAGE_SCN_MEM_DISCARDABLE which does not start with .debug

How about I do all that, and instead of raising an error if there is a discardable section that doesn't start with .debug, I emit the 'D' flag?

SGTM

[rnk]

• Implicitly make .debug sections discardable.

[majnemer]

LGTM

[majnemer]

lib/MC/MCSectionCOFF.cpp

67

Maybe parens because of the bitwise operator?

[rnk]

Thanks!

lib/MC/MCSectionCOFF.cpp

67

Yeah, man, I can never remember the bitwise operator precedence. Parens++

include/llvm/MC/MCSectionCOFF.h

public:
   bool isVirtualSection() const override;
 
   unsigned getOrAssignWinCFISectionID(unsigned *NextID) const {
     if (WinCFISectionID == ~0U)
       WinCFISectionID = (*NextID)++;
     return WinCFISectionID;
   }
 
+  static bool isImplicitlyDiscardable(StringRef Name) {
+    return Name.startswith(".debug");
+  }
+
   static bool classof(const MCSection *S) { return S->getVariant() == SV_COFF; }
 };
 
 } // end namespace llvm
 
 #endif

lib/MC/MCParser/COFFAsmParser.cpp

bool ParseSectionSwitch(StringRef Section,
                           unsigned Characteristics,
                           SectionKind Kind);
 
   bool ParseSectionSwitch(StringRef Section, unsigned Characteristics,
                           SectionKind Kind, StringRef COMDATSymName,
                           COFF::COMDATType Type);
 
   bool ParseSectionName(StringRef &SectionName);
-  bool ParseSectionFlags(StringRef FlagsString, unsigned* Flags);
+  bool ParseSectionFlags(StringRef SectionName, StringRef FlagsString,
+                         unsigned *Flags);
 
   void Initialize(MCAsmParser &Parser) override {
     // Call the base implementation.
     MCAsmParserExtension::Initialize(Parser);
 
     addDirectiveHandler<&COFFAsmParser::ParseSectionDirectiveText>(".text");
     addDirectiveHandler<&COFFAsmParser::ParseSectionDirectiveData>(".data");
     addDirectiveHandler<&COFFAsmParser::ParseSectionDirectiveBSS>(".bss");
static SectionKind computeSectionKind(unsigned Flags) {
   if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
     return SectionKind::getText();
   if (Flags & COFF::IMAGE_SCN_MEM_READ &&
       (Flags & COFF::IMAGE_SCN_MEM_WRITE) == 0)
     return SectionKind::getReadOnly();
   return SectionKind::getData();
 }
 
-bool COFFAsmParser::ParseSectionFlags(StringRef FlagsString, unsigned* Flags) {
+bool COFFAsmParser::ParseSectionFlags(StringRef SectionName,
+                                      StringRef FlagsString, unsigned *Flags) {
   enum {
-    None      = 0,
-    Alloc     = 1 << 0,
-    Code      = 1 << 1,
-    Load      = 1 << 2,
-    InitData  = 1 << 3,
-    Shared    = 1 << 4,
-    NoLoad    = 1 << 5,
-    NoRead    = 1 << 6,
-    NoWrite  =  1 << 7
+    None        = 0,
+    Alloc       = 1 << 0,
+    Code        = 1 << 1,
+    Load        = 1 << 2,
+    InitData    = 1 << 3,
+    Shared      = 1 << 4,
+    NoLoad      = 1 << 5,
+    NoRead      = 1 << 6,
+    NoWrite     = 1 << 7,
+    Discardable = 1 << 8,
   };
 
   bool ReadOnlyRemoved = false;
   unsigned SecFlags = None;
 
   for (char FlagChar : FlagsString) {
     switch (FlagChar) {
     case 'a':
case 'd': // data section
         SecFlags |= Load;
       break;
 
     case 'n': // section is not loaded
       SecFlags |= NoLoad;
       SecFlags &= ~Load;
       break;
 
+    case 'D': // discardable
+      SecFlags |= Discardable;
+      break;
+
     case 'r': // read-only
       ReadOnlyRemoved = false;
       SecFlags |= NoWrite;
       if ((SecFlags & Code) == 0)
         SecFlags |= InitData;
       if ((SecFlags & NoLoad) == 0)
         SecFlags |= Load;
       break;
bool COFFAsmParser::ParseSectionFlags(StringRef SectionName,
   if (SecFlags & Code)
     *Flags |= COFF::IMAGE_SCN_CNT_CODE | COFF::IMAGE_SCN_MEM_EXECUTE;
   if (SecFlags & InitData)
     *Flags |= COFF::IMAGE_SCN_CNT_INITIALIZED_DATA;
   if ((SecFlags & Alloc) && (SecFlags & Load) == 0)
     *Flags |= COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;
   if (SecFlags & NoLoad)
     *Flags |= COFF::IMAGE_SCN_LNK_REMOVE;
+  if ((SecFlags & Discardable) ||
+      MCSectionCOFF::isImplicitlyDiscardable(SectionName))
+    *Flags |= COFF::IMAGE_SCN_MEM_DISCARDABLE;
   if ((SecFlags & NoRead) == 0)
     *Flags |= COFF::IMAGE_SCN_MEM_READ;
   if ((SecFlags & NoWrite) == 0)
     *Flags |= COFF::IMAGE_SCN_MEM_WRITE;
   if (SecFlags & Shared)
     *Flags |= COFF::IMAGE_SCN_MEM_SHARED;
 
   return false;

 }
 
 // .section name [, "flags"] [, identifier [ identifier ], identifier]
 //
 // Supported flags:
 //   a: Ignored.
 //   b: BSS section (uninitialized data)
 //   d: data section (initialized data)
-//   n: Discardable section
+//   n: "noload" section (removed by linker)
+//   D: Discardable section
 //   r: Readable section
 //   s: Shared section
 //   w: Writable section
 //   x: Executable section
 //   y: Not-readable section (clears 'r')
 //
 // Subsections are not supported.
 bool COFFAsmParser::ParseDirectiveSection(StringRef, SMLoc) {
if (getLexer().is(AsmToken::Comma)) {
     Lex();
 
     if (getLexer().isNot(AsmToken::String))
       return TokError("expected string in directive");
 
     StringRef FlagsStr = getTok().getStringContents();
     Lex();
 
-    if (ParseSectionFlags(FlagsStr, &Flags))
+    if (ParseSectionFlags(SectionName, FlagsStr, &Flags))
       return true;
   }
 
   COFF::COMDATType Type = (COFF::COMDATType)0;
   StringRef COMDATSymName;
   if (getLexer().is(AsmToken::Comma)) {
     Type = COFF::IMAGE_COMDAT_SELECT_ANY;
     Lex();

lib/MC/MCSectionCOFF.cpp

void MCSectionCOFF::PrintSwitchToSection(const MCAsmInfo &MAI,
   else if (getCharacteristics() & COFF::IMAGE_SCN_MEM_READ)
     OS << 'r';
   else
     OS << 'y';
   if (getCharacteristics() & COFF::IMAGE_SCN_LNK_REMOVE)
     OS << 'n';
   if (getCharacteristics() & COFF::IMAGE_SCN_MEM_SHARED)
     OS << 's';
+  if (getCharacteristics() & COFF::IMAGE_SCN_MEM_DISCARDABLE &&
+      !isImplicitlyDiscardable(SectionName))
+    OS << 'D';
   OS << '"';
 
   if (getCharacteristics() & COFF::IMAGE_SCN_LNK_COMDAT) {
     OS << ",";
     switch (Selection) {
       case COFF::IMAGE_COMDAT_SELECT_NODUPLICATES:
         OS << "one_only,";
         break;

test/MC/COFF/section.s

 // CHECK:   }
 
 // Test that the defaults are used
 .section s      ; .long 1
 .section s_, "" ; .long 1
 .section s_a,"a"; .long 1
 .section s_b,"b"; .long 1
 .section s_d,"d"; .long 1
+.section s_D,"D"; .long 1
 .section s_n,"n"; .long 1
 .section s_r,"r"; .long 1
 .section s_s,"s"; .long 1
 .section s_w,"w"; .long 1
 .section s_x,"x"; .long 1
 .section s_y,"y"; .long 1
 
 // CHECK:        Section {

 // CHECK:          Characteristics [
 // CHECK-NEXT:       IMAGE_SCN_ALIGN_1BYTES
 // CHECK-NEXT:       IMAGE_SCN_CNT_INITIALIZED_DATA
 // CHECK-NEXT:       IMAGE_SCN_MEM_READ
 // CHECK-NEXT:       IMAGE_SCN_MEM_WRITE
 // CHECK-NEXT:     ]
 // CHECK:        }
 // CHECK:        Section {
+// CHECK:          Name: s_D
+// CHECK:          Characteristics [
+// CHECK-NEXT:       IMAGE_SCN_ALIGN_1BYTES
+// CHECK-NEXT:       IMAGE_SCN_MEM_DISCARDABLE
+// CHECK-NEXT:       IMAGE_SCN_MEM_READ
+// CHECK-NEXT:       IMAGE_SCN_MEM_WRITE
+// CHECK-NEXT:     ]
+// CHECK:        }
+// CHECK:        Section {
 // CHECK:          Name: s_n
 // CHECK:          Characteristics [
 // CHECK-NEXT:       IMAGE_SCN_ALIGN_1BYTES
 // CHECK-NEXT:       IMAGE_SCN_LNK_REMOVE
 // CHECK-NEXT:       IMAGE_SCN_MEM_READ
 // CHECK-NEXT:       IMAGE_SCN_MEM_WRITE
 // CHECK-NEXT:     ]
 // CHECK:        }

 // CHECK:        Section {
 // CHECK:          Name: s_rwy
 // CHECK:          Characteristics [
 // CHECK-NEXT:       IMAGE_SCN_ALIGN_1BYTES
 // CHECK-NEXT:       IMAGE_SCN_CNT_INITIALIZED_DATA
 // CHECK-NEXT:     ]
 // CHECK:        }
 
+// Sections starting with ".debug" are implicitly discardable. This is
+// compatible with gas.
+.section .debug_asdf,"dr"; .long 1
+// CHECK:        Section {
+// CHECK:          Name: .debug_asdf
+// CHECK:          Characteristics [
+// CHECK-NEXT:       IMAGE_SCN_ALIGN_1BYTES
+// CHECK-NEXT:       IMAGE_SCN_CNT_INITIALIZED_DATA
+// CHECK-NEXT:       IMAGE_SCN_MEM_DISCARDABLE
+// CHECK-NEXT:       IMAGE_SCN_MEM_READ
+// CHECK-NEXT:     ]
+// CHECK:        }
+
+
 // CHECK:      ]