[Opaqueptrs][IR Serialization] Improve inlineasm [de]serialization

[lebedev.ri]

Summary

The bitcode reader expected that the pointers are typed,
so that it can extract the function type for the assembly
so bitc::CST_CODE_INLINEASM did not explicitly store said function type.

I'm not really sure how the upgrade path will look for existing bitcode,
but i think we can easily support opaque pointers going forward,
by simply storing the function type.

[nikic]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

2898

Shouldn't this be a dyn_cast<FunctionType>? We don't want to crash on invalid type.

[lebedev.ri]

Use dyn_cast_or_null<>()

[nikic]

LGTM

[lebedev.ri]

In D116341#3213595, @nikic wrote:

LGTM

Thank you for the review.

llvm/include/llvm/Bitcode/LLVMBitCodes.h

enum ConstantsCodes {
   CST_CODE_BLOCKADDRESS = 21,    // CST_CODE_BLOCKADDRESS [fnty, fnval, bb#]
   CST_CODE_DATA = 22,            // DATA:          [n x elements]
   CST_CODE_INLINEASM_OLD2 = 23,  // INLINEASM:     [sideeffect|alignstack|
                                  //                 asmdialect,asmstr,conststr]
   CST_CODE_CE_GEP_WITH_INRANGE_INDEX = 24, //      [opty, flags, n x operands]
   CST_CODE_CE_UNOP = 25,                   // CE_UNOP:      [opcode, opval]
   CST_CODE_POISON = 26,                    // POISON
   CST_CODE_DSO_LOCAL_EQUIVALENT = 27,      // DSO_LOCAL_EQUIVALENT [gvty, gv]
-  CST_CODE_INLINEASM = 28,    // INLINEASM:     [sideeffect|alignstack|
+  CST_CODE_INLINEASM_OLD3 = 28,    // INLINEASM:     [sideeffect|alignstack|
                                    //                 asmdialect|unwind,
                                    //                 asmstr,conststr]
   CST_CODE_NO_CFI_VALUE = 29, // NO_CFI [ fty, f ]
+  CST_CODE_INLINEASM = 30,    // INLINEASM:     [fnty,
+                              //                 sideeffect|alignstack|
+                              //                 asmdialect|unwind,
+                              //                 asmstr,conststr]
 };
 
 /// CastOpcodes - These are values used in the bitcode files to encode which
 /// cast a CST_CODE_CE_CAST or a XXX refers to.  The values of these enums
 /// have no fixed relation to the LLVM IR enum values.  Changing these will
 /// break compatibility with old files.
 enum CastOpcodes {
   CAST_TRUNC = 0,

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::CST_CODE_INLINEASM_OLD: {
       if (3+AsmStrSize+ConstStrSize > Record.size())
         return error("Invalid record");
 
       for (unsigned i = 0; i != AsmStrSize; ++i)
         AsmStr += (char)Record[2+i];
       for (unsigned i = 0; i != ConstStrSize; ++i)
         ConstrStr += (char)Record[3+AsmStrSize+i];
       UpgradeInlineAsmString(&AsmStr);
+      // FIXME: support upgrading in opaque pointers mode.
       V = InlineAsm::get(
           cast<FunctionType>(cast<PointerType>(CurTy)->getElementType()),
           AsmStr, ConstrStr, HasSideEffects, IsAlignStack);
       break;
     }
     // This version adds support for the asm dialect keywords (e.g.,
     // inteldialect).
     case bitc::CST_CODE_INLINEASM_OLD2: {
case bitc::CST_CODE_INLINEASM_OLD2: {
       if (3+AsmStrSize+ConstStrSize > Record.size())
         return error("Invalid record");
 
       for (unsigned i = 0; i != AsmStrSize; ++i)
         AsmStr += (char)Record[2+i];
       for (unsigned i = 0; i != ConstStrSize; ++i)
         ConstrStr += (char)Record[3+AsmStrSize+i];
       UpgradeInlineAsmString(&AsmStr);
+      // FIXME: support upgrading in opaque pointers mode.
       V = InlineAsm::get(
           cast<FunctionType>(cast<PointerType>(CurTy)->getElementType()),
           AsmStr, ConstrStr, HasSideEffects, IsAlignStack,
           InlineAsm::AsmDialect(AsmDialect));
       break;
     }
     // This version adds support for the unwind keyword.
-    case bitc::CST_CODE_INLINEASM: {
+    case bitc::CST_CODE_INLINEASM_OLD3: {
       if (Record.size() < 2)
         return error("Invalid record");
       unsigned OpNum = 0;
       std::string AsmStr, ConstrStr;
       bool HasSideEffects = Record[OpNum] & 1;
       bool IsAlignStack = (Record[OpNum] >> 1) & 1;
       unsigned AsmDialect = (Record[OpNum] >> 2) & 1;
       bool CanThrow = (Record[OpNum] >> 3) & 1;
       ++OpNum;
       unsigned AsmStrSize = Record[OpNum];
       ++OpNum;
       if (OpNum + AsmStrSize >= Record.size())
         return error("Invalid record");
       unsigned ConstStrSize = Record[OpNum + AsmStrSize];
       if (OpNum + 1 + AsmStrSize + ConstStrSize > Record.size())
         return error("Invalid record");
 
       for (unsigned i = 0; i != AsmStrSize; ++i)
         AsmStr += (char)Record[OpNum + i];
       ++OpNum;
       for (unsigned i = 0; i != ConstStrSize; ++i)
         ConstrStr += (char)Record[OpNum + AsmStrSize + i];
       UpgradeInlineAsmString(&AsmStr);
+      // FIXME: support upgrading in opaque pointers mode.
       V = InlineAsm::get(
           cast<FunctionType>(cast<PointerType>(CurTy)->getElementType()),
           AsmStr, ConstrStr, HasSideEffects, IsAlignStack,
           InlineAsm::AsmDialect(AsmDialect), CanThrow);
       break;
     }
+    // This version adds explicit function type.
+    case bitc::CST_CODE_INLINEASM: {
+      if (Record.size() < 3)
+        return error("Invalid record");
+      unsigned OpNum = 0;
+      auto *FnTy = dyn_cast_or_null<FunctionType>(getTypeByID(Record[OpNum]));
+      ++OpNum;
+      if (!FnTy)
+        return error("Invalid record");
+      std::string AsmStr, ConstrStr;
+      bool HasSideEffects = Record[OpNum] & 1;
+      bool IsAlignStack = (Record[OpNum] >> 1) & 1;
+      unsigned AsmDialect = (Record[OpNum] >> 2) & 1;
+      bool CanThrow = (Record[OpNum] >> 3) & 1;
+      ++OpNum;
+      unsigned AsmStrSize = Record[OpNum];
+      ++OpNum;
+      if (OpNum + AsmStrSize >= Record.size())
+        return error("Invalid record");
+      unsigned ConstStrSize = Record[OpNum + AsmStrSize];
+      if (OpNum + 1 + AsmStrSize + ConstStrSize > Record.size())
+        return error("Invalid record");
+
+      for (unsigned i = 0; i != AsmStrSize; ++i)
+        AsmStr += (char)Record[OpNum + i];
+      ++OpNum;
+      for (unsigned i = 0; i != ConstStrSize; ++i)
+        ConstrStr += (char)Record[OpNum + AsmStrSize + i];
+      UpgradeInlineAsmString(&AsmStr);
+      V = InlineAsm::get(FnTy, AsmStr, ConstrStr, HasSideEffects, IsAlignStack,
+                         InlineAsm::AsmDialect(AsmDialect), CanThrow);
+      break;
+    }
     case bitc::CST_CODE_BLOCKADDRESS:{
       if (Record.size() < 3)
         return error("Invalid record");
       Type *FnTy = getTypeByID(Record[0]);
       if (!FnTy)
         return error("Invalid record");
       Function *Fn =
         dyn_cast_or_null<Function>(ValueList.getConstantFwdRef(Record[1],FnTy));
case bitc::FUNC_CODE_INST_CALLBR: {
       PointerType *OpTy = dyn_cast<PointerType>(Callee->getType());
       if (!OpTy)
         return error("Callee is not a pointer type");
       if (!FTy) {
         FTy = dyn_cast<FunctionType>(
             cast<PointerType>(Callee->getType())->getElementType());
         if (!FTy)
           return error("Callee is not of pointer to function type");
-      } else if (cast<PointerType>(Callee->getType())->getElementType() != FTy)
+      } else if (!OpTy->isOpaqueOrPointeeTypeMatches(FTy))
         return error("Explicit call type does not match pointee type of "
                      "callee operand");
       if (Record.size() < FTy->getNumParams() + OpNum)
         return error("Insufficient operands to call");
 
       SmallVector<Value*, 16> Args;
       // Read the fixed params.
       for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i, ++OpNum) {

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

if (V->getType() != LastTy) {
       LastTy = V->getType();
       Record.push_back(VE.getTypeID(LastTy));
       Stream.EmitRecord(bitc::CST_CODE_SETTYPE, Record,
                         CONSTANTS_SETTYPE_ABBREV);
       Record.clear();
     }
 
     if (const InlineAsm *IA = dyn_cast<InlineAsm>(V)) {
+      Record.push_back(VE.getTypeID(IA->getFunctionType()));
       Record.push_back(
           unsigned(IA->hasSideEffects()) | unsigned(IA->isAlignStack()) << 1 |
           unsigned(IA->getDialect() & 1) << 2 | unsigned(IA->canThrow()) << 3);
 
       // Add the asm string.
       const std::string &AsmStr = IA->getAsmString();
       Record.push_back(AsmStr.size());
       Record.append(AsmStr.begin(), AsmStr.end());

llvm/test/Bitcode/callbr.ll

-; RUN:  llvm-dis < %s.bc | FileCheck %s
-
+; RUN:  llvm-dis < %s.bc | FileCheck %s --check-prefixes=CHECK,CHECK-TYPED
 ; callbr.ll.bc was generated by passing this file to llvm-as.
 
+; RUN: llvm-as < %s | llvm-dis | FileCheck %s --check-prefixes=CHECK,CHECK-TYPED
+; RUN: llvm-as -opaque-pointers < %s | llvm-dis -opaque-pointers | FileCheck %s --check-prefixes=CHECK,CHECK-OPAQUE
+
 define i32 @test_asm_goto(i32 %x){
 entry:
-; CHECK:      callbr void asm "", "r,X"(i32 %x, i8* blockaddress(@test_asm_goto, %fail))
+; CHECK-TYPED:      callbr void asm "", "r,X"(i32 %x, i8* blockaddress(@test_asm_goto, %fail))
+; CHECK-OPAQUE:     callbr void asm "", "r,X"(i32 %x, ptr blockaddress(@test_asm_goto, %fail))
 ; CHECK-NEXT: to label %normal [label %fail]
   callbr void asm "", "r,X"(i32 %x, i8* blockaddress(@test_asm_goto, %fail)) to label %normal [label %fail]
 normal:
   ret i32 1
 fail:
   ret i32 0
 }