Force __pnacl_pso_root to be an external declaration.

Force __pnacl_pso_root to be an external declaration.

This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol
"__pnacl_pso_root" as a function, but it is a declaration, and should be
checked as one. Additionally, when the PNaClTranslator is verifying the linkage
of declarations, allow "__pnacl_pso_root" to be flipped to external as a special
case.

TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
R=jpp@chromium.org

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

[Sean Klein, 2016-02-26 00:57:08]

Description was changed from

==========
Force __pnacl_pso_root to be an external declaration.

TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
Force __pnacl_pso_root to be an external declaration.

This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol
"__pnacl_pso_root" as a function, but it is a declaration, and should be
checked as one. Additionally, when the PNaClTranslator is verifying the linkage
of declarations, allow "__pnacl_pso_root" to be flipped to external as a special
case.

TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

[Sean Klein, 2016-02-26 00:58:32]

smklein@chromium.org changed reviewers:
+ mseaborn@chromium.org, stichnot@chromium.org

[Mark Seaborn, 2016-02-26 01:56:57]

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:157: if (isPNaClABIExternalName() || isIntrinsicName(Ctx))
This check is actually backwards.

It should be:

if (Linkage == llvm::GlobalValue::ExternalLinkage)
  return isPNaClABIExternalName();
[plus handle intrinsics]

As it stands, if we have a unstripped PSO with an internal function called
"_start", this check will reject it, which isn't right.  (However, we've not yet
clearly stated whether unstripped pexes/PSOs are allowed or not.  Currently
pnacl-finalize does strip them.)

Feel free to leave this aspect to a separate change though.

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:428: /// Force Linkage to be External for PNaCl ABI.
Nit: Just "//"

Also you probably don't want to capitalise "linkage" and "external".

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:429: setLinkage(llvm::GlobalValue::ExternalLinkage);
A comment about why this is necessary would be good.  i.e. PNaCl bitcode has a
linkage field for Functions but not for GlobalVariables (because we didn't need
the latter for pexes and so we took it out).

Also, ideally we want to check that exactly one of _start and __pnacl_pso_root
is present...

[Sean Klein, 2016-02-26 02:28:06]

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:157: if (isPNaClABIExternalName() || isIntrinsicName(Ctx))
This is small (and related) enough that I've updated it in this CL.

Intrinsics must be external,
Non-intrinsic externals can only be "_start",
and everything else is default.

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:428: /// Force Linkage to be External for PNaCl ABI.
On 2016/02/26 01:56:56, Mark Seaborn wrote:
> Nit: Just "//"
> 
> Also you probably don't want to capitalise "linkage" and "external".

Updated. Re: "//", I was just trying to match the style of the rest of the file.

https://codereview.chromium.org/1740033002/diff/20001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:429: setLinkage(llvm::GlobalValue::ExternalLinkage);
Comment added. Where would you recommend doing that check? There doesn't seem to
be an obvious spot that doesn't re-iterate over all the declarations/functions.

[Jim Stichnoth, 2016-02-26 17:35:27]

stichnot@chromium.org changed reviewers:
+ jpp@chromium.org

[John, 2016-02-26 18:40:53]

https://codereview.chromium.org/1740033002/diff/40001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1740033002/diff/40001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:456: const char *Name = getName().c_str();
static constexpr char PnaclPsoRoot[] = "__pnacl_pso_root";
return getName() == PnaclPsoRoot;

https://codereview.chromium.org/1740033002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1740033002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:550: for (Ice::VariableDeclaration *Decl :
*VariableDeclarations) {
I would much rather you forcePNaClABILinkage() where the variable is created.

[Sean Klein, 2016-02-26 19:45:04]

https://codereview.chromium.org/1740033002/diff/40001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1740033002/diff/40001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:456: const char *Name = getName().c_str();
On 2016/02/26 18:40:53, John wrote:
> static constexpr char PnaclPsoRoot[] = "__pnacl_pso_root";
> return getName() == PnaclPsoRoot;

Done.

https://codereview.chromium.org/1740033002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1740033002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:550: for (Ice::VariableDeclaration *Decl :
*VariableDeclarations) {
Done.

[John, 2016-02-26 19:48:37]

lgtm

https://codereview.chromium.org/1740033002/diff/60001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1740033002/diff/60001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:207: static constexpr char Start[] = "_start";
thanks! :)

[Sean Klein, 2016-02-26 22:34:42]

Description was changed from

==========
Force __pnacl_pso_root to be an external declaration.

This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol
"__pnacl_pso_root" as a function, but it is a declaration, and should be
checked as one. Additionally, when the PNaClTranslator is verifying the linkage
of declarations, allow "__pnacl_pso_root" to be flipped to external as a special
case.

TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
Force __pnacl_pso_root to be an external declaration.

This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol
"__pnacl_pso_root" as a function, but it is a declaration, and should be
checked as one. Additionally, when the PNaClTranslator is verifying the linkage
of declarations, allow "__pnacl_pso_root" to be flipped to external as a special
case.

TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
R=jpp@chromium.org

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

[Sean Klein, 2016-02-26 22:34:44]

Committed patchset #4 (id:60001) manually as
352db9354e9c30f99c6f87674f70a6f23a9ef7eb (presubmit successful).

[Mark Seaborn, 2016-02-26 23:18:58]

https://codereview.chromium.org/1740033002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1740033002/diff/60001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:483:
Decl->setLinkage(llvm::GlobalValue::ExternalLinkage);
It looks like you're doing this for functions, but that's not ideal -- you
should only do this for GlobalVariables.