Issue 1113023005: Add abilities to generate bitcode buffers from munged bitcode.

Issue 1113023005: Add abilities to generate bitcode buffers from munged bitcode. (Closed)

Created:
5 years, 7 months ago by Karl

Modified:
5 years, 7 months ago

Reviewers:
jvoung (off chromium), Derek Schuff

CC:
native-client-reviews_googlegroups.com

Base URL:
https://chromium.googlesource.com/native_client/pnacl-llvm.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

Add abilities to generate bitcode buffers from munged bitcode. This CL adds capabilities to read/write a munged list of bitcode records. BUG=None R=jvoung@chromium.org Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/4c20e87a21e2fee40f45769d6d5ae1a7ec1cc87a

Patch Set 2 : Initial version of code. #

Total comments: 30

Patch Set 3 : Fix issues brought up in patch set 2. #

Patch Set 4 : Try to get better patch. #

Total comments: 21

Patch Set 5 : Fix issues in patch set 4. #

Patch Set 6 : Fix nits. #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+876 lines, -307 lines)			Patch
M	include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h	View	1 2 3 4 5	8 chunks	+39 lines, -58 lines	0 comments	Download
M	include/llvm/Bitcode/NaCl/NaClBitcodeMungeUtils.h	View	1 2 3 4 5	6 chunks	+92 lines, -1 line	0 comments	Download
M	include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	lib/Bitcode/NaCl/TestUtils/CMakeLists.txt	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp	View	1 2 3 4 5	7 chunks	+18 lines, -233 lines	0 comments	Download
A	lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp	View	1 2	1 chunk	+157 lines, -0 lines	0 comments	Download
A	lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp	View	1 2 3 4 5	1 chunk	+333 lines, -0 lines	0 comments	Download
M	unittests/Bitcode/CMakeLists.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	unittests/Bitcode/NaClAbbrevErrorTests.cpp	View	1 2 3 4 5	2 chunks	+65 lines, -14 lines	0 comments	Download
A	unittests/Bitcode/NaClMungedIoTest.cpp	View	1 2	1 chunk	+168 lines, -0 lines	0 comments	Download

Messages

Total messages: 8 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jvoung (off chromium)

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp (right): https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp#newcode10 lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp:10: // Implements bitcode reader for NaClBitcodeRecordList and NaClMungedBitcode. extra ...

5 years, 7 months ago (2015-05-05 00:27:23 UTC) #3

Karl

Note that I changed the way writing munged bitcode files work. You pass in a ...

5 years, 7 months ago (2015-05-05 22:38:04 UTC) #4

Note that I changed the way writing munged bitcode files work. You pass in a
flags class defining what the writer can do (in terms of recovery/repairs), and
it returns a results class that defines how the write went.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp (right):

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp:10: // Implements bitcode
reader for NaClBitcodeRecordList and  NaClMungedBitcode.
On 2015/05/05 00:27:23, jvoung wrote:
> extra space in "  NaClMungedBitcode"

Done.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp:37: // Listener use to get
abbreviations as they are read.
On 2015/05/05 00:27:23, jvoung wrote:
> "use to get" -> "used to get" ?

Done.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp:39: //
On 2015/05/05 00:27:23, jvoung wrote:
> Dangling "//"

