PNaCl localmod mods in LLVM to 223109 (local files only)

PNaCl localmod mods in LLVM to 223109 (local files only)

git diff 51c1c423a9061345763425845c283b56caf4176b --summary | grep "create mode" | cut -f5 -d' ' | sort -u > files && depot_tools/third_party/upload.py master $(cat files) --server codereview.chromium.org

[Karl, 2015-02-20 17:07:01]

kschimpf@google.com changed reviewers:
+ kschimpf@google.com

[Karl, 2015-02-20 17:07:02]

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

https://codereview.chromium.org/940243003/diff/1/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:388: assert(BytesRead ==
sizeof(Array));
Should this be an assert (i.e. has the length been checked elsewhere to be
divisible by sizeof(word_t))? What if the input doesn't match this requirement?

https://codereview.chromium.org/940243003/diff/1/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:409: CurWord >>= (NumBits &
0x1f);
Is the mask necessary? Doesn't the assert above guarantee this?

I guess you are adding this in case asserts are turned off?

https://codereview.chromium.org/940243003/diff/1/lib/Bitcode/NaCl/TestUtils/N...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp (right):

https://codereview.chromium.org/940243003/diff/1/lib/Bitcode/NaCl/TestUtils/N...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp:351: // like the parser?
Probably. File an issue and I will work on it.

[jvoung (off chromium), 2015-02-20 17:38:21]

Thanks Karl, looks like I missed some diffs between the plain BitstreamReader
and the NaCl one -- will re-upload in a bit

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

https://codereview.chromium.org/940243003/diff/1/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:388: assert(BytesRead ==
sizeof(Array));
On 2015/02/20 17:07:02, Karl wrote:
> Should this be an assert (i.e. has the length been checked elsewhere to be
> divisible by sizeof(word_t))? What if the input doesn't match this
requirement?

Hmm looks like the assert is gone in the new BitstreamReader and I forgot to
copy that back to NaClBitstreamReader. Let me check the diffs and reupload.

https://codereview.chromium.org/940243003/diff/1/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:409: CurWord >>= (NumBits &
0x1f);
On 2015/02/20 17:07:02, Karl wrote:
> Is the mask necessary? Doesn't the assert above guarantee this?
> 
> I guess you are adding this in case asserts are turned off?

Yes, in case asserts are turned off.

Otherwise it's also to match what the non-NaClBitstreamReader looks like.

Actually, hold on, I think there are more changes to the upstream
BitStreamReader that I will check if it makes sense to copy to
NaClBitstreamReader. E.g., when sizeof(word_t) is 64-bit it may be able to read
64-bits at a time instead, and the mask could end up 0x3f. I guess for the
pnacl-llc.nexe self-build. I'll upload a new patch and let me know if it makes
sense.

https://codereview.chromium.org/940243003/diff/1/lib/Bitcode/NaCl/TestUtils/N...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp (right):

https://codereview.chromium.org/940243003/diff/1/lib/Bitcode/NaCl/TestUtils/N...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp:351: // like the parser?
On 2015/02/20 17:07:02, Karl wrote:
> Probably. File an issue and I will work on it.

Okay thanks, filed:

https://code.google.com/p/nativeclient/issues/detail?id=4103

[jvoung (off chromium), 2015-02-21 01:02:12]

Merged some more changes from BitstreamReader -> NaClBitstreamReader, and also
had to change ThreadedStreamingCache some.

isObjectEnd() is gone, so readBytes must now indicate EOF, which I rebased in
the merge incorrectly before. That made the trailing data check you added fail
because the bitstream couldn't find the right EOF for a pexe/pso that is smaller
than the ThreadedStreamingCache's "cache line".

[jvoung (off chromium), 2015-02-21 01:51:32]

