[PATCH] Refactor bitcode reader to simplify control.

[Filipe Cabecinhas]

Hi Karl,

On Thu, May 28, 2015 at 3:09 PM, Karl Schimpf <kschimpf at google.com> wrote:

> Fixes based on feedback by Filipe.
>
>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:289
> @@ -251,1 +288,3 @@
> +  /// \param M the module to build.
>
> +  /// \param ShouldMaterializeAll true when the module should be
> materialized
> ----------------
> filcab wrote:
> > Why the empty line?
> Removed.
>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:789
> @@ -717,3 +788,3 @@
>  namespace {
> -  /// @brief A class for maintaining the slot number definition
> +  /// \brief A class for maintaining the slot number definition
>    /// as a placeholder for the actual definition for forward constants
> defs.
> ----------------
> filcab wrote:
> > Omit \brief.
> Done.
>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:803
> @@ -731,3 +802,3 @@
>
> -    /// @brief Methods to support type inquiry through isa, cast, and
> dyn_cast.
> +    /// \brief Methods to support type inquiry through isa, cast, and
> dyn_cast.
>      static bool classof(const Value *V) {
> ----------------
> filcab wrote:
> > Omit \brief.
> Done.
>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3254
> @@ -3125,2 +3253,3 @@
>      case BitstreamEntry::EndBlock:
> +      setParseState(AtTopLevel);
>        return std::error_code();
> ----------------
> filcab wrote:
> > We're already at top level, no? (line 3234)
> > I might be missing something, but it looks like we're at top level, and
> saw an EndBlock. Shouldn't this be an error?
> I agree that we should be at top level.
>
> I also agree that it appears weird that we allow extra (unmathced)
> EndBlocks. This has been allowed by the bitcode reader/writer for years. I
> just wasn't willing to make the leap that I should remove this.
>
> However, I tried removing it (and making it an error), and no tests
> failed. Hence, Converting this to an error.
>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4676
> @@ -4517,2 +4675,3 @@
> +    return EC;
>
>    // Iterate over the module, deserializing any functions that are still
> on
> ----------------
> Moved the iterating of functions to before the call to finishParse. This
> deals with the problems I was having with tests in
> test/Bitcode/invalid.test (i.e. Inputs/invalid-fwdref-type-mismatch-2.bc
> and Inputs/invalid-load-ptr-type.bc). These two files had multiple errors
> (the one they intended which was inside a function body, and the one
> probably not intended - extraneous stuff at the end of the bitcode file).
>
> This removes the need for the command line flag UseOldLazyBitcodeParse,
> and I have deleted it.
>

The files came straight from the fuzzer, so it is likely have more than one
error. If, in order to support them (where support is: keep the test
working and diagnosing what we want), you have to change the code in a
convoluted way, I would prefer to change the test.

If the change is minimal and not a problem (doesn't impact legibility or
architecture), then keeping the tests as they are is not a problem either.
I just want to avoid having worse code just so we don't have to re-do some
tests.

Of course, if we started crashing on the tests, that's a problem :-)

Thanks,

  Filipe


>
> http://reviews.llvm.org/D8786
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150528/5e01313b/attachment.html>