[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

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

[skan]

llvm/lib/MC/MCELFStreamer.cpp

540–541

This change is not related.

[MaskRay]

llvm/lib/MC/MCELFStreamer.cpp

514–519

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

I moved the variable to the top.

[skan]

llvm/lib/MC/MCELFStreamer.cpp

514–519

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 ↗

(On Diff #257601)

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 ↗

(On Diff #257601)

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 ↗

(On Diff #257716)

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 ↗

(On Diff #257716)

Ah, thanks for noting this.

[rnk]

lgtm with suggestion to improve the diagnostic at the old location

llvm/lib/MC/MCAssembler.cpp

686

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.

689–690

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

[MaskRay]

Address comments

llvm/lib/MC/MCAssembler.cpp

689–690

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

689–690

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

689–690

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

[MaskRay]

Rebase after MCSection::getName() is available

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!");
         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/MCELFStreamer.cpp

 static void CheckBundleSubtargets(const MCSubtargetInfo *OldSTI,
                                   const MCSubtargetInfo *NewSTI) {
   if (OldSTI && NewSTI && OldSTI != NewSTI)
     report_fatal_error("A Bundle can only have one Subtarget.");
 }
 
 void MCELFStreamer::EmitInstToData(const MCInst &Inst,
                                    const MCSubtargetInfo &STI) {
+  MCSection &Sec = *getCurrentSectionOnly();
+  if (Sec.isVirtualSection()) {
+    getContext().reportError(Inst.getLoc(),
+                             "SHT_NOBITS section cannot have instructions");
+    return;
+  }
   MCAssembler &Assembler = getAssembler();
   SmallVector<MCFixup, 4> Fixups;
   SmallString<256> Code;
   raw_svector_ostream VecOS(Code);
   Assembler.getEmitter().encodeInstruction(Inst, VecOS, Fixups, STI);
 
   for (unsigned i = 0, e = Fixups.size(); i != e; ++i)
     fixSymbolsInTLSFixups(Fixups[i].getValue());
void MCELFStreamer::EmitInstToData(const MCInst &Inst,
   //   instruction, emit a MCCompactEncodedInstFragment. Otherwise, emit a
   //   MCDataFragment.
   // - If we're in a bundle-locked group, append the instruction to the current
   //   data fragment because we want all the instructions in a group to get into
   //   the same fragment. Be careful not to do that for the first instruction in
   //   the group, though.
   MCDataFragment *DF;
 
   if (Assembler.isBundlingEnabled()) {
-    MCSection &Sec = *getCurrentSectionOnly();
     if (Assembler.getRelaxAll() && isBundleLocked()) {
       // If the -mc-relax-all flag is used and we are bundle-locked, we re-use
       // the current bundle group.
       DF = BundleGroups.back();
       CheckBundleSubtargets(DF->getSubtargetInfo(), &STI);
     }
     else if (Assembler.getRelaxAll() && !isBundleLocked())
       // When not in a bundle-locked group and the -mc-relax-all flag is used,

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
+# CHECK: {{.*}}.s:[[#@LINE+1]]:3: error: SHT_NOBITS section cannot have instructions
+  nop
+
+.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