Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(105)

Issue 1146203003: Fix abbreviation handling in munged bitcode writer. (Closed)

Created:
5 years, 7 months ago by Karl
Modified:
5 years, 7 months ago
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.

Description

Fix abbreviation handling in munged bitcode writer. The bitstream writer assumes several properties, and will fail (either through assert or core dump) if these properties aren't true. Fixing munged bitcode writer to handle the following assumptions: 1) Don't write out an abbreviation if the abbreviation can't be applied to the record. 2) Don't write out an abbreviation definition if the record doesn't define a valid abbreviation (previous code was checking this, but not correctly for the abbreviation operand). 3) Don't use abbreviations whose abbreviation index is larger than couldn't be written out (case 2). Such indices will be broken because the bitstream reader doesn't correctly match. BUG=https://code.google.com/p/nativeclient/issues/detail?id=4169 R=jvoung@chromium.org Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/e439162d4e1a4b422cab9dfc07819f891f633306

Patch Set 1 #

Patch Set 2 : Fix nits. #

Patch Set 3 : Don't allow application of abbreviation if index out of range. #

Total comments: 39

Patch Set 4 : Fix isses raised in patch set 3. #

Total comments: 4

Patch Set 5 : Fix nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+350 lines, -60 lines) Patch
M include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h View 1 2 3 4 6 chunks +36 lines, -23 lines 0 comments Download
M lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp View 1 2 3 14 chunks +128 lines, -23 lines 0 comments Download
M unittests/Bitcode/NaClMungeWriteErrorTests.cpp View 1 2 3 4 4 chunks +186 lines, -14 lines 0 comments Download

Messages

Total messages: 9 (1 generated)
Karl
5 years, 7 months ago (2015-05-20 19:31:41 UTC) #2
Karl
https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp (right): https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp#newcode239 lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:239: NaClBitstreamWriter &Writer, const NaClBitcodeAbbrevRecord &Record) { Added following two ...
5 years, 7 months ago (2015-05-20 20:32:04 UTC) #3
jvoung (off chromium)
https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h#newcode425 include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:425: // the abbreviation index. Returns nullptr if no such ...
5 years, 7 months ago (2015-05-20 22:40:10 UTC) #4
Karl
https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h#newcode425 include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:425: // the abbreviation index. Returns nullptr if no such ...
5 years, 7 months ago (2015-05-21 18:22:08 UTC) #5
jvoung (off chromium)
LGTM -- please remember to update deps when ready =) https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h#newcode164 ...
5 years, 7 months ago (2015-05-21 20:58:00 UTC) #6
Karl
Committed patchset #5 (id:80001) manually as e439162d4e1a4b422cab9dfc07819f891f633306 (presubmit successful).
5 years, 7 months ago (2015-05-22 15:03:45 UTC) #7
Karl
https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h#newcode164 include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:164: // Nax Number of bits that can be written ...
5 years, 7 months ago (2015-05-22 15:30:43 UTC) #8
Karl
5 years, 7 months ago (2015-05-22 15:30:43 UTC) #9
Message was sent while issue was closed.
https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right):

https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:164: // Nax Number of bits that
can be written using Emit.
On 2015/05/21 20:57:59, jvoung wrote:
> Nax Number -> Max number

Done.

https://codereview.chromium.org/1146203003/diff/60001/unittests/Bitcode/NaClM...
File unittests/Bitcode/NaClMungeWriteErrorTests.cpp (right):

https://codereview.chromium.org/1146203003/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:464: // Block only spefies 2 bits
for abbreviations (i.e. limit = 3).
On 2015/05/21 20:57:59, jvoung wrote:
> spefies -> specifies

Done.

Powered by Google App Engine
This is Rietveld 408576698