Subzero: Fix symbol name mangling. Make flags global.

Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is done based on logic in the parser instead of scattered all over the code base.  Another nice side effect is that it allows some getName() style functions to return a const string reference instead of a string copy.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global, so that these constant flags can be accessed without having to plumb a GlobalContext object.  Note that some of the ClFlags and ClFlagsExtra plumbing in the compile server classes could be simplified to directly use the corresponding static fields, but this is left mostly as is for now, for when we do a proper separation between core and supplemental flags.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-03-06 17:49:37]

Description was changed from

==========
Subzero: Fix symbol name mangling. Make flags global.

BUG= none
==========

to

==========
Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is
done based on logic in the parser.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global,
so that these constant flags can be accessed without having to plumb a
GlobalContext object.

BUG= none
==========

[Jim Stichnoth, 2016-03-06 18:51:01]

Description was changed from

==========
Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is
done based on logic in the parser.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global,
so that these constant flags can be accessed without having to plumb a
GlobalContext object.

BUG= none
==========

to

==========
Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is
done based on logic in the parser instead of scattered all over the code base. 
Another nice side effect is that it allows some getName() style functions to
return a const string reference instead of a string copy.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global,
so that these constant flags can be accessed without having to plumb a
GlobalContext object.

BUG= none
==========

[Jim Stichnoth, 2016-03-06 20:02:32]

Description was changed from

==========
Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is
done based on logic in the parser instead of scattered all over the code base. 
Another nice side effect is that it allows some getName() style functions to
return a const string reference instead of a string copy.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global,
so that these constant flags can be accessed without having to plumb a
GlobalContext object.

BUG= none
==========

to

==========
Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is
done based on logic in the parser instead of scattered all over the code base. 
Another nice side effect is that it allows some getName() style functions to
return a const string reference instead of a string copy.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global,
so that these constant flags can be accessed without having to plumb a
GlobalContext object.  Note that some of the ClFlags and ClFlagsExtra plumbing
in the compile server classes could be simplified to directly use the
corresponding static fields, but this is left mostly as is for now, for when we
do a proper separation between core and supplemental flags.

BUG= none
==========

[Jim Stichnoth, 2016-03-06 20:16:44]

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

[John, 2016-03-06 22:39:38]

lgtm

I would strongly recommend the flags to be global objects, and not class
members.

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

https://codereview.chromium.org/1766233002/diff/20001/Makefile.standalone#new...
Makefile.standalone:542: +make -f Makefile.standalone \
Unrelated change??

https://codereview.chromium.org/1766233002/diff/20001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.h:108: ClFlagsExtra &ExtraFlags;
Could these be const? Otherwise, make them pointers?

https://codereview.chromium.org/1766233002/diff/20001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceCfg.h#newcode78
src/IceCfg.h:78: const IceString &getError() const { return ErrorMessage; }
I understand this change, but consider this carefully. String references might
bite you are careless - especially with auto

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

https://codereview.chromium.org/1766233002/diff/20001/src/IceClFlags.h#newcode34
src/IceClFlags.h:34: // optional flags might as well be global, i.e. static
members of GlobalContext,
If they are global, please make them, uh, global!

https://codereview.chromium.org/1766233002/diff/20001/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceELFSection.h#new...
src/IceELFSection.h:358: const IceString Name = Fixup.symbol(Asm);
Are you missing a reference here?

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:1029: ClFlagsExtra GlobalContext::ExtraFlags;
Do these have constructors?

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:188: static IceString mangleName(const IceString &Name);
This declaration "smells". There's really no good reason for this function to be
a class member, is there? Consider make it global -- because it really is a
global. :)

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:225: static const ClFlags &getFlags() { return Flags; }
Ditto.

https://codereview.chromium.org/1766233002/diff/20001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:239: const IceString NameSuffix =
Name.substr(LLVMPrefixLen);
Reference?

https://codereview.chromium.org/1766233002/diff/20001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceOperand.h#newcod...
src/IceOperand.h:651: return "lv$" + getName(Func);
Return "" is clearer, and should be the same.

[Jim Stichnoth, 2016-03-07 00:03:11]

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

https://codereview.chromium.org/1766233002/diff/20001/Makefile.standalone#new...
Makefile.standalone:542: +make -f Makefile.standalone \
On 2016/03/06 22:39:38, John wrote:
> Unrelated change??

Yes - the "sb" build target is already tested in the DEBUG=1 configuration
above, I guess I forgot to remove it from here before.

https://codereview.chromium.org/1766233002/diff/20001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.h:108: ClFlagsExtra &ExtraFlags;
On 2016/03/06 22:39:38, John wrote:
> Could these be const? Otherwise, make them pointers?

Done.

https://codereview.chromium.org/1766233002/diff/20001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceCfg.h#newcode78
src/IceCfg.h:78: const IceString &getError() const { return ErrorMessage; }
On 2016/03/06 22:39:38, John wrote:
> I understand this change, but consider this carefully. String references might
> bite you are careless - especially with auto

You're right, no sense doing risks to optimize getting an error message.  
Reverted.

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

https://codereview.chromium.org/1766233002/diff/20001/src/IceClFlags.h#newcode34
src/IceClFlags.h:34: // optional flags might as well be global, i.e. static
members of GlobalContext,
On 2016/03/06 22:39:38, John wrote:
> If they are global, please make them, uh, global!

Do you mean just pull them out of the Ice::GlobalContext namespace into the Ice
namespace?

In any case, I was hoping to defer the full cleanup of flags into a separate CL.
 The idea is to keep actual parameters to the browser translation in a
GlobalContext specific object, and make the rest global.  The core parameters
are just optlevel, numthreads, and output file.  In the browser build, those
would have a simple command line parser rather than using llvm::cl, and all the
others would be hard-coded.  This would reduce the size of the translator by
maybe 50-100KB.  Since the core parameters would use the same non-global
plumbing as the existing flags, I kept that plumbing reasonably intact to make
the implementation easier.

https://codereview.chromium.org/1766233002/diff/20001/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceELFSection.h#new...
src/IceELFSection.h:358: const IceString Name = Fixup.symbol(Asm);
On 2016/03/06 22:39:38, John wrote:
> Are you missing a reference here?

You mean like "const IceString &Name"?  The symbol() method constructs a new
string on the stack and returns it, so a reference won't do.

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:1029: ClFlagsExtra GlobalContext::ExtraFlags;
On 2016/03/06 22:39:38, John wrote:
> Do these have constructors?

Yes - and they get run as static initializers.  I guess that's not great, but
something I'd like to fix in a follow-on CL.

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:188: static IceString mangleName(const IceString &Name);
On 2016/03/06 22:39:38, John wrote:
> This declaration "smells". There's really no good reason for this function to
be
> a class member, is there? Consider make it global -- because it really is a
> global. :)

Done - moved into new file IceMangling.cpp.

https://codereview.chromium.org/1766233002/diff/20001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:225: static const ClFlags &getFlags() { return Flags; }
On 2016/03/06 22:39:38, John wrote:
> Ditto.

I assume this is about flags being global, in which case my related comments
should apply.

https://codereview.chromium.org/1766233002/diff/20001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:239: const IceString NameSuffix =
Name.substr(LLVMPrefixLen);
On 2016/03/06 22:39:38, John wrote:
> Reference?

std::string::substr() returns a newly constructed string object, so no?

I just added const here (and other places) for general const goodness, i.e.
NameSuffix is computed once and won't change thereafter.

https://codereview.chromium.org/1766233002/diff/20001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceOperand.h#newcod...
src/IceOperand.h:651: return "lv$" + getName(Func);
On 2016/03/06 22:39:38, John wrote:
> Return "" is clearer, and should be the same.

Done - here and in IceInstARM32.cpp and IceInstMIPS32.cpp.

[John, 2016-03-07 16:41:08]

lgtm

Another thing to keep in mind: contemporary c++ compilers will optimize

Doh d = Hommer.think();

to something like

Allocate, but do not construct, Doh d;
Hommer.think(d);

because Named Return Value Optimization (NRVO), so no copy is happening behind
the scenes.

I would also assume string copies **not** to be a problem for sz at the momment,
so I would not early optimize methods/functions to return string references.

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

