Add (unsupported experimental) feature allowing byte aligned bitcode.

Add (unsupported experimental) feature allowing byte aligned bitcode.

This is the first step of allowing PNaCl bitcode files to generate and
accept byte aligned bitcode records. To write bitcode files with byte
alignment, one uses the command-line argument
'--align-bitcode-records'. When such bitcode files are written, a
corresponding flag is added to the bitcode header so that the reader
knows whether it should align bitcode records. This allows old PNaCl
bitcode files to continue to work.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=4164
R=jvoung@chromium.org

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

[Karl, 2015-05-06 19:30:00]

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

[jvoung (off chromium), 2015-05-07 18:11:17]

Would probably be a good idea to file a bug to track this since there will be
multiple steps and some evaluation.

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeHeader.h (right):

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeHeader.h:170: // Byte align bitcode records
when nonzero.
Is this meant to say how much to align to (why uint32_t vs a bool)? Is that for
future expansion?

The field type is a "kFlagType" so you can't really specify an alignment amount
as is. Expansion would require making a different Tag with a different field
type.

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:89: InitialAddress =
MyInitialAddress;
This variant no longer calls init(Start, End) so
BitcodeBytes.reset(getNonStreamedMemoryObject(Start, End)); no longer happens?

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:93: NaClBitcodeHeader &Header) {
Not a huge fan of having some many variants of the constructor, since you have
to make sure they all initialize enough of the fields.

Could you have just one of the "MyInitialAddress" vs "Header" variants instead
of both? It's not clear what the versions that don't call initFromHeader() are
missing out on.

Otherwise, could you make the InitialAddress initialization stylistically
consistent? One of ctors does "InitialAddress = Header.getHeaderSize();", and
the other does ": InitialAddress(Header.getHeaderSize())"?

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClCompress.cpp (right):

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClCompress.cpp:1165: if (Header.Read(HeaderPtr,
EndBufPtr) || !Header.IsSupported())
Should this check of Read vs IsSupported be split too? (like NaClObjDump and
Analyze)

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

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:297: IsReadableFlag = IsReadable;
Can a second call to ReportProblem cause you to overwrite a previous setting of
IsReadableFlag (from false -> true again)? The loop sometimes will "continue;"
after a ReportProblem... call, so it seems possible (but maybe not)?

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:301: bool IsReadable=false) {
spaces instead of tabs

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:338:
UnsupportedMessage.append("Unsupported");
got some tabs

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Writer...
File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right):

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Writer...
lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1183: Header.push_back(new
NaClBitcodeHeaderField(NaClBitcodeHeaderField::kAlignBitcodeRecords));
80 col

https://codereview.chromium.org/1122423005/diff/20001/test/NaCl/Bitcode/pnacl...
File test/NaCl/Bitcode/pnacl-bcdis/no-abbreviations.ll (right):

https://codereview.chromium.org/1122423005/diff/20001/test/NaCl/Bitcode/pnacl...
test/NaCl/Bitcode/pnacl-bcdis/no-abbreviations.ll:148: ; ALN-NEXT:      58:0|   
2: <65533, 4, 1, 2, 0, 2,|      @a3 = abbrev <2, vbr(8),
neat -- can see that all the records are byte aligned =)

[Karl, 2015-05-07 22:18:55]

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeHeader.h (right):

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeHeader.h:170: // Byte align bitcode records
when nonzero.
On 2015/05/07 18:11:17, jvoung wrote:
> Is this meant to say how much to align to (why uint32_t vs a bool)? Is that
for
> future expansion?
> 
> The field type is a "kFlagType" so you can't really specify an alignment
amount
> as is. Expansion would require making a different Tag with a different field
> type.

My screw up. Always meant it to be a boolean flag. Fixing.

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:89: InitialAddress =
MyInitialAddress;
On 2015/05/07 18:11:17, jvoung wrote:
> This variant no longer calls init(Start, End) so
> BitcodeBytes.reset(getNonStreamedMemoryObject(Start, End)); no longer happens?

