Allow pnacl-sz to be compiled to textual bitcode records.

Allow pnacl-sz to be compiled to textual bitcode records.

This has been added to allow fuzzing to be applied to textual bitcode
records. When built with make option TEXTUAL_BITCODE=1, the
corresponding generated pnacl-sz will preprocess the input file
(containing the textual form of bitcode records) and generate
a corresponding data stream with the binary form.

Note that the texual form of bitcode records is not LLVM assembly
(i.e. .ll files). Rather, it is sequences of texual integers
corresponding to bitcode records.

Dependent on: https://codereview.chromium.org/1191393004

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

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=6f9ba115f546d0e3446f08bd6f4477d2b5645bd5

[Karl, 2015-06-19 20:00:51]

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

[Jim Stichnoth, 2015-06-21 07:09:34]

https://codereview.chromium.org/1190413004/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/20001/Makefile.standalone#new...
Makefile.standalone:108: TEXTBC_DEFS = -DINPUT_IS_TEXTUAL_BITCODE
Do this like ALLOW_DUMP, i.e.
  -DINPUT_IS_TEXTUAL_BITCODE=1

https://codereview.chromium.org/1190413004/diff/20001/Makefile.standalone#new...
Makefile.standalone:112: TEXTBC_DEFS =
and
  -DINPUT_IS_TEXTUAL_BITCODE=0

Then in the code, use #if instead of #ifdef, or when possible, use
  if (INPUT_IS_TEXTUAL_BITCODE) {...}
so that as much code as possible gets compiled (but trivially dead-code
eliminated) under all configurations, to avoid rot.

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:18: #ifdef INPUT_IS_TEXTUAL_BITCODE
Can this be included unconditionally?  (maybe with a comment that it is only
needed for INPUT_IS_TEXTUAL_BITCODE)

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:38: #ifdef INPUT_IS_TEXTUAL_BITCODE
Would it be possible to define TextDataStreamer even when
INPUT_IS_TEXTUAL_BITCODE is undefined (or defined as 0)?

It's probably fine to still check the invalid combination of defines, but maybe
instead with a static assert?

static_assert(!(INPUT_IS_TEXTUAL_BITCODE && PNACL_BROWSER_TRANSLATOR),
              "Can not define ...");

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:153: #ifdef INPUT_IS_TEXTUAL_BITCODE
Hopefully you could use this instead?

INPUT_IS_TEXTUAL_BITCODE ? TextDataStreamer::create(...) :
llvm::getDataFileStreamer(...)

[Karl, 2015-06-22 19:52:02]

Cleaned up macros as requested.

https://codereview.chromium.org/1190413004/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/20001/Makefile.standalone#new...
Makefile.standalone:108: TEXTBC_DEFS = -DINPUT_IS_TEXTUAL_BITCODE
On 2015/06/21 07:09:34, stichnot wrote:
> Do this like ALLOW_DUMP, i.e.
>   -DINPUT_IS_TEXTUAL_BITCODE=1

Done.

https://codereview.chromium.org/1190413004/diff/20001/Makefile.standalone#new...
Makefile.standalone:112: TEXTBC_DEFS =
On 2015/06/21 07:09:34, stichnot wrote:
> and
>   -DINPUT_IS_TEXTUAL_BITCODE=0
> 
> Then in the code, use #if instead of #ifdef, or when possible, use
>   if (INPUT_IS_TEXTUAL_BITCODE) {...}
> so that as much code as possible gets compiled (but trivially dead-code
> eliminated) under all configurations, to avoid rot.

Done.

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:18: #ifdef INPUT_IS_TEXTUAL_BITCODE
On 2015/06/21 07:09:34, stichnot wrote:
> Can this be included unconditionally?  (maybe with a comment that it is only
> needed for INPUT_IS_TEXTUAL_BITCODE)

Done.

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:38: #ifdef INPUT_IS_TEXTUAL_BITCODE
On 2015/06/21 07:09:34, stichnot wrote:
> Would it be possible to define TextDataStreamer even when
> INPUT_IS_TEXTUAL_BITCODE is undefined (or defined as 0)?
> 
> It's probably fine to still check the invalid combination of defines, but
maybe
> instead with a static assert?
> 
> static_assert(!(INPUT_IS_TEXTUAL_BITCODE && PNACL_BROWSER_TRANSLATOR),
>               "Can not define ...");

Done.

https://codereview.chromium.org/1190413004/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:153: #ifdef INPUT_IS_TEXTUAL_BITCODE
On 2015/06/21 07:09:34, stichnot wrote:
> Hopefully you could use this instead?
> 
> INPUT_IS_TEXTUAL_BITCODE ? TextDataStreamer::create(...) :
> llvm::getDataFileStreamer(...)

Done.

[Jim Stichnoth, 2015-06-22 20:02:48]

otherwise lgtm

