Subzero: Remove IceString.

Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion, especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking, even in a MINIMAL build.  In a MINIMAL build, the set of strings includes external symbol names, intrinsic names, helper function names, and label names of pooled constants to facilitate deterministic ELF string table output.  For now, it also includes jump table entry names until that gets sorted out.

Constants are fixed so that the name and pooled fields are properly immutable after the constant is fully initialized.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-03-27 20:53:32]

Description was changed from

==========
Subzero: Remove IceString.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
==========

to

==========
Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion,
especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now
explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally
in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and
pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking,
even in a MINIMAL build.

The StringID also includes a Cfg or GlobalContext pointer, so that the
underlying string can be set or fetched without having to plumb through the
pointer.  This could perhaps be improved later.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
==========

[Jim Stichnoth, 2016-03-27 21:08:07]

Description was changed from

==========
Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion,
especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now
explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally
in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and
pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking,
even in a MINIMAL build.

The StringID also includes a Cfg or GlobalContext pointer, so that the
underlying string can be set or fetched without having to plumb through the
pointer.  This could perhaps be improved later.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
==========

to

==========
Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion,
especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now
explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally
in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and
pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking,
even in a MINIMAL build.  In a MINIMAL build, the set of strings includes
external symbol names, intrinsic names, helper function names, and label names
of pooled constants to facilitate deterministic ELF string table output.  For
now, it also includes jump table entry names until that gets sorted out.

Constants are fixed so that the name and pooled fields are properly immutable
after the constant is fully initialized.

The StringID also includes a Cfg or GlobalContext pointer, so that the
underlying string can be set or fetched without having to plumb through the
pointer.  This could perhaps be improved later.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
==========

[Jim Stichnoth, 2016-03-28 22:30:03]

Description was changed from

==========
Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion,
especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now
explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally
in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and
pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking,
even in a MINIMAL build.  In a MINIMAL build, the set of strings includes
external symbol names, intrinsic names, helper function names, and label names
of pooled constants to facilitate deterministic ELF string table output.  For
now, it also includes jump table entry names until that gets sorted out.

Constants are fixed so that the name and pooled fields are properly immutable
after the constant is fully initialized.

The StringID also includes a Cfg or GlobalContext pointer, so that the
underlying string can be set or fetched without having to plumb through the
pointer.  This could perhaps be improved later.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
==========

to

==========
Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion,
especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now
explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally
in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and
pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking,
even in a MINIMAL build.  In a MINIMAL build, the set of strings includes
external symbol names, intrinsic names, helper function names, and label names
of pooled constants to facilitate deterministic ELF string table output.  For
now, it also includes jump table entry names until that gets sorted out.

Constants are fixed so that the name and pooled fields are properly immutable
after the constant is fully initialized.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
==========

[Jim Stichnoth, 2016-03-28 22:30:03]

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

[John, 2016-03-29 14:23:16]

lgtm

https://codereview.chromium.org/1838753002/diff/140001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceAssemblerARM32....
src/IceAssemblerARM32.cpp:596: const std::string Symbol = symbol().toString();
consider auto here -- toString() kinda seems to return just that, and I would be
baffled if it didn't.

https://codereview.chromium.org/1838753002/diff/140001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceCfg.h#newcode97
src/IceCfg.h:97: StringPool *getNodeStrings() const { return NodeStrings.get();
}
what's node strings? var strings?

https://codereview.chromium.org/1838753002/diff/140001/src/IceClFlags.h
File src/IceClFlags.h (left):

https://codereview.chromium.org/1838753002/diff/140001/src/IceClFlags.h#oldco...
src/IceClFlags.h:150: 
optional: add the empty line back

