[MC][COFF][ELF] Reject instructions in IMAGE_SCN_CNT_UNINITIALIZED_DATA/SHT_NOBITS sections

[MaskRay]

Summary

For .bss; nop, MC inappropriately calls abort() (via report_fatal_error()) with a message
cannot have fixups in virtual section!
It is a bug to crash for invalid user input. Fix it by erroring out early in EmitInstToData().

Similarly, emitIntValue() in a virtual section (SHT_NOBITS in ELF) can crash with the mssage
non-zero initializer found in section '.bss' (see D4199)
It'd be nice to report the location but so many directives can call emitIntValue()
and it is difficult to track every location.
Note, COFF does not crash because MCAssembler::writeSectionData() is not
called for an IMAGE_SCN_CNT_UNINITIALIZED_DATA section.

Note, GNU as' arm64 backend reports `Error: attempt to store non-zero value in section .bss'``
for a non-zero .inst but fails to do so for other instructions.
We simply reject all instructions, even if the encoding is all zeros.

The Mach-O counterpart is D48517 (see test/MC/MachO/zerofill-text.s)

[skan]

llvm/lib/MC/MCELFStreamer.cpp

514–519 ↗

(On Diff #257425)

Do you think MCObjectStreamer::emitInstruction is a better place to check Sec.isVirtualSection()?

[skan]

llvm/lib/MC/MCELFStreamer.cpp

540–541 ↗

(On Diff #257425)

This change is not related.

[MaskRay]

llvm/lib/MC/MCELFStreamer.cpp

514–519 ↗

(On Diff #257425)

For an error message specific to the binary format (e.g. ELF), EmitInstToData is probably the best choice.

For the COFF error message, the error message may mention IMAGE_SCN_CNT_UNINITIALIZED_DATA.

[MaskRay]

llvm/lib/MC/MCELFStreamer.cpp

540–541 ↗

(On Diff #257425)

I moved the variable to the top.

[skan]

llvm/lib/MC/MCELFStreamer.cpp

514–519 ↗

(On Diff #257425)

But EmitInstToData can not check if a relaxable instruction is emitted into a virtual section.

[MaskRay]

Move the error to MCObjectStreamer::emitInstruction to make MCRelaxableFragment work.

[skan]

llvm/include/llvm/MC/MCSection.h

189–190

I think "llvm::StringRef" is better than const char * here.

https://llvm.org/doxygen/classllvm_1_1StringRef.html#details

llvm/lib/MC/MCObjectStreamer.cpp

372–373

const MCSection &?

[MaskRay]

Address comments

[rnk]

Sounds good. Do you mind implementing this for COFF? I don't think "virtual section" is standard terminology. It seems like something someone invented here in MC. The COFF impl of isVirtualSection is straightforward:

bool MCSectionCOFF::isVirtualSection() const {
  return getCharacteristics() & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;
}

This suggests we can return "IMAGE_SCN_CNT_UNINITIALIZED_DATA" instead of "virtual".

The .section flag for this seems to be b for the test.

llvm/include/llvm/MC/MCSection.h

189

This appears to be the first non-pure virtual method, i.e. the key method. Please define it out of line to avoid duplicate debug info & vtables.

[MaskRay]

Check COFF errors and add test/MC/COFF/bss-text.s

[MaskRay]

llvm/include/llvm/MC/MCSection.h

189

Ah, thanks for noting this.

[rnk]

lgtm with suggestion to improve the diagnostic at the old location

llvm/lib/MC/MCAssembler.cpp

686–687

I wonder if there are other creative ways to trigger this assertion, something like:

.section ... # stuff to make it bss-y
.uleb .LtmpN - .LtmpM

Might be worth transitioning it to reportError.

690–691

This can also be unified to use the new method, right?

[MaskRay]

Address comments

llvm/lib/MC/MCAssembler.cpp

690–691

This would be really difficult... so difficult that I have to mention it in the description

It'd be nice to report the location but so many directives can call emitIntValue()
and it is difficult to track every location.

[rnk]

llvm/lib/MC/MCAssembler.cpp

690–691

I didn't mean to ask about the source location, I was wondering if we could avoid the special ELF dyn_cast. The message could be:

"non-zero initializer found in " + Sec->getVirtualSectionKind() + " section " + Sec->getSectionName()

[skan]

LGTM

llvm/lib/MC/MCAssembler.cpp

690–691

I am aslo wondering if we can avoid the special ELF dyn_cast?

[MaskRay]

Rebase after MCSection::getName() is available

llvm/include/llvm/MC/MCSection.h

public:
   /// Return true if a .align directive should use "optimized nops" to fill
   /// instead of 0s.
   virtual bool UseCodeAlign() const = 0;
 
   /// Check whether this section is "virtual", that is has no actual object
   /// file contents.
   virtual bool isVirtualSection() const = 0;
 
+  virtual StringRef getVirtualSectionKind() const;
+
   /// Add a pending label for the requested subsection. This label will be
   /// associated with a fragment in flushPendingLabels()
   void addPendingLabel(MCSymbol* label, unsigned Subsection = 0);
 
   /// Associate all pending labels in a subsection with a fragment.
   void flushPendingLabels(MCFragment *F, uint64_t FOffset = 0,
         unsigned Subsection = 0);
 
   /// Associate all pending labels with empty data fragments. One fragment
   /// will be created for each subsection as necessary.
   void flushPendingLabels();
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCSECTION_H

llvm/include/llvm/MC/MCSectionCOFF.h

public:
 
   void setSelection(int Selection) const;
 
   void PrintSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
                             raw_ostream &OS,
                             const MCExpr *Subsection) const override;
   bool UseCodeAlign() const override;
   bool isVirtualSection() const override;
+  StringRef getVirtualSectionKind() const override;
 
   unsigned getOrAssignWinCFISectionID(unsigned *NextID) const {
     if (WinCFISectionID == ~0U)
       WinCFISectionID = (*NextID)++;
     return WinCFISectionID;
   }
 
   static bool isImplicitlyDiscardable(StringRef Name) {

llvm/include/llvm/MC/MCSectionELF.h

public:
   void setFlags(unsigned F) { Flags = F; }
   const MCSymbolELF *getGroup() const { return Group; }
 
   void PrintSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
                             raw_ostream &OS,
                             const MCExpr *Subsection) const override;
   bool UseCodeAlign() const override;
   bool isVirtualSection() const override;
+  StringRef getVirtualSectionKind() const override;
 
   bool isUnique() const { return UniqueID != NonUniqueID; }
   unsigned getUniqueID() const { return UniqueID; }
 
   const MCSection *getLinkedToSection() const {
     return &LinkedToSym->getSection();
   }
   const MCSymbol *getLinkedToSymbol() const { return LinkedToSym; }

llvm/lib/MC/MCAssembler.cpp

for (const MCFragment &F : *Sec) {
       switch (F.getKind()) {
       default: llvm_unreachable("Invalid fragment in virtual section!");
       case MCFragment::FT_Data: {
         // Check that we aren't trying to write a non-zero contents (or fixups)
         // into a virtual section. This is to support clients which use standard
         // directives to fill the contents of virtual sections.
         const MCDataFragment &DF = cast<MCDataFragment>(F);
         if (DF.fixup_begin() != DF.fixup_end())
-          report_fatal_error("cannot have fixups in virtual section!");
+          getContext().reportError(SMLoc(),
+                                   "virtual section cannot have fixups");
         for (unsigned i = 0, e = DF.getContents().size(); i != e; ++i)
           if (DF.getContents()[i]) {
             if (auto *ELFSec = dyn_cast<const MCSectionELF>(Sec))
-              report_fatal_error("non-zero initializer found in section '" +
+              getContext().reportError(
+                  SMLoc(), "non-zero initializer found in section '" +
                                ELFSec->getSectionName() + "'");
             else
-              report_fatal_error("non-zero initializer found in virtual section");
+              getContext().reportError(
+                  SMLoc(), "non-zero initializer found in virtual section");
           }
         break;
       }
       case MCFragment::FT_Align:
         // Check that we aren't trying to write a non-zero value into a virtual
         // section.
         assert((cast<MCAlignFragment>(F).getValueSize() == 0 ||
                 cast<MCAlignFragment>(F).getValue() == 0) &&

llvm/lib/MC/MCObjectStreamer.cpp

 }
 
 bool MCObjectStreamer::mayHaveInstructions(MCSection &Sec) const {
   return Sec.hasInstructions();
 }
 
 void MCObjectStreamer::emitInstruction(const MCInst &Inst,
                                        const MCSubtargetInfo &STI) {
+  const MCSection &Sec = *getCurrentSectionOnly();
+  if (Sec.isVirtualSection()) {
+    getContext().reportError(Inst.getLoc(),
+                             Twine(Sec.getVirtualSectionKind()) +
+                                 " section cannot have instructions");
+    return;
+  }
   getAssembler().getBackend().emitInstructionBegin(*this, Inst);
   emitInstructionImpl(Inst, STI);
   getAssembler().getBackend().emitInstructionEnd(*this, Inst);
 }
 
 void MCObjectStreamer::emitInstructionImpl(const MCInst &Inst,
                                            const MCSubtargetInfo &STI) {
   MCStreamer::emitInstruction(Inst, STI);

llvm/lib/MC/MCSection.cpp

if (!ExactMatch && Subsection != 0) {
     SubsectionFragmentMap.insert(MI, std::make_pair(Subsection, F));
     getFragmentList().insert(IP, F);
     F->setParent(this);
   }
 
   return IP;
 }
 
-void MCSection::addPendingLabel(MCSymbol* label, unsigned Subsection) {
+StringRef MCSection::getVirtualSectionKind() const { return "virtual"; }
+
+void MCSection::addPendingLabel(MCSymbol *label, unsigned Subsection) {
   PendingLabels.push_back(PendingLabel(label, Subsection));
 }
 
 void MCSection::flushPendingLabels(MCFragment *F, uint64_t FOffset,
            unsigned Subsection) {
   if (PendingLabels.empty())
     return;
 

llvm/lib/MC/MCSectionCOFF.cpp

 
 bool MCSectionCOFF::UseCodeAlign() const {
   return getKind().isText();
 }
 
 bool MCSectionCOFF::isVirtualSection() const {
   return getCharacteristics() & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;
 }
+
+StringRef MCSectionCOFF::getVirtualSectionKind() const {
+  return "IMAGE_SCN_CNT_UNINITIALIZED_DATA";
+}

llvm/lib/MC/MCSectionELF.cpp

 
 bool MCSectionELF::UseCodeAlign() const {
   return getFlags() & ELF::SHF_EXECINSTR;
 }
 
 bool MCSectionELF::isVirtualSection() const {
   return getType() == ELF::SHT_NOBITS;
 }
+
+StringRef MCSectionELF::getVirtualSectionKind() const { return "SHT_NOBITS"; }

llvm/test/MC/COFF/bss-text.s

+# RUN: not llvm-mc -filetype=obj -triple=x86_64-pc-win32 %s -o /dev/null 2>&1 | FileCheck %s
+
+## -filetype=asm does not check the error.
+# RUN: llvm-mc -triple=x86_64-pc-win32 %s
+
+.section uninitialized,"b"
+# MCRelaxableFragment
+# CHECK: {{.*}}.s:[[#@LINE+1]]:3: error: IMAGE_SCN_CNT_UNINITIALIZED_DATA section cannot have instructions
+  jmp foo
+
+.bss
+# CHECK: {{.*}}.s:[[#@LINE+1]]:3: error: IMAGE_SCN_CNT_UNINITIALIZED_DATA section cannot have instructions
+  addb %al,(%rax)

llvm/test/MC/ELF/ARM/bss-non-zero-value.s

-// RUN: not --crash llvm-mc -filetype=obj -triple arm-linux-gnu %s -o %t 2>%t.out
-// RUN: FileCheck --input-file=%t.out %s
-// CHECK: non-zero initializer found in section '.bss'
-  .bss
-  .globl  a
-  .align  2
-a:
-  .long 1
-  .size a, 4

llvm/test/MC/ELF/nobits-non-zero-value.s

+# RUN: not llvm-mc -filetype=obj -triple=x86_64 %s -o /dev/null 2>&1 | FileCheck %s
+
+## -filetype=asm does not check the error.
+# RUN: llvm-mc -triple=x86_64 %s
+
+.section .tbss,"aw",@nobits
+# MCRelaxableFragment
+# CHECK: {{.*}}.s:[[#@LINE+1]]:3: error: SHT_NOBITS section cannot have instructions
+  jmp foo
+
+.bss
+# CHECK: {{.*}}.s:[[#@LINE+1]]:3: error: SHT_NOBITS section cannot have instructions
+  addb %al,(%rax)
+
+# CHECK: <unknown>:0: error: non-zero initializer found in section '.bss'
+  .long 1

llvm/test/MC/X86/reloc-bss.s

-# RUN: not --crash llvm-mc -filetype=obj -triple=x86_64-linux-gnu %s 2>&1 | FileCheck %s
-# CHECK: LLVM ERROR: cannot have fixups in virtual section!
-
-.section        .init_array,"awT",@nobits
-
-.hidden patatino
-.globl  patatino
-patatino:
-  movl __init_array_start, %eax