[Bitcode] Add partial support for opaque pointer auto-upgrade

[nikic]

Summary

Auto-upgrades that rely on the pointer element type do not work in opaque pointer mode. The idea behind this patch is that we can instead work with type IDs, for which we can retain the pointer element type. For typed pointer bitcode, we will have a distinct type ID for pointers with distinct element type, even if there will only be a single corresponding opaque pointer type.

The disclaimer here is that this is only the first step of the change, and there are still more getPointerElementType() calls to remove. I expect that two more patches will be needed: 1. Track all "contained" type IDs, which will allow us to handle function params (which are contained in the function type) and GEPs (which may use vectors of pointers) 2. Track type IDs for values, which is e.g. necessary to handle loads.

[dblaikie]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1185–1190

Would it be reasonable/possible to always use the ElementTypeList even when typed pointers are supported? (frontload changes somewhat) - maybe with typed pointer support this could be an assert that the type in ElementTypeList matches the non-opaque pointer type?

(oh, I guess since this support is currently incomplete that's not possible, but would be possible when the support is complete but before opaque pointers are enabled by default? I guess it could still be an assert today in the case where ElementTypeList is available, maybe?)

5036–5037

I wonder if these sort of checks could be asserts? I realize they are dynamically reachable, but they're also not intended to be reached by end users (only by LLVM developers during this migration)... I guess it's weird either way. Either we have asserts that are dynamically reachable, or we have error paths that are untested... I don't feel great about either of those, but either seem like acceptable tradeoffs during the transition.

[nikic]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1185–1190

Yes, we could always use ElementTypeList, would just have to populate it unconditionally. Currently I'm avoiding it if we're not doing a typed -> opaque transition, but maybe that's unnecessary micro-optimization.

5036–5037

I believe that bitcode reading is supposed to be resistant against invalid input and shouldn't assert in that case. At the same time we don't actually test these cases, because constructing the necessary invalid bitcode files would be really hard. I'm not sure if we have a fuzzer for bitcode fuzzing deployed anywhere.

[dblaikie]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1185–1190

Might be worth enabling it always (if not now, eventually/before/during the opaque pointer default switch) to validate that the new tracking solution (ElementTypeList) matches the old results?

5036–5037

(oh, I misunderstood - I'd thought these errors were only temporary while the support is incomplete? But I guess maybe these errors are permanent/could be reached even after support is fully implemented)

But in general bitcode reading errors can be/are tested for instance, here: llvm/test/Bitcode/invalid.test - though, yes, writing tests is hard. Not sure if we have any particularly good techniques - I suspect it's hex editing and such at the moment, unfortunately.

[nikic]

Always use element type list.

[nikic]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1185–1190

I've changed it to always use ElementTypeList now.

5036–5037

(oh, I misunderstood - I'd thought these errors were only temporary while the support is incomplete? But I guess maybe these errors are permanent/could be reached even after support is fully implemented)

Yes, these are permanent. An obvious way to hit this is via a non-pointer type -- which is the case this was checking for previously already.

[dblaikie]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5036–5037

@dexonsmith, @aprantl - what're your thoughts/ideas on invalid bitcode testing?

[aprantl]

For a one-off test checking in a manually crafted .bc file seems fine to me. If you are planning to add more tests, we should invest in some kind of yaml2bc utility (like yaml2obj). Not sure if this answers your question?

[dblaikie]

In D118694#3298975, @aprantl wrote:

For a one-off test checking in a manually crafted .bc file seems fine to me. If you are planning to add more tests, we should invest in some kind of yaml2bc utility (like yaml2obj). Not sure if this answers your question?

Yeah, was mostly asking how firm we are about constructing those sort of test cases. Seems not every existing error is covered - so I don't want to request undue work/unreasonable standard for new work like this - but testing error paths does seem important to me.

@nikic could you add hand-crafted bitcode test coverage for these error messages/paths?

[nikic]

In D118694#3299435, @dblaikie wrote:

In D118694#3298975, @aprantl wrote:

For a one-off test checking in a manually crafted .bc file seems fine to me. If you are planning to add more tests, we should invest in some kind of yaml2bc utility (like yaml2obj). Not sure if this answers your question?

Yeah, was mostly asking how firm we are about constructing those sort of test cases. Seems not every existing error is covered - so I don't want to request undue work/unreasonable standard for new work like this - but testing error paths does seem important to me.

@nikic could you add hand-crafted bitcode test coverage for these error messages/paths?

Sorry, I don't think that would be a good use of my time. Of course, if you'd like to construct invalid bitcode files yourself, I will not object to their inclusion.

[dblaikie]

Would it be possible/could you use the fuzzer to generate test cases that cover these error cases minimally, and check those in as coverage for them?

[nikic]

In D118694#3299687, @dblaikie wrote:

Would it be possible/could you use the fuzzer to generate test cases that cover these error cases minimally, and check those in as coverage for them?

I did a bit of work on the fuzzer side today and fixed the first half dozen issues I encountered. Unfortunately, it looks like this isn't a case where we're missing a small handful of checks -- there's assertion failures all over the place. It would take a significant amount of work to get us to a clean baseline, I can't even estimate how much.

[aeubanks]

sorry for the slow review, I think this is fine
I agree constructing invalid bitcode is probably not worth the time

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

487

perhaps a comment here would be good

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

class BitcodeReader : public BitcodeReaderBase, public GVMaterializer {
   // Last function offset found in the VST.
   uint64_t LastFunctionBlockBit = 0;
   bool SeenValueSymbolTable = false;
   uint64_t VSTOffset = 0;
 
   std::vector<std::string> SectionTable;
   std::vector<std::string> GCTable;
 
-  std::vector<Type*> TypeList;
+  std::vector<Type *> TypeList;
+  std::vector<Type *> ElementTypeList;
   DenseMap<Function *, FunctionType *> FunctionTypes;
   BitcodeReaderValueList ValueList;
   Optional<MetadataLoader> MDLoader;
   std::vector<Comdat *> ComdatList;
   DenseSet<GlobalObject *> ImplicitComdatObjects;
   SmallVector<Instruction *, 64> InstructionList;
 
   std::vector<std::pair<GlobalVariable *, unsigned>> GlobalInits;
public:
   void setStripDebugInfo() override;
 
 private:
   std::vector<StructType *> IdentifiedStructTypes;
   StructType *createIdentifiedStructType(LLVMContext &Context, StringRef Name);
   StructType *createIdentifiedStructType(LLVMContext &Context);
 
   Type *getTypeByID(unsigned ID);
+  Type *getElementTypeByID(unsigned ID);
 
   Value *getFnValueByID(unsigned ID, Type *Ty) {
     if (Ty && Ty->isMetadataTy())
       return MetadataAsValue::get(Ty->getContext(), getFnMetadataByID(ID));
     return ValueList.getValueFwdRef(ID, Ty);
   }
 
   Metadata *getFnMetadataByID(unsigned ID) {
Type *BitcodeReader::getTypeByID(unsigned ID) {
   if (Type *Ty = TypeList[ID])
     return Ty;
 
   // If we have a forward reference, the only possible case is when it is to a
   // named struct.  Just create a placeholder for now.
   return TypeList[ID] = createIdentifiedStructType(Context);
 }
 
+Type *BitcodeReader::getElementTypeByID(unsigned ID) {
+  if (ID >= TypeList.size())
+    return nullptr;
+
+  if (Context.supportsTypedPointers()) {
+    Type *Ty = TypeList[ID];
+    if (Ty->isPointerTy())
+      return Ty->getNonOpaquePointerElementType();
+    return nullptr;
+  }
+
+  return ID < ElementTypeList.size() ? ElementTypeList[ID] : nullptr;
+}
+
 StructType *BitcodeReader::createIdentifiedStructType(LLVMContext &Context,
                                                       StringRef Name) {
   auto *Ret = StructType::create(Context, Name);
   IdentifiedStructTypes.push_back(Ret);
   return Ret;
 }
 
 StructType *BitcodeReader::createIdentifiedStructType(LLVMContext &Context) {
while (true) {
     case BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read a record.
     Record.clear();
     Type *ResultTy = nullptr;
+    Type *ElemTy = nullptr;
     Expected<unsigned> MaybeRecord = Stream.readRecord(Entry.ID, Record);
     if (!MaybeRecord)
       return MaybeRecord.takeError();
     switch (MaybeRecord.get()) {
     default:
       return error("Invalid value");
     case bitc::TYPE_CODE_NUMENTRY: // TYPE_CODE_NUMENTRY: [numentries]
       // TYPE_CODE_NUMENTRY contains a count of the number of types in the
case bitc::TYPE_CODE_POINTER: { // POINTER: [pointee type] or
         return error("Invalid record");
       unsigned AddressSpace = 0;
       if (Record.size() == 2)
         AddressSpace = Record[1];
       ResultTy = getTypeByID(Record[0]);
       if (!ResultTy ||
           !PointerType::isValidElementType(ResultTy))
         return error("Invalid type");
+      ElemTy = ResultTy;
       ResultTy = PointerType::get(ResultTy, AddressSpace);
       break;
     }
     case bitc::TYPE_CODE_OPAQUE_POINTER: { // OPAQUE_POINTER: [addrspace]
       if (Record.size() != 1)
         return error("Invalid record");
       if (Context.supportsTypedPointers())
         return error(
while (true) {
     }
 
     if (NumRecords >= TypeList.size())
       return error("Invalid TYPE table");
     if (TypeList[NumRecords])
       return error(
           "Invalid TYPE table: Only named structs can be forward referenced");
     assert(ResultTy && "Didn't read a type?");
-    TypeList[NumRecords++] = ResultTy;
+    TypeList[NumRecords] = ResultTy;
+    if (ElemTy && !Context.supportsTypedPointers()) {
+      // We encountered a typed pointer in opaque pointers mode, and may need
+      // to auto-upgrade related IR. Remember the element type.
+      if (ElementTypeList.empty())
+        ElementTypeList.resize(TypeList.size());
+      ElementTypeList[NumRecords] = ElemTy;
+    }
+    ++NumRecords;
   }
 }
 
 Error BitcodeReader::parseOperandBundleTags() {
   if (Error Err = Stream.EnterSubBlock(bitc::OPERAND_BUNDLE_TAGS_BLOCK_ID))
     return Err;
 
   if (!BundleTags.empty())

 Error BitcodeReader::parseConstants() {
   if (Error Err = Stream.EnterSubBlock(bitc::CONSTANTS_BLOCK_ID))
     return Err;
 
   SmallVector<uint64_t, 64> Record;
 
   // Read all the records for this value table.
   Type *CurTy = Type::getInt32Ty(Context);
+  Type *CurElemTy = nullptr;
   unsigned NextCstNo = ValueList.size();
 
   struct DelayedShufTy {
     VectorType *OpTy;
     VectorType *RTy;
     uint64_t Op0Idx;
     uint64_t Op1Idx;
     uint64_t Op2Idx;
while (true) {
     case bitc::CST_CODE_SETTYPE:   // SETTYPE: [typeid]
       if (Record.empty())
         return error("Invalid record");
       if (Record[0] >= TypeList.size() || !TypeList[Record[0]])
         return error("Invalid record");
       if (TypeList[Record[0]] == VoidType)
         return error("Invalid constant type");
       CurTy = TypeList[Record[0]];
+      CurElemTy = getElementTypeByID(Record[0]);
       continue;  // Skip the ValueList manipulation.
     case bitc::CST_CODE_NULL:      // NULL
       if (CurTy->isVoidTy() || CurTy->isFunctionTy() || CurTy->isLabelTy())
         return error("Invalid type for a constant null value");
       V = Constant::getNullValue(CurTy);
       break;
     case bitc::CST_CODE_INTEGER:   // INTEGER: [intval]
       if (!CurTy->isIntegerTy() || Record.empty())
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>(CurTy->getPointerElementType()),
-                         AsmStr, ConstrStr, HasSideEffects, IsAlignStack);
+      if (!CurElemTy)
+        return error("Missing element type for old-style inlineasm");
+      V = InlineAsm::get(cast<FunctionType>(CurElemTy), AsmStr, ConstrStr,
+                         HasSideEffects, IsAlignStack);
       break;
     }
     // This version adds support for the asm dialect keywords (e.g.,
     // inteldialect).
     case bitc::CST_CODE_INLINEASM_OLD2: {
       if (Record.size() < 2)
         return error("Invalid record");
       std::string AsmStr, ConstrStr;
       bool HasSideEffects = Record[0] & 1;
       bool IsAlignStack = (Record[0] >> 1) & 1;
       unsigned AsmDialect = Record[0] >> 2;
       unsigned AsmStrSize = Record[1];
       if (2+AsmStrSize >= Record.size())
         return error("Invalid record");
       unsigned ConstStrSize = Record[2+AsmStrSize];
       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>(CurTy->getPointerElementType()),
-                         AsmStr, ConstrStr, HasSideEffects, IsAlignStack,
+      if (!CurElemTy)
+        return error("Missing element type for old-style inlineasm");
+      V = InlineAsm::get(cast<FunctionType>(CurElemTy), AsmStr, ConstrStr,
+                         HasSideEffects, IsAlignStack,
                          InlineAsm::AsmDialect(AsmDialect));
       break;
     }
     // This version adds support for the unwind keyword.
     case bitc::CST_CODE_INLINEASM_OLD3: {
       if (Record.size() < 2)
         return error("Invalid record");
       unsigned OpNum = 0;
case bitc::CST_CODE_INLINEASM_OLD3: {
         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>(CurTy->getPointerElementType()),
-                         AsmStr, ConstrStr, HasSideEffects, IsAlignStack,
+      if (!CurElemTy)
+        return error("Missing element type for old-style inlineasm");
+      V = InlineAsm::get(cast<FunctionType>(CurElemTy), 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;
Error BitcodeReader::parseGlobalVarRecord(ArrayRef<uint64_t> Record) {
   bool explicitType = Record[1] & 2;
   unsigned AddressSpace;
   if (explicitType) {
     AddressSpace = Record[1] >> 2;
   } else {
     if (!Ty->isPointerTy())
       return error("Invalid type for value");
     AddressSpace = cast<PointerType>(Ty)->getAddressSpace();
-    Ty = Ty->getPointerElementType();
+    Ty = getElementTypeByID(Record[0]);
+    if (!Ty)
+      return error("Missing element type for old-style global");
   }
 
   uint64_t RawLinkage = Record[3];
   GlobalValue::LinkageTypes Linkage = getDecodedLinkage(RawLinkage);
   MaybeAlign Alignment;
   if (Error Err = parseAlignmentValue(Record[4], Alignment))
     return Err;
   std::string Section;
Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
   StringRef Name;
   std::tie(Name, Record) = readNameFromStrtab(Record);
 
   if (Record.size() < 8)
     return error("Invalid record");
   Type *FTy = getTypeByID(Record[0]);
   if (!FTy)
     return error("Invalid record");
-  if (auto *PTy = dyn_cast<PointerType>(FTy))
-    FTy = PTy->getPointerElementType();
+  if (isa<PointerType>(FTy)) {
+    FTy = getElementTypeByID(Record[0]);
+    if (!FTy)
+      return error("Missing element type for old-style function");
+  }
 
   if (!isa<FunctionType>(FTy))
     return error("Invalid type for value");
   auto CC = static_cast<CallingConv::ID>(Record[1]);
   if (CC & ~CallingConv::MaxID)
     return error("Invalid calling convention ID");
 
   unsigned AddrSpace = TheModule->getDataLayout().getProgramAddressSpace();
Error BitcodeReader::parseGlobalIndirectSymbolRecord(
   // v2: [strtab_offset, strtab_size, v1]
   StringRef Name;
   std::tie(Name, Record) = readNameFromStrtab(Record);
 
   bool NewRecord = BitCode != bitc::MODULE_CODE_ALIAS_OLD;
   if (Record.size() < (3 + (unsigned)NewRecord))
     return error("Invalid record");
   unsigned OpNum = 0;
-  Type *Ty = getTypeByID(Record[OpNum++]);
+  unsigned TypeID = Record[OpNum++];
+  Type *Ty = getTypeByID(TypeID);
   if (!Ty)
     return error("Invalid record");
 
   unsigned AddrSpace;
   if (!NewRecord) {
     auto *PTy = dyn_cast<PointerType>(Ty);
     if (!PTy)
       return error("Invalid type for value");
-    Ty = PTy->getPointerElementType();
     AddrSpace = PTy->getAddressSpace();
+    Ty = getElementTypeByID(TypeID);
+    if (!Ty)
+      return error("Missing element type for old-style indirect symbol");
   } else {
     AddrSpace = Record[OpNum++];
   }
 
   auto Val = Record[OpNum++];
   auto Linkage = Record[OpNum++];
   GlobalValue *NewGA;
   if (BitCode == bitc::MODULE_CODE_ALIAS ||
case bitc::FUNC_CODE_INST_ALLOCA: { // ALLOCA: [instty, opty, op, align]
       if (Record.size() != 4)
         return error("Invalid record");
       using APV = AllocaPackedValues;
       const uint64_t Rec = Record[3];
       const bool InAlloca = Bitfield::get<APV::UsedWithInAlloca>(Rec);
       const bool SwiftError = Bitfield::get<APV::SwiftError>(Rec);
       Type *Ty = getTypeByID(Record[0]);
       if (!Bitfield::get<APV::ExplicitType>(Rec)) {
-        auto *PTy = dyn_cast_or_null<PointerType>(Ty);
-        if (!PTy)
-          return error("Old-style alloca with a non-pointer type");
-        Ty = PTy->getPointerElementType();
+        Ty = getElementTypeByID(Record[0]);
+        if (!Ty)
+          return error("Missing element type for old-style alloca");
       }
       Type *OpTy = getTypeByID(Record[1]);
       Value *Size = getFnValueByID(Record[2], OpTy);
       MaybeAlign Align;
       uint64_t AlignExp =
           Bitfield::get<APV::AlignLower>(Rec) |
           (Bitfield::get<APV::AlignUpper>(Rec) << APV::AlignLower::Bits);
       if (Error Err = parseAlignmentValue(AlignExp, Align)) {

llvm/test/Bitcode/aggregateInstructions.3.2.ll

-; RUN: llvm-dis < %s.bc| FileCheck %s
+; RUN: llvm-dis -opaque-pointers=0 < %s.bc| FileCheck %s
+; RUN: llvm-dis -opaque-pointers=1 < %s.bc| FileCheck %s
 ; RUN: verify-uselistorder < %s.bc
 
 ; aggregateOperations.3.2.ll.bc was generated by passing this file to llvm-as-3.2.
 ; The test checks that LLVM does not misread instructions with aggregate operands
 ; in older bitcode files.
 
 define void @extractvalue([4 x i8] %x1, [4 x [4 x i8]] %x2, {{i32, float}} %x3){
 entry: