Fix handling errors in switch instruction by PNaCl bitcode reader.

Fix handling errors in switch instruction by PNaCl bitcode reader.

The bitcode reader for the switch instruction has a couple of errors,
which can cause asserts/crashes to occur. One problem was that it
didn't check if the branch block of a case was defined. The other
problem was that if an error occurred after the switch instruction was
created, an assertion was fired for not deleting the switch
instruction.

BUG=None
R=jfb@chromium.org

Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/510766d08eef6c49b259a24fa8b5f6d6e7fa6071

[Karl, 2015-08-12 20:46:39]

kschimpf@google.com changed reviewers:
+ dschuff@chromium.org, jfb@chromium.org, jvoung@chromium.org

[Karl, 2015-08-17 17:45:27]

ping?

[JF, 2015-08-18 17:07:46]

https://codereview.chromium.org/1283353002/diff/1/lib/Bitcode/NaCl/Reader/NaC...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/1283353002/diff/1/lib/Bitcode/NaCl/Reader/NaC...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1514: delete SI;
Can this file as well as upstream lib/Bitcode/Reader/BitcodeReader.cpp use a
unique_ptr instead? All temporaries would have a well-bounded lifetime, and
would transfer ownership to I (also changed to unique_ptr) cleanly.

Or are switches special compared to other ops?

[Karl, 2015-08-19 19:39:00]

https://codereview.chromium.org/1283353002/diff/1/lib/Bitcode/NaCl/Reader/NaC...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/1283353002/diff/1/lib/Bitcode/NaCl/Reader/NaC...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1514: delete SI;
On 2015/08/18 17:07:46, JF wrote:
> Can this file as well as upstream lib/Bitcode/Reader/BitcodeReader.cpp use a
> unique_ptr instead? All temporaries would have a well-bounded lifetime, and
> would transfer ownership to I (also changed to unique_ptr) cleanly.
> 
> Or are switches special compared to other ops?

Switches are not special. The code was written this way because the upstream was
written this way. What makes this case different that most is that you can't
create a complete switch instruction with a single constructor. Rather, you
create a switch instruction with just the default branch, and then add each of
the cases by calling method addCase().

Hence, the switch is created up front, before the cases are processed. This
results in having to delete the switch if there is an error in the case.

I agree that a unique_ptr would be better in this case, and that the upstream
code should be fixed to use it as well. Fixing this CL to use unique_ptr in this
case.

[JF, 2015-08-20 16:24:00]

lgtm

https://codereview.chromium.org/1283353002/diff/20001/lib/Bitcode/NaCl/Reader...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/1283353002/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1878: I = SI.release();
At this point it's probably better to do the change upstream and then copy it
over here: I'd like `I` to be a `unique_ptr`, and all the other intermediate
instructions (not just switch).

[Karl, 2015-08-20 18:30:51]

https://codereview.chromium.org/1283353002/diff/20001/lib/Bitcode/NaCl/Reader...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/1283353002/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1878: I = SI.release();
On 2015/08/20 16:24:00, JF wrote:
> At this point it's probably better to do the change upstream and then copy it
> over here: I'd like `I` to be a `unique_ptr`, and all the other intermediate
> instructions (not just switch).

Acknowledged.

[Karl, 2015-08-20 18:31:56]

Committed patchset #2 (id:20001) manually as
510766d08eef6c49b259a24fa8b5f6d6e7fa6071 (presubmit successful).