Bad refactoring on my part. Fixing to have both forms.

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:93: NaClBitcodeHeader &Header) {
On 2015/05/07 18:11:17, jvoung wrote:
> Not a huge fan of having some many variants of the constructor, since you have
> to make sure they all initialize enough of the fields.
> 
> Could you have just one of the "MyInitialAddress" vs "Header" variants instead
> of both? It's not clear what the versions that don't call initFromHeader() are
> missing out on.
> 
> Otherwise, could you make the InitialAddress initialization stylistically
> consistent? One of ctors does "InitialAddress = Header.getHeaderSize();", and
> the other does ": InitialAddress(Header.getHeaderSize())"?

There are two reasons for multiple headers. The first is that Subzero uses the
old constructor, and hence it can't be removed (or not yet). Some unit tests run
without adding a header, and hence it can't be removed.

However, many cases use a header, and it is annoying to have to repeat this
pattern on those cases. Once we fix subzero, I think we can
simplify these two constructors to:

NaClBitstreamReader(Start, End) and NaClBitstreamReader(Start, End, Header).
Note that we already have both forms for the MemoryObject version.

Decided to use MemoryObject on all (will submit fix to subzero separately).

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClCompress.cpp (right):

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClCompress.cpp:1165: if (Header.Read(HeaderPtr,
EndBufPtr) || !Header.IsSupported())
On 2015/05/07 18:11:17, jvoung wrote:
> Should this check of Read vs IsSupported be split too? (like NaClObjDump and
> Analyze)

Good point. Changing.

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

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:297: IsReadableFlag = IsReadable;
On 2015/05/07 18:11:17, jvoung wrote:
> Can a second call to ReportProblem cause you to overwrite a previous setting
of
> IsReadableFlag (from false -> true again)? The loop sometimes will "continue;"
> after a ReportProblem... call, so it seems possible (but maybe not)?
> 

The intent is continue with the next field, and not report any more problems
with the current field.

The current code does this directly or indirectly. However, we should make it
clear that this is what we want. Changing all calls to ReportProblem... be
immediately followed by a continue.

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:301: bool IsReadable=false) {
On 2015/05/07 18:11:17, jvoung wrote:
> spaces instead of tabs

Done.

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:338:
UnsupportedMessage.append("Unsupported");
On 2015/05/07 18:11:17, jvoung wrote:
> got some tabs

Done.

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Writer...
File lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp (right):

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Writer...
lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp:1183: Header.push_back(new
NaClBitcodeHeaderField(NaClBitcodeHeaderField::kAlignBitcodeRecords));
On 2015/05/07 18:11:17, jvoung wrote:
> 80 col

Done.

https://codereview.chromium.org/1122423005/diff/20001/test/NaCl/Bitcode/pnacl...
File test/NaCl/Bitcode/pnacl-bcdis/no-abbreviations.ll (right):

https://codereview.chromium.org/1122423005/diff/20001/test/NaCl/Bitcode/pnacl...
test/NaCl/Bitcode/pnacl-bcdis/no-abbreviations.ll:148: ; ALN-NEXT:      58:0|   
2: <65533, 4, 1, 2, 0, 2,|      @a3 = abbrev <2, vbr(8),
On 2015/05/07 18:11:17, jvoung wrote:
> neat -- can see that all the records are byte aligned =)

Acknowledged.