Hmm... patch set 2/3 seems to have introduced flaky deadlock in the sandboxed
version now =(

[Karl, 2015-02-23 17:46:55]

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:220: /// is always from
[0...bits_of(size_t)-1] inclusive.
word_t rather than size_t?

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:273: return Size == NextChar;
What if NextChar is at the beginning of the file, and Size is initialized to
zero (since size is not known yet)?

https://codereview.chromium.org/940243003/diff/40001/tools/pnacl-llc/Threaded...
File tools/pnacl-llc/ThreadedStreamingCache.cpp (right):

https://codereview.chromium.org/940243003/diff/40001/tools/pnacl-llc/Threaded...
tools/pnacl-llc/ThreadedStreamingCache.cpp:46: MinObjectSize = End;
Is this correct? Shouldn't it be relative to Base?

[jvoung (off chromium), 2015-02-23 20:25:53]

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

[jvoung (off chromium), 2015-02-23 20:26:57]

On 2015/02/21 01:51:32, jvoung wrote:
> Hmm... patch set 2/3 seems to have introduced flaky deadlock in the sandboxed
> version now =(

Looks like the deadlock has been happening on the bots for a while!

http://build.chromium.org/p/client.nacl.toolchain/builders/linux-pnacl-x86_32...

I looked at the new nacl pthread_rwlocks and looks like it has a problem when
using > 2 threads, it may fail to wake a reader thread.

Going to file a bug and disable that for now.

[Karl, 2015-02-23 21:04:38]

Once nits fixed in early message, LGTM.

[jvoung (off chromium), 2015-02-23 21:27:19]

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:220: /// is always from
[0...bits_of(size_t)-1] inclusive.
On 2015/02/23 17:46:55, Karl wrote:
> word_t rather than size_t?

Done.

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:273: return Size == NextChar;
On 2015/02/23 17:46:55, Karl wrote:
> What if NextChar is at the beginning of the file, and Size is initialized to
> zero (since size is not known yet)?

There is a check that Size != 0 first, so this branch would be skipped if size
is still unknown?

Or do you mean what if the NextChar is at the beginning of the file, and that is
truly the end of the stream already? Or some other case?

https://codereview.chromium.org/940243003/diff/40001/tools/pnacl-llc/Threaded...
File tools/pnacl-llc/ThreadedStreamingCache.cpp (right):

https://codereview.chromium.org/940243003/diff/40001/tools/pnacl-llc/Threaded...
tools/pnacl-llc/ThreadedStreamingCache.cpp:46: MinObjectSize = End;
On 2015/02/23 17:46:55, Karl wrote:
> Is this correct? Shouldn't it be relative to Base?

The: "Base = Address & kCacheSizeMask"?

As far as I know, MinObjectSize is relative to the full stream, not just the
cacheline? I think this is okay, but dschuff can confirm.

[Karl, 2015-02-23 21:40:27]

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/940243003/diff/40001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:273: return Size == NextChar;
On 2015/02/23 21:27:19, jvoung wrote:
> On 2015/02/23 17:46:55, Karl wrote:
> > What if NextChar is at the beginning of the file, and Size is initialized to
> > zero (since size is not known yet)?
> 
> There is a check that Size != 0 first, so this branch would be skipped if size
> is still unknown?
> 
> Or do you mean what if the NextChar is at the beginning of the file, and that
is
> truly the end of the stream already? Or some other case?

I meant if NextChar is at the beginning of the file, and Size == 0. This is two
cases: (1) We are already at the end of the stream, or (2) Size has not yet been
set (see comment on field Size).

For case (2) Size will be set something different than zero iff fillCurWord has
already been called. If it hasn't, how does Size become non-zero if the
constructor set it to zero.

https://codereview.chromium.org/940243003/diff/40001/tools/pnacl-llc/Threaded...
File tools/pnacl-llc/ThreadedStreamingCache.cpp (right):

https://codereview.chromium.org/940243003/diff/40001/tools/pnacl-llc/Threaded...
tools/pnacl-llc/ThreadedStreamingCache.cpp:46: MinObjectSize = End;
On 2015/02/23 21:27:19, jvoung wrote:
> On 2015/02/23 17:46:55, Karl wrote:
> > Is this correct? Shouldn't it be relative to Base?
> 
> The: "Base = Address & kCacheSizeMask"?
> 
> As far as I know, MinObjectSize is relative to the full stream, not just the
> cacheline? I think this is okay, but dschuff can confirm.
> 

Ok. I see. I didn't realize that MinObjectSize was how much it has now read (I
would have not guessed that from its name).

[JF, 2015-02-24 05:35:37]

https://codereview.chromium.org/940243003/diff/60001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right):

https://codereview.chromium.org/940243003/diff/60001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:62: ErrorOr<Module *>
getNaClLazyBitcodeModule(std::unique_ptr<MemoryBuffer> &&Buffer,
Linewrap.

Does the function take ownership of the Buf? r-value tells me yes, but doc above
says no.

https://codereview.chromium.org/940243003/diff/60001/lib/Bitcode/NaCl/Reader/...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h (right):

https://codereview.chromium.org/940243003/diff/60001/lib/Bitcode/NaCl/Reader/...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:239: void releaseBuffer();
Not virtual anymore?

https://codereview.chromium.org/940243003/diff/60001/test/NaCl/PNaClABI/abi-d...
File test/NaCl/PNaClABI/abi-debug-info.ll (right):

https://codereview.chromium.org/940243003/diff/60001/test/NaCl/PNaClABI/abi-d...
test/NaCl/PNaClABI/abi-debug-info.ll:9: !0 = metadata !{i32 1, metadata !"Debug
Info Version", i32 2}
Hmm, so this update in debug info version means that when we check 3.6 in
debugging won't work unless the Chrome version is also updated. It's worth
pointing out in the release notes I think.

https://codereview.chromium.org/940243003/diff/60001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/940243003/diff/60001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:320: static Module* getModule(StringRef
ProgramName, LLVMContext &Context,
This could now be a unique_ptr. One of the two uses is a bit weird (because of
the GlobalModule)?

[jvoung (off chromium), 2015-02-24 18:38:48]

https://codereview.chromium.org/940243003/diff/60001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right):

https://codereview.chromium.org/940243003/diff/60001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:46: /// getNaClLazyBitcodeModule -
Read the header of the specified bitcode buffer
New style seems to be to avoid repeating the function name in comments, so I
also went and deleted them.

https://codereview.chromium.org/940243003/diff/60001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:62: ErrorOr<Module *>
getNaClLazyBitcodeModule(std::unique_ptr<MemoryBuffer> &&Buffer,
On 2015/02/24 05:35:36, JF wrote:
> Linewrap.
> 
> Does the function take ownership of the Buf? r-value tells me yes, but doc
above
> says no.

It is conditional/complicated =).  See above comment (before When Verbose is ...
etc.) -- on success it "takes ownership" of the underlying MemoryBuffer and
extends the lifetime and on error it does not extend the lifetime, but since the
unique_ptr goes out of scope I guess it would delete.

Linewrapped.

https://codereview.chromium.org/940243003/diff/60001/lib/Bitcode/NaCl/Reader/...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h (right):

https://codereview.chromium.org/940243003/diff/60001/lib/Bitcode/NaCl/Reader/...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:239: void releaseBuffer();
On 2015/02/24 05:35:37, JF wrote:
> Not virtual anymore?

Yes, not virtual anymore. There's fancy new clang warning to catch
inconsistent/missing overrides =)

https://codereview.chromium.org/940243003/diff/60001/test/NaCl/PNaClABI/abi-d...
File test/NaCl/PNaClABI/abi-debug-info.ll (right):

https://codereview.chromium.org/940243003/diff/60001/test/NaCl/PNaClABI/abi-d...
test/NaCl/PNaClABI/abi-debug-info.ll:9: !0 = metadata !{i32 1, metadata !"Debug
Info Version", i32 2}
On 2015/02/24 05:35:37, JF wrote:
> Hmm, so this update in debug info version means that when we check 3.6 in
> debugging won't work unless the Chrome version is also updated. It's worth
> pointing out in the release notes I think.

Yes good point.

https://codereview.chromium.org/940243003/diff/60001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/940243003/diff/60001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:320: static Module* getModule(StringRef
ProgramName, LLVMContext &Context,
On 2015/02/24 05:35:37, JF wrote:
> This could now be a unique_ptr. One of the two uses is a bit weird (because of
> the GlobalModule)?

Converted the function to return a unique_ptr.

The GlobalModule does make things a bit weird in one function where a reference
is borrowed. It's mainly used for thread 0 vs thread 1-n.

Maybe in a later CL we can try allocating all of the modules and contexts up
front before threads are created, and then every thread is uniform. I can't
imagine the extra parallelism in the allocation of module/context is that big?

https://codereview.chromium.org/940243003/diff/90001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/940243003/diff/90001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:374: static int runCompilePasses(Module
*ModuleRef,
Went ahead and capitalized the variable name for consistency.

[JF, 2015-02-24 19:32:55]

lgtm

https://codereview.chromium.org/940243003/diff/60001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right):

https://codereview.chromium.org/940243003/diff/60001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:62: ErrorOr<Module *>
getNaClLazyBitcodeModule(std::unique_ptr<MemoryBuffer> &&Buffer,
On 2015/02/24 18:38:48, jvoung wrote:
> On 2015/02/24 05:35:36, JF wrote:
> > Linewrap.
> > 
> > Does the function take ownership of the Buf? r-value tells me yes, but doc
> above
> > says no.
> 
> It is conditional/complicated =).  See above comment (before When Verbose is
...
> etc.) -- on success it "takes ownership" of the underlying MemoryBuffer and
> extends the lifetime and on error it does not extend the lifetime, but since
the
> unique_ptr goes out of scope I guess it would delete.

That sounds like ownership :-p
The updated documentation clarifies this well.

Is that just because of LLVM's current odd handling of errors?

https://codereview.chromium.org/940243003/diff/60001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/940243003/diff/60001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:320: static Module* getModule(StringRef
ProgramName, LLVMContext &Context,
On 2015/02/24 18:38:48, jvoung wrote:
> On 2015/02/24 05:35:37, JF wrote:
> > This could now be a unique_ptr. One of the two uses is a bit weird (because
of
> > the GlobalModule)?
> 
> Converted the function to return a unique_ptr.
> 
> The GlobalModule does make things a bit weird in one function where a
reference
> is borrowed. It's mainly used for thread 0 vs thread 1-n.
> 
> Maybe in a later CL we can try allocating all of the modules and contexts up
> front before threads are created, and then every thread is uniform. I can't
> imagine the extra parallelism in the allocation of module/context is that big?

sgtm

[Karl, 2015-02-24 20:06:17]

lgtm