Allow ability to name unnamed global addresses in Subzero.

Allow ability to name unnamed global addresses in Subzero.

This is a workaround for issue that Subzero currently assumes all
global addresses have a name, but finalized pexe files leave most
global addresses unnamed.

It does this by allowing two optional command-line flag name prefixes
that are used to generate names for unnamed global addresses.

BUG= None
R=stichnot@chromium.org

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

[Karl, 2014-09-11 21:13:57]

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

[Jim Stichnoth, 2014-09-11 22:08:19]

https://codereview.chromium.org/567703003/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/567703003/diff/20001/src/IceClFlags.h#newcode32
src/IceClFlags.h:32: std::string DefaultGlobalPrefix;
1. I originally thought ClFlags was just for bool flags.  I think it's fine to
add non-bool stuff, but could you add a TODO(stichnot) to move even more into
ClFlags?

2. Do you want to commit to std::string for the type, or is it reasonable to use
IceString?  In either case, you need to #include "IceDefs.h" in this file.

https://codereview.chromium.org/567703003/diff/20001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/567703003/diff/20001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:37: llvm::outs() << "Warning: Default global prefix '" <<
Prefix
Can you use Translator::getGlobalContext()->getStrDump() instead of
llvm::outs()?  That way all the messages are sure to go to the same place.

https://codereview.chromium.org/567703003/diff/20001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/567703003/diff/20001/src/llvm2ice.cpp#newcode115
src/llvm2ice.cpp:115: "unnamed globals/funtions"),
functions

https://codereview.chromium.org/567703003/diff/20001/tests_lit/reader_tests/u...
File tests_lit/reader_tests/unnamed.ll (right):

https://codereview.chromium.org/567703003/diff/20001/tests_lit/reader_tests/u...
tests_lit/reader_tests/unnamed.ll:5: ; RUN:               
-default-global-prefix=__g -allow-pnacl-reader-error-recovery \
80-char, here and line 10

https://codereview.chromium.org/567703003/diff/20001/tests_lit/reader_tests/u...
tests_lit/reader_tests/unnamed.ll:11: ; RUN:        2>& 1 | FileCheck
--check-prefix=BAD %s
Currently the warning message is going to llvm::outs() i.e. stdout not stderr,
so is there a reason for redirecting stderr?

[jvoung (off chromium), 2014-09-11 22:16:43]

Thanks, but one question --

https://codereview.chromium.org/567703003/diff/20001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/567703003/diff/20001/src/llvm2ice.cpp#newcode116
src/llvm2ice.cpp:116: cl::init(""));
Should this have a default?

At least llc has a default of just "unnamed" (and pnacl-llc has a default of
"Functions" and "Globals"), with no option to change it.

Typically a pexe either has symbols for everything or no symbols at all (except
_start and the intrinsics), so any choice of a default prefix shouldn't actually
conflict?

[Karl, 2014-09-12 16:29:06]

https://codereview.chromium.org/567703003/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/567703003/diff/20001/src/IceClFlags.h#newcode32
src/IceClFlags.h:32: std::string DefaultGlobalPrefix;
On 2014/09/11 22:08:19, stichnot wrote:
> 1. I originally thought ClFlags was just for bool flags.  I think it's fine to
> add non-bool stuff, but could you add a TODO(stichnot) to move even more into
> ClFlags?
> 
> 2. Do you want to commit to std::string for the type, or is it reasonable to
use
> IceString?  In either case, you need to #include "IceDefs.h" in this file.

Both changes done.

https://codereview.chromium.org/567703003/diff/20001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/567703003/diff/20001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:37: llvm::outs() << "Warning: Default global prefix '" <<
Prefix
On 2014/09/11 22:08:19, stichnot wrote:
> Can you use Translator::getGlobalContext()->getStrDump() instead of
> llvm::outs()?  That way all the messages are sure to go to the same place.

Done.

https://codereview.chromium.org/567703003/diff/20001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/567703003/diff/20001/src/llvm2ice.cpp#newcode115
src/llvm2ice.cpp:115: "unnamed globals/funtions"),
On 2014/09/11 22:08:19, stichnot wrote:
> functions

Done.

https://codereview.chromium.org/567703003/diff/20001/src/llvm2ice.cpp#newcode116
src/llvm2ice.cpp:116: cl::init(""));
On 2014/09/11 22:16:43, jvoung wrote:
> Should this have a default?
> 
> At least llc has a default of just "unnamed" (and pnacl-llc has a default of
> "Functions" and "Globals"), with no option to change it.
> 
> Typically a pexe either has symbols for everything or no symbols at all
(except
> _start and the intrinsics), so any choice of a default prefix shouldn't
actually
> conflict?

Good point. Changing default for globals to "Global" and functions "Function".

https://codereview.chromium.org/567703003/diff/20001/tests_lit/reader_tests/u...
File tests_lit/reader_tests/unnamed.ll (right):

https://codereview.chromium.org/567703003/diff/20001/tests_lit/reader_tests/u...
tests_lit/reader_tests/unnamed.ll:11: ; RUN:        2>& 1 | FileCheck
--check-prefix=BAD %s
On 2014/09/11 22:08:19, stichnot wrote:
> Currently the warning message is going to llvm::outs() i.e. stdout not stderr,
> so is there a reason for redirecting stderr?

Not anymore. Fixing.

[Jim Stichnoth, 2014-09-12 17:20:09]

LGTM with some nits.

You should tweak the commit message since there are now two prefix-related
command-line options instead of one.

Also, in light of your recent look at pnacl-llc, is there any hope of
successfully reproducing its unnamed symbol naming?
Function
Function1
Function2
...
Function<k>
_start
Function<k+1>
...
Function<n>
Global
Global<n+1>
Global<n+2>
...

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:30: static inline void setValueName(llvm::Value *V, const
char *Kind,
Don't use static.  And can you omit inline?

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:31: const std::string &Prefix, uint32_t &NameIndex,
IceString

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:32: Ice::Ostream &errs) {
Remove Ice:: everywhere in this file (or maybe better, remove "using namespace
Ice;").

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:47: }
// end of anonymous namespace

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:50: const std::string &GlobalPrefix =
Flags.DefaultGlobalPrefix;
IceString

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:60: const std::string &FunctionPrefix =
Flags.DefaultFunctionPrefix;
IceString

[Karl, 2014-09-12 17:41:48]

Committed patchset #5 (id:80001) manually as 5ee234a (presubmit successful).

[Karl, 2014-09-12 17:41:59]

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:30: static inline void setValueName(llvm::Value *V, const
char *Kind,
On 2014/09/12 17:20:09, stichnot wrote:
> Don't use static.  And can you omit inline?

Done.

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:31: const std::string &Prefix, uint32_t &NameIndex,
On 2014/09/12 17:20:09, stichnot wrote:
> IceString

Done.

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:32: Ice::Ostream &errs) {
On 2014/09/12 17:20:09, stichnot wrote:
> Remove Ice:: everywhere in this file (or maybe better, remove "using namespace
> Ice;").

Done.

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:40: }
Added check if NameIndex == 0, then use prefix as name instead so that we are
more like pnacl-llc.

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:47: }
On 2014/09/12 17:20:09, stichnot wrote:
> // end of anonymous namespace

Done.

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:50: const std::string &GlobalPrefix =
Flags.DefaultGlobalPrefix;
On 2014/09/12 17:20:09, stichnot wrote:
> IceString

Done.

https://codereview.chromium.org/567703003/diff/60001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:60: const std::string &FunctionPrefix =
Flags.DefaultFunctionPrefix;
On 2014/09/12 17:20:09, stichnot wrote:
> IceString

Done.