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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1185-1190
+  if (Context.supportsTypedPointers()) {
+    Type *Ty = TypeList[ID];
+    if (Ty->isPointerTy())
+      return Ty->getNonOpaquePointerElementType();
+    return nullptr;
+  }
----------------
dblaikie wrote:
> 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?)
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.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5036-5037
+        Ty = getElementTypeByID(Record[0]);
+        if (!Ty)
+          return error("Missing element type for old-style alloca");
       }
----------------
dblaikie wrote:
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118694/new/

https://reviews.llvm.org/D118694