Make sure that all globals are internal, except for "start" functions.

Make sure that all globals are internal, except for "start" functions.

The existing code, when run on a fuzzed example, generates a runtime
assertion. The reason for this is that the input defines "memmove" as
an external global. However, the code generator can generate calls to
"memmove" which assumes it is internal (see PNaCl ABI). As a result,
the assertion that checks that global names are unique (for memmove)
fails.

This code fixes the problem by checking that global names are
internal, unless they are one of the "start" functions,
or the function is an intrinsic. To allow for
non-PNaCl ABI input, a flag was added to allow functions to be
external. However, in such cases the external can't be one of
Subzero's runtime helper functions.

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

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

[Karl, 2015-10-06 19:20:13]

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

[John, 2015-10-06 19:30:15]

lgtm

https://chromiumcodereview.appspot.com/1387963002/diff/40001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://chromiumcodereview.appspot.com/1387963002/diff/40001/src/IceClFlags.h...
src/IceClFlags.h:248: bool AllowExternDefinedSymbols = false;
I have mixed feelings about this initialization here. while we do want to use it
whenever possible, nothing else uses it in this class. What about initializing
the member in the ctor, and leave a TODO() here for someone to fix all inits?

[Jim Stichnoth, 2015-10-06 19:45:37]

Not lgtm quite yet please.

I've only looked at one of the tests so far, but I'm not crazy about the fact
that everything has to be defined as "internal" now.  If I run "pnacl-sz
mytest.ll" manually, I won't get an error because LLVM is happily allowing
external, but if lit runs it through %p2i I do get an error.

https://chromiumcodereview.appspot.com/1387963002/diff/60001/tests_lit/llvm2i...
File tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll (right):

https://chromiumcodereview.appspot.com/1387963002/diff/60001/tests_lit/llvm2i...
tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll:53: define internal void
@test_memcpy_very_small_const_len(i32 %iptr_dst,
FWIW, I don't think we require 80-column max for the bitcode, apart from the RUN
lines.

[Karl, 2015-10-06 19:50:13]

On 2015/10/06 19:45:37, stichnot wrote:
> Not lgtm quite yet please.
> 
> I've only looked at one of the tests so far, but I'm not crazy about the fact
> that everything has to be defined as "internal" now.  If I run "pnacl-sz
> mytest.ll" manually, I won't get an error because LLVM is happily allowing
> external, but if lit runs it through %p2i I do get an error.
> 
>
https://chromiumcodereview.appspot.com/1387963002/diff/60001/tests_lit/llvm2i...
> File tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll (right):
> 
>
https://chromiumcodereview.appspot.com/1387963002/diff/60001/tests_lit/llvm2i...
> tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll:53: define internal void
> @test_memcpy_very_small_const_len(i32 %iptr_dst,
> FWIW, I don't think we require 80-column max for the bitcode, apart from the
RUN
> lines.

I forgot to fix the IceConverter, which is part of the problem. However,
we need to do something to deal with the fact that the PNaCl ABI requires
all definitions to be internal. The flag -allows-externally-defined-symbols
turns
off that check.

[Jim Stichnoth, 2015-10-06 20:16:21]

I think I agree that fixing IceConverter in the same way, would be the best fix
to the issue I raised.

https://chromiumcodereview.appspot.com/1387963002/diff/40001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://chromiumcodereview.appspot.com/1387963002/diff/40001/src/IceClFlags.h...
src/IceClFlags.h:248: bool AllowExternDefinedSymbols = false;
On 2015/10/06 19:30:15, John wrote:
> I have mixed feelings about this initialization here. while we do want to use
it
> whenever possible, nothing else uses it in this class. What about initializing
> the member in the ctor, and leave a TODO() here for someone to fix all inits?

I agree that there should be consistency here.

https://chromiumcodereview.appspot.com/1387963002/diff/80001/src/IceGlobalIni...
File src/IceGlobalInits.h (right):

https://chromiumcodereview.appspot.com/1387963002/diff/80001/src/IceGlobalIni...
src/IceGlobalInits.h:97: bool verifyLinkageDefault(GlobalContext *Ctx) const {
Here and below, maybe you can use "const GlobalContext *"?

https://chromiumcodereview.appspot.com/1387963002/diff/80001/src/IceGlobalIni...
src/IceGlobalInits.h:164: if (!hasName()) return false;
make format

[Karl, 2015-10-06 21:47:18]

https://chromiumcodereview.appspot.com/1387963002/diff/40001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://chromiumcodereview.appspot.com/1387963002/diff/40001/src/IceClFlags.h...
src/IceClFlags.h:248: bool AllowExternDefinedSymbols = false;
On 2015/10/06 19:30:15, John wrote:
> I have mixed feelings about this initialization here. while we do want to use
it
> whenever possible, nothing else uses it in this class. What about initializing
> the member in the ctor, and leave a TODO() here for someone to fix all inits?

Done.

https://chromiumcodereview.appspot.com/1387963002/diff/60001/tests_lit/llvm2i...
File tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll (right):

https://chromiumcodereview.appspot.com/1387963002/diff/60001/tests_lit/llvm2i...
tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll:53: define internal void
@test_memcpy_very_small_const_len(i32 %iptr_dst,
On 2015/10/06 19:45:36, stichnot wrote:
> FWIW, I don't think we require 80-column max for the bitcode, apart from the
RUN
> lines.

Acknowledged.

https://chromiumcodereview.appspot.com/1387963002/diff/80001/src/IceGlobalIni...
File src/IceGlobalInits.h (right):

https://chromiumcodereview.appspot.com/1387963002/diff/80001/src/IceGlobalIni...
src/IceGlobalInits.h:97: bool verifyLinkageDefault(GlobalContext *Ctx) const {
On 2015/10/06 20:16:21, stichnot wrote:
> Here and below, maybe you can use "const GlobalContext *"?

Done.

https://chromiumcodereview.appspot.com/1387963002/diff/80001/src/IceGlobalIni...
src/IceGlobalInits.h:164: if (!hasName()) return false;
On 2015/10/06 20:16:21, stichnot wrote:
> make format

Done.

[Jim Stichnoth, 2015-10-06 22:42:49]

LGTM, but you probably want to revert those formatting changes in
IceParseInstsTest.cpp .

[Karl, 2015-10-07 16:53:17]

Committed patchset #8 (id:140001) manually as
57d31ac73a35263d42357259329fa7d3e565731b (presubmit successful).