https://codereview.chromium.org/1838753002/diff/140001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceDefs.h#newcode359
src/IceDefs.h:359: template <typename T> class LockedPtr {
Consider documenting locked ptr.

https://codereview.chromium.org/1838753002/diff/140001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:79: const std::string NullSectionName = "";
these could all be constexpr char[]

https://codereview.chromium.org/1838753002/diff/140001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceGlobalContext.h...
src/IceGlobalContext.h:512: mutable GlobalLockType StringsLock;
I would either make all Locks mutable, or none Mutable. your call, though.

https://codereview.chromium.org/1838753002/diff/140001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:143: void initShouldBePooled();
maybe explain this?
should this be private?
Should it be a const bool?

https://codereview.chromium.org/1838753002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:355: #if 0
remove if 0

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h
File src/IceStringPool.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:46: assert(StringToId.find(Value) == StringToId.end());
why the assert?

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:52: void dump(Ostream &Str) {
const

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:57: for (auto &Tuple : StringToId) {
const auto &

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:77: StringID() = default;
I would declare ctors/assignment opers in the following order

StringID()
StringID(const StringID &) = default;
StringID &operator=(const StringID &) = default;

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:79: /// unique integral ID from the Owner.
The comment you added explain what the ctor does, but then, when you use the
class, we will see

StringID str(Ctx);

which is kinda meaningless, unless you're familiar with this code.

Consider using named constructors:

static StringID createUnbound(Owner) {}
static StringID createBound(Owner, Value) {}

sure, Bounc/Unbound are definitively not the best option, so feel free to rename
to something else. :)

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:89: static bool hasString(IDType ID) { return isValid(ID) &&
((ID & 0x1) == 0); }
I am suspicious of static methods as they are often a sign of code smell. In
this case, there are two issues with this static:
1) There's a non-static version of it; and
2) the non-static version is the only caller of it.

Maybe remove the static instance?

hasString() also fails to convey (to me) what the method really does. how about
s/hasString/representsStdString/?

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:97: assert(hasString());
an assert here looks wrong -- if somehow toString() is invoked without a string
the program will silently cast an odd number to a pointer.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:126: static bool isValid(IDType ID) { return ID !=
InvalidID; }
same thing for the static method here.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:130: struct GlobalStringPoolTraits {
I got to move this to IceDefs.h, but not to IceGlobalContext.h -- yet another
proof that IceDefs.h needs some **serious** refactoring. Ideally, IceDefs.h
would either die, or simply contain using foo = bar statements -- plus the llvm/
includes (and those should not appear anywhere else), and nothing else.

To accomplish this, you need to

/// IceDefs.h

#include "IceStringPool.h"

/* move the GlobalStringPoolTraits stuff there */

/// IceStringPool.h
#include "llvm/Support/raw_ostream.h"

#include <unordered_map> // please add this anyway -- include what you use!

and the s/Ostream/llvm::raw_ostream/g on this file (leave a TODO behind to
s/llvm::raw_ostream/Ostream/g once someone fixes IceDefs.h)

[Jim Stichnoth, 2016-03-29 21:40:50]

https://codereview.chromium.org/1838753002/diff/140001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceAssemblerARM32....
src/IceAssemblerARM32.cpp:596: const std::string Symbol = symbol().toString();
On 2016/03/29 14:23:15, John wrote:
> consider auto here -- toString() kinda seems to return just that, and I would
be
> baffled if it didn't.

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceCfg.h#newcode97
src/IceCfg.h:97: StringPool *getNodeStrings() const { return NodeStrings.get();
}
On 2016/03/29 14:23:15, John wrote:
> what's node strings? var strings?

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceClFlags.h
File src/IceClFlags.h (left):

https://codereview.chromium.org/1838753002/diff/140001/src/IceClFlags.h#oldco...
src/IceClFlags.h:150: 
On 2016/03/29 14:23:15, John wrote:
> optional: add the empty line back

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceDefs.h#newcode359
src/IceDefs.h:359: template <typename T> class LockedPtr {
On 2016/03/29 14:23:15, John wrote:
> Consider documenting locked ptr.

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:79: const std::string NullSectionName = "";
On 2016/03/29 14:23:15, John wrote:
> these could all be constexpr char[]

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceGlobalContext.h...
src/IceGlobalContext.h:512: mutable GlobalLockType StringsLock;
On 2016/03/29 14:23:15, John wrote:
> I would either make all Locks mutable, or none Mutable. your call, though.

Done - mutable in this file.

https://codereview.chromium.org/1838753002/diff/140001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:143: void initShouldBePooled();
On 2016/03/29 14:23:16, John wrote:
> maybe explain this?
> should this be private?
> Should it be a const bool?

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceOperand.h#newco...
src/IceOperand.h:355: #if 0
On 2016/03/29 14:23:16, John wrote:
> remove if 0

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h
File src/IceStringPool.h (right):

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:46: assert(StringToId.find(Value) == StringToId.end());
On 2016/03/29 14:23:16, John wrote:
> why the assert?

I think this was left over from an earlier implementation where the expression
wasn't guaranteed to be true.  Removed.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:52: void dump(Ostream &Str) {
On 2016/03/29 14:23:16, John wrote:
> const

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:57: for (auto &Tuple : StringToId) {
On 2016/03/29 14:23:16, John wrote:
> const auto &

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:77: StringID() = default;
On 2016/03/29 14:23:16, John wrote:
> I would declare ctors/assignment opers in the following order
> 
> StringID()
> StringID(const StringID &) = default;
> StringID &operator=(const StringID &) = default;

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:79: /// unique integral ID from the Owner.
On 2016/03/29 14:23:16, John wrote:
> The comment you added explain what the ctor does, but then, when you use the
> class, we will see
> 
> StringID str(Ctx);
> 
> which is kinda meaningless, unless you're familiar with this code.
> 
> Consider using named constructors:
> 
> static StringID createUnbound(Owner) {}
> static StringID createBound(Owner, Value) {}
> 
> sure, Bounc/Unbound are definitively not the best option, so feel free to
rename
> to something else. :)

Nice, done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:89: static bool hasString(IDType ID) { return isValid(ID) &&
((ID & 0x1) == 0); }
On 2016/03/29 14:23:16, John wrote:
> I am suspicious of static methods as they are often a sign of code smell. In
> this case, there are two issues with this static:
> 1) There's a non-static version of it; and
> 2) the non-static version is the only caller of it.
> 
> Maybe remove the static instance?
> 
> hasString() also fails to convey (to me) what the method really does. how
about
> s/hasString/representsStdString/?

