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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: aprantl.
dblaikie added inline comments.


================
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");
       }
----------------
nikic wrote:
> 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.
@dexonsmith, @aprantl - what're your thoughts/ideas on invalid bitcode testing?


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

https://reviews.llvm.org/D118694