https://codereview.chromium.org/1190413004/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/40001/Makefile.standalone#new...
Makefile.standalone:238: # attributes were used to the build pnacl-sz.
"to the" ==> "to" ?

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:18: // Include code to handle converting textual bitcod
records to binary (for
bitcode

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:47: TextDataStreamer() {}
"() {}" ==> "() = default;" for ctor and dtor

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:74: if (Cursor + Len > BitcodeBuffer.size())
Might be clearer to do something like:
  size_t Remaining = BitcodeBuffer.size() - Cursor;
  Len = std::min(Len, Remaining);

(assuming I got it right...)

[Karl, 2015-06-22 20:20:27]

Committed patchset #4 (id:60001) manually as
6f9ba115f546d0e3446f08bd6f4477d2b5645bd5 (presubmit successful).

[Karl, 2015-06-22 20:20:37]

https://codereview.chromium.org/1190413004/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/40001/Makefile.standalone#new...
Makefile.standalone:238: # attributes were used to the build pnacl-sz.
On 2015/06/22 20:02:47, stichnot wrote:
> "to the" ==> "to" ?

Done.

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:18: // Include code to handle converting textual bitcod
records to binary (for
On 2015/06/22 20:02:47, stichnot wrote:
> bitcode

Done.

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:47: TextDataStreamer() {}
On 2015/06/22 20:02:47, stichnot wrote:
> "() {}" ==> "() = default;" for ctor and dtor

Done.

https://codereview.chromium.org/1190413004/diff/40001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:74: if (Cursor + Len > BitcodeBuffer.size())
On 2015/06/22 20:02:47, stichnot wrote:
> Might be clearer to do something like:
>   size_t Remaining = BitcodeBuffer.size() - Cursor;
>   Len = std::min(Len, Remaining);
> 
> (assuming I got it right...)

Done.

[jvoung (off chromium), 2015-06-23 22:17:36]

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone#new...
Makefile.standalone:92: TEXTBC_LIBS = -LLVMNaClBitTestUtils
Is this intended to be -LLVM... or should this be -lLLVM...?

Also, isn't it in the LLVM_LIBS_LIST below already? Or is there a link order
issue?

[Karl, 2015-06-23 22:49:52]

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone#new...
Makefile.standalone:92: TEXTBC_LIBS = -LLVMNaClBitTestUtils
On 2015/06/23 22:17:35, jvoung wrote:
> Is this intended to be -LLVM... or should this be -lLLVM...?
> 
> Also, isn't it in the LLVM_LIBS_LIST below already? Or is there a link order
> issue?

(1) I added it because it wasn't in LLVM_LIBS_LIST, and

(2) I didn't want to add it if you are building the sandboxed version of
pnacl-sz.

[jvoung (off chromium), 2015-06-24 01:30:59]

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone#new...
Makefile.standalone:92: TEXTBC_LIBS = -LLVMNaClBitTestUtils
On 2015/06/23 22:49:51, Karl wrote:
> On 2015/06/23 22:17:35, jvoung wrote:
> > Is this intended to be -LLVM... or should this be -lLLVM...?
> > 
> > Also, isn't it in the LLVM_LIBS_LIST below already? Or is there a link order
> > issue?
> 
> (1) I added it because it wasn't in LLVM_LIBS_LIST, and

But it is in LLVM_LIBS_LIST? Or am I reading this wrong?

The "-lLLVMNaClBitTestUtils" version is there (just not the
"-LLVMNaClBitTestUtils").


> 
> (2) I didn't want to add it if you are building the sandboxed version of
> pnacl-sz.

In that case, it shouldn't be in LLVM_LIBS_LIST then (if you agree with my reply
to (1))?

However, it should also be harmless for code size as long as the sandboxed
version doesn't reference code or data from that library, if code size is the
concern.

[Karl, 2015-06-24 15:11:50]

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1190413004/diff/60001/Makefile.standalone#new...
Makefile.standalone:92: TEXTBC_LIBS = -LLVMNaClBitTestUtils
On 2015/06/24 01:30:59, jvoung wrote:
> On 2015/06/23 22:49:51, Karl wrote:
> > On 2015/06/23 22:17:35, jvoung wrote:
> > > Is this intended to be -LLVM... or should this be -lLLVM...?
> > > 
> > > Also, isn't it in the LLVM_LIBS_LIST below already? Or is there a link
order
> > > issue?
> > 
> > (1) I added it because it wasn't in LLVM_LIBS_LIST, and
> 
> But it is in LLVM_LIBS_LIST? Or am I reading this wrong?
> 
> The "-lLLVMNaClBitTestUtils" version is there (just not the
> "-LLVMNaClBitTestUtils").
> 
> 
> > 
> > (2) I didn't want to add it if you are building the sandboxed version of
> > pnacl-sz.
> 
> In that case, it shouldn't be in LLVM_LIBS_LIST then (if you agree with my
reply
> to (1))?
> 
> However, it should also be harmless for code size as long as the sandboxed
> version doesn't reference code or data from that library, if code size is the
> concern.

You are right. I didn't see it. I must be blind! I did check using nm, and
nothing is included from that library if you compile just a minimal build. So my
assumption is it isn't a problem.

However, it is worth cleaning up the makefile, and will submit a CL to do this
later today.