[CSSPGO][llvm-profgen] Continue disassembling after illegal instruction is seen.

[hoy]

Summary

Previously we errored out when disassembling illegal instructions and there would be no profile generated. In fact illegal instructions are not uncommon and we'd better skip them and print "unknown" instead of erroring out. This matches the behavior of llvm-objdump (see disassembleObject in llvm-objdump.cpp).

[wenlei]

I'm wondering if we need a warning when running into such cases. It's possible with data in code for assembly code, but for normal c++ compilation, this could be a red flag that something might be off?

The consequence of such cases for llvm-objdump is less important and also visible in the output, so we don't need warning there. But llvm-profgen could be different - clues like warning may help us detect subtle profile issues.

llvm/tools/llvm-profgen/ProfiledBinary.cpp

241

Does llvm-objdump also prints one <unknown> for each byte of illegal instruction, or one <unknown> for entire range of illegal instructions?

[hoy]

In D97776#2597686, @wenlei wrote:

I'm wondering if we need a warning when running into such cases. It's possible with data in code for assembly code, but for normal c++ compilation, this could be a red flag that something might be off?

The consequence of such cases for llvm-objdump is less important and also visible in the output, so we don't need warning there. But llvm-profgen could be different - clues like warning may help us detect subtle profile issues.

Sure, a warning makes sense.

llvm/tools/llvm-profgen/ProfiledBinary.cpp

241

llvm-objdump prints <unknown> for each bad instruction: https://github.com/llvm/llvm-project/blob/eecbb1c77655d38c06e47cf32e2dcc72da45c517/llvm/tools/llvm-objdump/llvm-objdump.cpp#L1158

objdump prints <bad> instead.

[wenlei]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

241

what's the length of a bad instruction? one byte? not a big deal, but thought nice to be consistent.

[hoy]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

241

The logic in llvm-objdump is, if the dissembler doesn't tell the length of the bad instruction, moves forward byte-by-byte until a good instruction is seen. This is typically with X86 or variant sized ISA. For Arm, I think it'll skip 2 bytes or 4 bytes which is the length of fixed sized instructions.

[hoy]

Warning when disassembling bad instructions.

[wlei]

Thanks for working on this! LGTM

llvm/tools/llvm-profgen/ProfiledBinary.cpp

228

Nit: how about also use Disassembled as the condition and merge this to the Line274, like:

if (Disassembled)
  Offset += Size;
else
  Offset += 1;

[hoy]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

228

Size can still be a valid value even the disassembling fails on some targets. For example, on arm64, instruction size is always four byte. When the current disassembling fails, it might skip 4 bytes to the next instruction.

[wlei]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

228

I see, thanks for the clarification.

[wenlei]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

270

It would be good to have one warning for a consecutive range of invalid instructions, instead of one warning for each byte. Otherwise it can be noisy.

nit: Bad disassembling -> Invalid instructions at <start> - <end>

[hoy]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

270

Good point, fixed.

[hoy]

Addressing Wenlei's feedbacks.

[wenlei]

lgtm.

llvm/tools/llvm-profgen/ProfiledBinary.cpp

223

nit: InvalidInstLength

[hoy]

Renaming InvalidInstRange to InvalidInstLength.

llvm/tools/llvm-profgen/ProfiledBinary.cpp

bool ProfiledBinary::dissassembleSymbol(std::size_t SI, ArrayRef<uint8_t> Bytes,
 
   std::string &&SymbolName = Symbols[SI].Name.str();
   if (ShowDisassemblyOnly)
     outs() << '<' << SymbolName << ">:\n";
 
   uint64_t Offset = StartOffset;
   while (Offset < EndOffset) {
     MCInst Inst;
     uint64_t Size;
     // Disassemble an instruction.
-    if (!DisAsm->getInstruction(Inst, Size, Bytes.slice(Offset - SectionOffset),
-                                Offset + PreferredBaseAddress, nulls()))
-      return false;
+    bool Disassembled =
+        DisAsm->getInstruction(Inst, Size, Bytes.slice(Offset - SectionOffset),
+                               Offset + PreferredBaseAddress, nulls());
+    if (Size == 0)
+      Size = 1;
 
     if (ShowDisassemblyOnly) {
       if (ShowPseudoProbe) {
         ProbeDecoder.printProbeForAddress(outs(),
                                           Offset + PreferredBaseAddress);
       }
       outs() << format("%8" PRIx64 ":", Offset);
       size_t Start = outs().tell();
+      if (Disassembled)
         IPrinter->printInst(&Inst, Offset + Size, "", *STI.get(), outs());
+      else
+        outs() << "\t<unknown>";
       if (ShowSourceLocations) {
         unsigned Cur = outs().tell() - Start;
         if (Cur < 40)
           outs().indent(40 - Cur);
-        InstructionPointer Inst(this, Offset);
-        outs() << getReversedLocWithContext(symbolize(Inst));
+        InstructionPointer IP(this, Offset);
+        outs() << getReversedLocWithContext(symbolize(IP));
       }
       outs() << "\n";
     }
 
+    if (Disassembled) {
       const MCInstrDesc &MCDesc = MII->get(Inst.getOpcode());
-
       // Populate a vector of the symbolized callsite at this location
-    // We don't need symbolized info for probe-based profile, just use an empty
-    // stack as an entry to indicate a valid binary offset
+      // We don't need symbolized info for probe-based profile, just use an
+      // empty stack as an entry to indicate a valid binary offset
       FrameLocationStack SymbolizedCallStack;
       if (!UsePseudoProbes) {
         InstructionPointer IP(this, Offset);
         SymbolizedCallStack = symbolize(IP, true);
       }
       Offset2LocStackMap[Offset] = SymbolizedCallStack;
       // Populate address maps.
       CodeAddrs.push_back(Offset);
       if (MCDesc.isCall())
         CallAddrs.insert(Offset);
       else if (MCDesc.isReturn())
         RetAddrs.insert(Offset);
+    } else {
+      WithColor::warning() << "Bad disassembling at "
+                           << format("%8\n" PRIx64, Offset);
+    }
 
     Offset += Size;
   }
 
   if (ShowDisassemblyOnly)
     outs() << "\n";
 
   FuncStartAddrMap[StartOffset] = Symbols[SI].Name.str();