[PATCH] Refactor bitcode reader to simplify control.

[Karl Schimpf]

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:243
@@ -221,1 +242,3 @@
+  /// The number of modules read at the top level.
+  bool NumModulesParsed = 0;
 
----------------
jvoung wrote:
> nit: This is usually 0 or 1, but it seems unexpected for this field to be named "NumModulesParsed", and yet have the type be "bool". Rename or change type?
Good catch. I did meant to use size_t. Fixing.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3113
@@ +3112,3 @@
+    if (std::error_code EC = MaterializeModule(TheModule))
+      return EC;
+  } else {
----------------
jvoung wrote:
> Does this need to be cleanupOnError(EC) also?
Yes. Good catch. Fixing.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3151
@@ +3150,3 @@
+    return Error("Can't continue, bitcode error already found");
+  default:
+    // Restore input position to saved position on last call.
----------------
jvoung wrote:
> This covers two states? InsideModule and AtTopLevel? It might be more clear if you list them out so it's clear what the "break" corresponds to (AtTopLevel).
> 
> Previously, the Stream.JumpToBit(NextUnreadBit); was only needed when InsideModule... is it now needed for AtTopLevel too?
The jumpToBit is needed because various "materialize" methods may be called between calls to continueParse. By forcing a jumpToBit to happen at all calls to continueParse, we no longer need to know where the materialize methods leave the bitcursor.

While I did not see an example of an error caused by interleaved calls to materialize, I was very suspicious that they could occur, and wanted to make sure that this would not happen. Hence, I made sure that continueParse always resets the position to where it left off.

I will fix to not use default, so that a corresponding warning will be generated if a new value is added to the enumeration.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3244
@@ +3243,3 @@
+  default:
+    llvm_unreachable("finishParse exits with ParseState < NoMorInput");
+  }
----------------
jvoung wrote:
> "NoMorInput" -> "NoMoreInput"
> 
> It could also be that more states >= NoMoreInput are added as code evolves, but not handled here. Can the compiler accept/handle a "static_assert(ParseState < NoMoreInput, "...") to catch what happens if more states are added after NoMoreInput but not handled by this switch?
Fixed string. Also removed "default" case and made all states explicit. This will force a warning if a new state is added.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4503
@@ +4502,3 @@
+  // pointing to the END_BLOCK record after them. Now make sure the rest
+  // of the bits in the module have been read.
+  if (std::error_code EC = finishParse())
----------------
Removing the comment about being after a function body. This is no longer true. A call to materializeMetadata would put us some place else in the bitcode file.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4518
@@ +4517,3 @@
+
+  if (std::error_code EC = materializeForwardReferencedFunctions())
+    return EC;
----------------
jvoung wrote:
> Is this necessary at this point? Should that already be covered by the " // Iterate over the module, deserializing any functions that are still on disk" loop?
In correct bitcode files, you are right. However, if the function doesn't define any function blocks, but (incorrectly) references function block addresses, this code will cause the error to be generated.

However, looking at the following instruction, this is checked anyway. Removing.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4522
@@ -4387,3 +4521,3 @@
   // Check that all block address forward references got resolved (as we
   // promised above).
   if (!BasicBlockFwdRefs.empty())
----------------
jvoung wrote:
> The "promise" comment from "above" is removed now, so you could update this comment.
Done.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:424
@@ +423,3 @@
+
+  std::error_code updateParseState() {
+    NextUnreadBit = Stream.GetCurrentBitNo();
----------------
jvoung wrote:
> kschimpf wrote:
> > jvoung wrote:
> > > This is a bit weird to me... you have a bunch of function called "updateParseState" but some variants modify ParseState and some variants don't.
> > The refactoring is a bit better now. Hopefully good enough.
> Thanks -- this is better.
> 
> For a while I was also wondering how many places need to be aware of setting the state to ParseError, but I think it's just continueParse() because most/all searching for bit position, etc. goes through that.
That is correct and was the intent. State updates (and bit positioning) is intentionally now localized to continueParse.

The only exception is in ParseModule, which updates the state to state whether it returned without completing.

http://reviews.llvm.org/D8786

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/