Done - using hasStdString().

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:97: assert(hasString());
On 2016/03/29 14:23:16, John wrote:
> an assert here looks wrong -- if somehow toString() is invoked without a
string
> the program will silently cast an odd number to a pointer.

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:126: static bool isValid(IDType ID) { return ID !=
InvalidID; }
On 2016/03/29 14:23:16, John wrote:
> same thing for the static method here.

Done.

https://codereview.chromium.org/1838753002/diff/140001/src/IceStringPool.h#ne...
src/IceStringPool.h:130: struct GlobalStringPoolTraits {
On 2016/03/29 14:23:16, John wrote:
> I got to move this to IceDefs.h, but not to IceGlobalContext.h -- yet another
> proof that IceDefs.h needs some **serious** refactoring. Ideally, IceDefs.h
> would either die, or simply contain using foo = bar statements -- plus the
llvm/
> includes (and those should not appear anywhere else), and nothing else.
> 
> To accomplish this, you need to
> 
> /// IceDefs.h
> 
> #include "IceStringPool.h"
> 
> /* move the GlobalStringPoolTraits stuff there */
> 
> /// IceStringPool.h
> #include "llvm/Support/raw_ostream.h"
> 
> #include <unordered_map> // please add this anyway -- include what you use!
> 
> and the s/Ostream/llvm::raw_ostream/g on this file (leave a TODO behind to
> s/llvm::raw_ostream/Ostream/g once someone fixes IceDefs.h)

Left this alone, but added a TODO.

[Jim Stichnoth, 2016-03-29 22:01:11]

Description was changed from

==========
Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion,
especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now
explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally
in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and
pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking,
even in a MINIMAL build.  In a MINIMAL build, the set of strings includes
external symbol names, intrinsic names, helper function names, and label names
of pooled constants to facilitate deterministic ELF string table output.  For
now, it also includes jump table entry names until that gets sorted out.

Constants are fixed so that the name and pooled fields are properly immutable
after the constant is fully initialized.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
==========

to

==========
Subzero: Remove IceString.

The purpose is to get control over excess string creation and deletion,
especially when the strings are long enough to involve malloc/free.

Strings that interface with the outside world, or used for dump/debug, are now
explicitly std::string.

Variable names and node names are represented as string IDs and pooled locally
in the CFG.  (In a non-DUMP build, this pool should always be empty.)

Other strings that are used across functions are represented as string IDs and
pooled globally in the GlobalContext.

The --dump-strings flag allows these strings to be dumped for sanity checking,
even in a MINIMAL build.  In a MINIMAL build, the set of strings includes
external symbol names, intrinsic names, helper function names, and label names
of pooled constants to facilitate deterministic ELF string table output.  For
now, it also includes jump table entry names until that gets sorted out.

Constants are fixed so that the name and pooled fields are properly immutable
after the constant is fully initialized.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4360
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-03-29 22:01:13]

Committed patchset #9 (id:160001) manually as
467ffe51bebcb3ae3e3ce745c38fc20e8837c31c (presubmit successful).