https://codereview.chromium.org/1766233002/diff/20001/src/IceClFlags.h#newcode34
src/IceClFlags.h:34: // optional flags might as well be global, i.e. static
members of GlobalContext,
On 2016/03/07 00:03:10, stichnot wrote:
> On 2016/03/06 22:39:38, John wrote:
> > If they are global, please make them, uh, global!
> 
> Do you mean just pull them out of the Ice::GlobalContext namespace into the
Ice
> namespace?
> 
> In any case, I was hoping to defer the full cleanup of flags into a separate
CL.
>  The idea is to keep actual parameters to the browser translation in a
> GlobalContext specific object, and make the rest global.  The core parameters
> are just optlevel, numthreads, and output file.  In the browser build, those
> would have a simple command line parser rather than using llvm::cl, and all
the
> others would be hard-coded.  This would reduce the size of the translator by
> maybe 50-100KB.  Since the core parameters would use the same non-global
> plumbing as the existing flags, I kept that plumbing reasonably intact to make
> the implementation easier.

Yes, just move it to the Ice namespace. There's very little value added in
having static members like these. In general, this kind of construct "smells"
like java -- which lacks globals.

If you are "worried" about "naked" uses of these member, you could move their
declaration to some cpp file, and just expose the getters -- which have no real
reason to be class members.

https://codereview.chromium.org/1766233002/diff/20001/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceELFSection.h#new...
src/IceELFSection.h:358: const IceString Name = Fixup.symbol(Asm);
On 2016/03/07 00:03:10, stichnot wrote:
> On 2016/03/06 22:39:38, John wrote:
> > Are you missing a reference here?
> 
> You mean like "const IceString &Name"?  The symbol() method constructs a new
> string on the stack and returns it, so a reference won't do.

And that's why I would standardize into **never** returning string references.
It is weird that some methods return references while others return new objects.

Note. C++ guarantees that the returned object will be alive until the reference
to it goes out of scope -- it won't dangle!

#include <iostream>
#include <memory>

class Foo {
public:
  Foo(const Foo &) = default;
  ~Foo() { std::cerr << "Foo::~Foo()" "\n"; }
  static Foo create() { return Foo(); }

private:
  Foo() { std::cerr << "Foo::Foo()" "\n"; }
};

int main(int argc, char* argv[]) {
  const Foo &F = Foo::create();
  std::cerr << "before return." "\n";
  return 0;
}

prints:

Foo::Foo()
before return.
Foo::~Foo()

https://codereview.chromium.org/1766233002/diff/20001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1766233002/diff/20001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:239: const IceString NameSuffix =
Name.substr(LLVMPrefixLen);
On 2016/03/07 00:03:10, stichnot wrote:
> On 2016/03/06 22:39:38, John wrote:
> > Reference?
> 
> std::string::substr() returns a newly constructed string object, so no?
> 
> I just added const here (and other places) for general const goodness, i.e.
> NameSuffix is computed once and won't change thereafter.

same comment about const reference. :)

[Jim Stichnoth, 2016-03-07 17:26:31]

Description was changed from

==========
Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is
done based on logic in the parser instead of scattered all over the code base. 
Another nice side effect is that it allows some getName() style functions to
return a const string reference instead of a string copy.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global,
so that these constant flags can be accessed without having to plumb a
GlobalContext object.  Note that some of the ClFlags and ClFlagsExtra plumbing
in the compile server classes could be simplified to directly use the
corresponding static fields, but this is left mostly as is for now, for when we
do a proper separation between core and supplemental flags.

BUG= none
==========

to

==========
Subzero: Fix symbol name mangling. Make flags global.

This cleans up the whole --prefix handling mechanism, so that all mangling is
done based on logic in the parser instead of scattered all over the code base. 
Another nice side effect is that it allows some getName() style functions to
return a const string reference instead of a string copy.

Also, moves ClFlags into a static field of GlobalContext, i.e. makes it global,
so that these constant flags can be accessed without having to plumb a
GlobalContext object.  Note that some of the ClFlags and ClFlagsExtra plumbing
in the compile server classes could be simplified to directly use the
corresponding static fields, but this is left mostly as is for now, for when we
do a proper separation between core and supplemental flags.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-03-07 17:26:40]

Committed patchset #3 (id:40001) manually as
98ba00666271be1bdcd45b72b3dec04419efe61b (presubmit successful).