[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:
> nikic wrote:
> > 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.
> 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?
I've changed it to always use ElementTypeList now.


================
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:
> nikic wrote:
> > 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.
> (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.
> (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.


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

https://reviews.llvm.org/D118694