[jvoung (off chromium), 2015-05-08 17:55:01]

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:93: NaClBitcodeHeader &Header) {
On 2015/05/07 22:18:54, Karl wrote:
> On 2015/05/07 18:11:17, jvoung wrote:
> > Not a huge fan of having some many variants of the constructor, since you
have
> > to make sure they all initialize enough of the fields.
> > 
> > Could you have just one of the "MyInitialAddress" vs "Header" variants
instead
> > of both? It's not clear what the versions that don't call initFromHeader()
are
> > missing out on.
> > 
> > Otherwise, could you make the InitialAddress initialization stylistically
> > consistent? One of ctors does "InitialAddress = Header.getHeaderSize();",
and
> > the other does ": InitialAddress(Header.getHeaderSize())"?
> 
> There are two reasons for multiple headers. The first is that Subzero uses the
> old constructor, and hence it can't be removed (or not yet). Some unit tests
run
> without adding a header, and hence it can't be removed.
> 
> However, many cases use a header, and it is annoying to have to repeat this
> pattern on those cases. Once we fix subzero, I think we can
> simplify these two constructors to:
> 
> NaClBitstreamReader(Start, End) and NaClBitstreamReader(Start, End, Header).
> Note that we already have both forms for the MemoryObject version.
> 
> Decided to use MemoryObject on all (will submit fix to subzero separately).

Yeah it is definitely annoying to have to either set up a header or set up a
getNonStreamedMemoryObject in more places...

I'd consider the form with the explicit InitialAddress (no header) less useful
compared to the (Start, End) variant. The InitialAddress is used by passing in
Header.getHeaderSize(), but that fails to pass along the other header fields.

I brought this up because it seemed easy to start drifting on the amount of
initialization in the various ctor combos and wasn't clear which variants were
tested where. There were really (2 x 3 + 1) variants:

{Start/End vs MemObj} x {Header vs InitialAddress=X vs InitialAddress
unspecified) + null-ctor.

So considering those priorities, something like the following seemed reasonable
to me:

(1) NaClBitstreamReader(Start, End, Header)      // For convenience of
non-streaming, non-lazy tools.
(2) NaClBitstreamReader(MemObj, Header)          // For streaming/lazy tools.
(3) NaClBitstreamReader(MemObj, InitialAddress)  // Limit this use... Force
unittest to just pick a very specific form w/out the header and w/out a default
parameter.
(4) NaClBitstreamReader() // For ???

WDYT?

If (4) can be removed that would be nice too.

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

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:297: IsReadableFlag = IsReadable;
On 2015/05/07 22:18:54, Karl wrote:
> On 2015/05/07 18:11:17, jvoung wrote:
> > Can a second call to ReportProblem cause you to overwrite a previous setting
> of
> > IsReadableFlag (from false -> true again)? The loop sometimes will
"continue;"
> > after a ReportProblem... call, so it seems possible (but maybe not)?
> > 
> 
> The intent is continue with the next field, and not report any more problems
> with the current field.
> 
> The current code does this directly or indirectly. However, we should make it
> clear that this is what we want. Changing all calls to ReportProblem... be
> immediately followed by a continue.

I mean that since you are processing all the fields even after hitting an
problem, what if there was an early unreadable problem, followed by
"ReportProblemWithContents(Field, true)" at the end? Then IsReadable flips from
false to true, even though it should stay false (I think). E.g., header has a
bad bitcode version followed by the align records flag.

So the update would be safer if it was like "IsReadableFlag = IsReadableFlag &&
IsReadable"?

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp (right):

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp:411: const unsigned char
*HeaderPtr = BufPtr;
I know Header.Read(x, ...) actually modifies x, but I'm not sure why this is
changed now? Is the NaClBistreamReader tolerant of starting where the header
begins instead of after?

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Reader...
File lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp (right):

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:140: switch (FType) {
no need for extra indent on switch (FType)

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:292: std::set<unsigned> FieldIDs;
Might be able to just use a SmallSet<unsigned, kTag_MAX> ?

[Karl, 2015-05-08 21:09:00]

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1122423005/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:93: NaClBitcodeHeader &Header) {
On 2015/05/08 17:55:01, jvoung wrote:
> On 2015/05/07 22:18:54, Karl wrote:
> > On 2015/05/07 18:11:17, jvoung wrote:
> > > Not a huge fan of having some many variants of the constructor, since you
> have
> > > to make sure they all initialize enough of the fields.
> > > 
> > > Could you have just one of the "MyInitialAddress" vs "Header" variants
> instead
> > > of both? It's not clear what the versions that don't call initFromHeader()
> are
> > > missing out on.
> > > 
> > > Otherwise, could you make the InitialAddress initialization stylistically
> > > consistent? One of ctors does "InitialAddress = Header.getHeaderSize();",
> and
> > > the other does ": InitialAddress(Header.getHeaderSize())"?
> > 
> > There are two reasons for multiple headers. The first is that Subzero uses
the
> > old constructor, and hence it can't be removed (or not yet). Some unit tests
> run
> > without adding a header, and hence it can't be removed.
> > 
> > However, many cases use a header, and it is annoying to have to repeat this
> > pattern on those cases. Once we fix subzero, I think we can
> > simplify these two constructors to:
> > 
> > NaClBitstreamReader(Start, End) and NaClBitstreamReader(Start, End, Header).
> > Note that we already have both forms for the MemoryObject version.
> > 
> > Decided to use MemoryObject on all (will submit fix to subzero separately).
> 
> Yeah it is definitely annoying to have to either set up a header or set up a
> getNonStreamedMemoryObject in more places...
> 
> I'd consider the form with the explicit InitialAddress (no header) less useful
> compared to the (Start, End) variant. The InitialAddress is used by passing in
> Header.getHeaderSize(), but that fails to pass along the other header fields.
> 
> I brought this up because it seemed easy to start drifting on the amount of
> initialization in the various ctor combos and wasn't clear which variants were
> tested where. There were really (2 x 3 + 1) variants:
> 
> {Start/End vs MemObj} x {Header vs InitialAddress=X vs InitialAddress
> unspecified) + null-ctor.
> 
> So considering those priorities, something like the following seemed
reasonable
> to me:
> 
> (1) NaClBitstreamReader(Start, End, Header)      // For convenience of
> non-streaming, non-lazy tools.
> (2) NaClBitstreamReader(MemObj, Header)          // For streaming/lazy tools.
> (3) NaClBitstreamReader(MemObj, InitialAddress)  // Limit this use... Force
> unittest to just pick a very specific form w/out the header and w/out a
default
> parameter.
> (4) NaClBitstreamReader() // For ???
> 
> WDYT?
> 
> If (4) can be removed that would be nice too.

Looks reasonable, changing.

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

https://codereview.chromium.org/1122423005/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:297: IsReadableFlag = IsReadable;
On 2015/05/08 17:55:01, jvoung wrote:
> On 2015/05/07 22:18:54, Karl wrote:
> > On 2015/05/07 18:11:17, jvoung wrote:
> > > Can a second call to ReportProblem cause you to overwrite a previous
setting
> > of
> > > IsReadableFlag (from false -> true again)? The loop sometimes will
> "continue;"
> > > after a ReportProblem... call, so it seems possible (but maybe not)?
> > > 
> > 
> > The intent is continue with the next field, and not report any more problems
> > with the current field.
> > 
> > The current code does this directly or indirectly. However, we should make
it
> > clear that this is what we want. Changing all calls to ReportProblem... be
> > immediately followed by a continue.
> 
> I mean that since you are processing all the fields even after hitting an
> problem, what if there was an early unreadable problem, followed by
> "ReportProblemWithContents(Field, true)" at the end? Then IsReadable flips
from
> false to true, even though it should stay false (I think). E.g., header has a
> bad bitcode version followed by the align records flag.
> 
> So the update would be safer if it was like "IsReadableFlag = IsReadableFlag
&&
> IsReadable"?

Good point. Fixing.

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp (right):

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp:411: const unsigned char
*HeaderPtr = BufPtr;
On 2015/05/08 17:55:01, jvoung wrote:
> I know Header.Read(x, ...) actually modifies x, but I'm not sure why this is
> changed now? Is the NaClBistreamReader tolerant of starting where the header
> begins instead of after?

I changed this a while back. The problem is that the bitstream reader assumes
the beginning passed in is the beginning of the corresponding file it comes
from. Hence, if you pass in the modified BufPtr (as passed to Header.Read(), it
will incorrectly report bitcode byte locations.

The InitialAddress value let adjust the beginning to the position past the
header. However, the old code wasn't doing that, because of the fact that BufPtr
started past the beginning of the bitcode header.

I fixed some of the applications, but I apparently missed a few (including this
one).

I agree that it would be nice to fix Header.Read() to not update the pointer,
and added it to this CL.

The new constructors for NaClBitstreamReader with a Header argument
automatically set the initial index based on the size of the header, allowing
one not to pass that value in.

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Reader...
File lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp (right):

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:140: switch (FType) {
On 2015/05/08 17:55:01, jvoung wrote:
> no need for extra indent on switch (FType)

Done.

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:292: std::set<unsigned> FieldIDs;
On 2015/05/08 17:55:01, jvoung wrote:
> Might be able to just use a SmallSet<unsigned, kTag_MAX> ?

Done.

[jvoung (off chromium), 2015-05-11 17:51:39]

lgtm

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp (right):

https://codereview.chromium.org/1122423005/diff/40001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp:411: const unsigned char
*HeaderPtr = BufPtr;
On 2015/05/08 21:09:00, Karl wrote:
> On 2015/05/08 17:55:01, jvoung wrote:
> > I know Header.Read(x, ...) actually modifies x, but I'm not sure why this is
> > changed now? Is the NaClBistreamReader tolerant of starting where the header
> > begins instead of after?
> 
> I changed this a while back. The problem is that the bitstream reader assumes
> the beginning passed in is the beginning of the corresponding file it comes
> from. Hence, if you pass in the modified BufPtr (as passed to Header.Read(),
it
> will incorrectly report bitcode byte locations.
> 
> The InitialAddress value let adjust the beginning to the position past the
> header. However, the old code wasn't doing that, because of the fact that
BufPtr
> started past the beginning of the bitcode header.
> 
> I fixed some of the applications, but I apparently missed a few (including
this
> one).
> 
> I agree that it would be nice to fix Header.Read() to not update the pointer,
> and added it to this CL.

Thanks, that's easier to follow.


> 
> The new constructors for NaClBitstreamReader with a Header argument
> automatically set the initial index based on the size of the header, allowing
> one not to pass that value in.

https://codereview.chromium.org/1122423005/diff/80001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1122423005/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:97: {
nit: seems like the other cases put the { on the previous line

https://codereview.chromium.org/1122423005/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:107: // Read stream from Bytes,
starting at the given initial address
ctor-like comment for non-ctor?

[Karl, 2015-05-11 19:40:40]

Committed patchset #6 (id:90001) manually as
f46b7247105268eb474eda566076eab0e7766676 (presubmit successful).

[Karl, 2015-05-11 19:41:09]

https://codereview.chromium.org/1122423005/diff/80001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1122423005/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:97: {
On 2015/05/11 17:51:39, jvoung wrote:
> nit: seems like the other cases put the { on the previous line

Done.

https://codereview.chromium.org/1122423005/diff/80001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:107: // Read stream from Bytes,
starting at the given initial address
On 2015/05/11 17:51:39, jvoung wrote:
> ctor-like comment for non-ctor?

Done.