Done.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp:58: virtual
~BitcodeParser() override {
On 2015/05/05 00:27:23, jvoung wrote:
> Only need one "override" and don't need "virtual"

Done.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp:129:
report_fatal_error("Bitcode stream must be a multiple of 4 bytes in length");
On 2015/05/05 00:27:23, jvoung wrote:
> just a little too long for 80 cols

Done.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeReader.cpp:147: while
(!Parser.AtEndOfStream()) {
On 2015/05/05 00:27:22, jvoung wrote:
> There are a couple of instances of code that are very similar to this block
> (e.g., Analyzer, Compress, and ObjDump files). They use
"Cursor.AtEndOfStream()"
> directly. So, is it necessary to introduce the AtEndOfStream() wrapper in
> NaClBitcodeParser, or at least should the similar code be kept similar?

The problem I discovered was that this pattern was making a potentially bad
assumption by assuming that the cursor passed into the parser was the same as
the one used by the parser. I fixed this instance by adding the wrapper. I was
planning on fixing the other places as well with a separate wrapper.

Perhaps I should follow the other cases, and fix them all in a separate CL.
Changing the code to match the other cases (like Analyzer, Compress and
ObjDump).

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp (right):

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:105: unsigned
DefaultMaxBits =
On 2015/05/05 00:27:23, jvoung wrote:
> Hmm bug fix / got lucky before?

Yep. Because the test examples didn't really use abbreviations, this problem
didn't get caught. The new test case uses abbreviations and caused the discovery
of the problem.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:181:
Writer.EmitCode(Record.Abbrev);
On 2015/05/05 00:27:23, jvoung wrote:
> Just revisiting this since it's been a while... this says "we output the
record
> data using the default abbreviation pattern"
> 
> Should this be Writer.EmitCode(naclbitc::UNABBREV_RECORD) instead or the idea
is
> that you go ahead and emit the bad abbrev, but the reader knows to recover by
> following this format?

Actually, your close, but not quite right. The point of this code is to provide
a way to write out bitcode with a bad abbreviation value, so that we can test
the bitcode reader to see if it catches the problem. It probably is enough to
just generate the abbreviation code above, but I went ahead and added output
that looks close to what the abbreviation would look like.

I was letting the writer continue so that the proper number of exit blocks get
added (otherwise Writer fails on asserts). To make this clearer, I have modified
the code to stop writing as soon as something went wrong. This means that this
function now returns a boolean.

For this case, it generates the bad abbreviation index, followed by the proper
number of close blocks, and then returns false.

I have also modified buildAbbrev to return nullptr if an error occurs.

I have also modified Fatal() to record if an error occurred, rather than calling
ReportFatalError(). This allows the caller to NaClMungedBitcode::write to decide
what to do if there was a fatal error.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:193: // Rathter just set
SetBID and let call to Writer->EmitBlockInfoAbbrev
On 2015/05/05 00:27:23, jvoung wrote:
> Rathter -> Rather

Done.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:212: // BUilds an
abbreviation from the given bitcode record.
On 2015/05/05 00:27:23, jvoung wrote:
> BUilds -> Builds

Removed comment since the declaration already has a comment.

https://codereview.chromium.org/1113023005/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:301:
State.emitRecord(Writer, Record, RecoverOnBadAbbrevId);
On 2015/05/05 00:27:23, jvoung wrote:
> nit: Might have been able to make RecoverOnBadAbbrevId a field of State vs a
> parameter here, but either way is fine.

Good point. The state is probably the better place to put it. Changing.

https://codereview.chromium.org/1113023005/diff/20001/unittests/Bitcode/NaClM...
File unittests/Bitcode/NaClMungedIoTest.cpp (right):

https://codereview.chromium.org/1113023005/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungedIoTest.cpp:1: //===-
llvm/unittest/Bitcode/NaClMungedIoTest.cpp -------------------===//
On 2015/05/05 00:27:23, jvoung wrote:
> could fill up the --- to line up

Done.

https://codereview.chromium.org/1113023005/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungedIoTest.cpp:11: // For class NaClMungedBitcode, Tests
reading initial sequence of records and
On 2015/05/05 00:27:23, jvoung wrote:
> ", Tests reading" -> ", tests reading"

Done.

https://codereview.chromium.org/1113023005/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungedIoTest.cpp:51: void
testWriteMungedBitcode(NaClMungedBitcode &Bitcode) {
On 2015/05/05 00:27:23, jvoung wrote:
> This seems a little weird that it's ignoring the result.
> 
> Could you just use parseWrittenMungedBitcode in the death test instead of
having
> this variant as well? EXPECT_DEATH doesn't really care if the statement
returns
> a value vs void does it or if there is a bunch of remaining work to do if it
had
> not crashed earlier?

The reason for this function was because I used it in two spots in an earlier
version. Each call was expected to be run in an EXPECT_DEATH test. However,
since I cleaned up the test cases, it only appears in one place (inside an
EXPECT_DEATH). Removing this function.

https://codereview.chromium.org/1113023005/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungedIoTest.cpp:64: /// Sample toy bitcode records.
On 2015/05/05 00:27:23, jvoung wrote:
> nit: This isn't really an API or anything that needs to get doxygen'ized, so
no
> real need for the "///" in this file (here and line 87, and 124).

Done.

jvoung (off chromium)

https://codereview.chromium.org/1113023005/diff/60001/include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right): https://codereview.chromium.org/1113023005/diff/60001/include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h#newcode67 include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:67: NaClMungedBitcode::WriteFlags WriteFlags; nit: Seems a bit weird for this ...

5 years, 7 months ago (2015-05-06 22:08:10 UTC) #5

Karl

5 years, 7 months ago (2015-05-07 20:09:18 UTC) #6

https://codereview.chromium.org/1113023005/diff/60001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right):

https://codereview.chromium.org/1113023005/diff/60001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:67: NaClMungedBitcode::WriteFlags
WriteFlags;
On 2015/05/06 22:08:09, jvoung wrote:
> nit: Seems a bit weird for this field to be public when the MungedBitcode is
> not.

Moved to protected and added methods to set. Made these methods only apply to
the next test.

https://codereview.chromium.org/1113023005/diff/60001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeMungeUtils.h (right):

https://codereview.chromium.org/1113023005/diff/60001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeMungeUtils.h:176: /// than trying to error
recover.
On 2015/05/06 22:08:09, jvoung wrote:
> What does the Save case do after saving? Abort?
> 
> See other comment -- would it be clearer to be describing this as "written
out"
> instead of "saved" ?

Updated comment to clarify.

https://codereview.chromium.org/1113023005/diff/60001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeMungeUtils.h:180: /// Defines how writing
performed.
On 2015/05/06 22:08:09, jvoung wrote:
> "Defines how writing *is* performed"?

Changed to: Defines the results associated with writing bitcode.

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp (right):

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp:52: && (!WriteFlags.TryToRecover
|| !Results.HasRepairs)
On 2015/05/06 22:08:09, jvoung wrote:
> Hmm might be easier to read with a round of Captain DeMorgan's.
> 
> HasErrors
> && !(WriteFlags.TryToRecover && Results.HasRepairs)
> && !(FlagSaysOkayForX && ResultWasX)
> 
> ?

Done.

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp (right):

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:86: unsigned NumBits =
naclbitc::DEFAULT_MAX_ABBREV;
On 2015/05/06 22:08:09, jvoung wrote:
> Should this also be NaClBitsNeededForValue( naclbitc::DEFAULT_MAX_ABBREV) ?
> 
> If I'm reading this correctly, the initial value of NumBits happened to  never
> be read, because the it'll luckily bail first.

Good catch. Fixing!

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:183: if (Record.Abbrev >
BlockAbbrevIndexLimit)
On 2015/05/06 22:08:09, jvoung wrote:
> When does this > check fail? If for some reason the AbbrevIndexLimitStack is
> empty and the Limit isn't known?
> 
> Seems like you should know that the Abbrev > SomeLimit of some kind at this
> point, according to the "Can't generate abbreviation ID because not enough
> bits..." comment.

Decided to simplify this code and be more generic.

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:186: if
(!Flags.TryToRecover && !Flags.SaveBadAbbrevIndices)
On 2015/05/06 22:08:09, jvoung wrote:
> So this code is saying that SaveBadAbbrevIndices will have one type of error
> recovery, while plain "TryToRecover" will have a different type of error
> recovery?

Hopefully the code is now clearer since I simplified. If SaveBadAbbrevIndices is
true, we write out an incorrect but read testable bitcode file. If TryToRecover,
we default to the default abbreviation as a recovery.

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:190: //
RecoverOnBadAbbrevID is true. Generate bad abbreviation index
On 2015/05/06 22:08:09, jvoung wrote:
> there is no RecoverOnBadAbbrevID anymore
> 
> You also talk about "Can't generate abbreviation ID because not enough bits"
> earlier, so "If this point is reached, the abbreviation is bad..." applies to
> the earlier code too?

Again, I simplified this code so that we don't need to be so specific (and
complicated).

https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:196:
Results.SavedBadAbbrevIndices = true;
On 2015/05/06 22:08:09, jvoung wrote:
> Perhaps "Save..." should be "Write..." ?
> 
> In some ways Save is a bit less clear to me. It could be saved to some
> field/vector somewhere, while the destination of Write might deduced to be the
> Writer.

Hopefully I have clarified that the Save Case (now renamed WriteBadAbbrevIndex)
generates an illegal bitcode file that can be used to test the bitcode reader on
such a case.

https://codereview.chromium.org/1113023005/diff/60001/unittests/Bitcode/NaClA...
File unittests/Bitcode/NaClAbbrevErrorTests.cpp (right):

https://codereview.chromium.org/1113023005/diff/60001/unittests/Bitcode/NaClA...
unittests/Bitcode/NaClAbbrevErrorTests.cpp:98:
Munger.WriteFlags.SaveBadAbbrevIndices = true;
On 2015/05/06 22:08:09, jvoung wrote:
> It looks like there you could also separately test WriteFlags.TryRecover =
true
> w/ SaveBadAbbrevIndices = false, based on the check
> 
> +      if (!Flags.TryToRecover && !Flags.SaveBadAbbrevIndices)
> +        return false;

Added recovery cases. Also refactored the RunAsDeathTest so that it applies to
all mungers. I did this to guarantee we can see what did occurred before the
death of the test.

I also moved death tests into their own copies, so that there is no state
pollution (since the death may leave the munger in a bad state).

jvoung (off chromium)

lgtm https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp (right): https://codereview.chromium.org/1113023005/diff/60001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp#newcode186 lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:186: if (!Flags.TryToRecover && !Flags.SaveBadAbbrevIndices) On 2015/05/07 20:09:18, Karl ...

5 years, 7 months ago (2015-05-07 21:33:56 UTC) #7

Karl

5 years, 7 months ago (2015-05-07 22:22:55 UTC) #8

Message was sent while issue was closed.

Committed patchset #6 (id:100001) manually as
4c20e87a21e2fee40f45769d6d5ae1a7ec1cc87a (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages