Force __pnacl_pso_root to be an external declaration.

src/IceGlobalInits.h

 //===- subzero/src/IceGlobalInits.h - Global declarations -------*- C++ -*-===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 136 matching lines @@
   bool isProto() const { return IsProto; }
   static bool classof(const GlobalDeclaration *Addr) {
     return Addr->getKind() == FunctionDeclarationKind;
   }
   void dumpType(Ostream &Stream) const final;
   void dump(GlobalContext *Ctx, Ostream &Stream) const final;
   bool getSuppressMangling() const final { return isExternal() && IsProto; }
 
   /// Returns true if linkage is correct for the function declaration.
   bool verifyLinkageCorrect(const GlobalContext *Ctx) const {
     if (isPNaClABIExternalName() || isIntrinsicName(Ctx))

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

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.

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

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.

       return Linkage == llvm::GlobalValue::ExternalLinkage;
     return verifyLinkageDefault(Ctx);
   }
 
   /// Validates that the type signature of the function is correct. Returns true
   /// if valid.
   bool validateTypeSignature(const GlobalContext *Ctx) const {
     bool IsIntrinsic;
     if (const Intrinsics::FullIntrinsicInfo *Info =
             getIntrinsicInfo(Ctx, &IsIntrinsic))
@@ skipping 23 matching lines @@
   bool IsProto;
 
   FunctionDeclaration(const FuncSigType &Signature,
                       llvm::CallingConv::ID CallingConv,
                       llvm::GlobalValue::LinkageTypes Linkage, bool IsProto)
       : GlobalDeclaration(FunctionDeclarationKind, Linkage),
         Signature(Signature), CallingConv(CallingConv), IsProto(IsProto) {}
 
   bool isPNaClABIExternalName() const {
     const char *Name = getName().c_str();
-    return strcmp(Name, "_start") == 0 || strcmp(Name, "__pnacl_pso_root") == 0;
+    return strcmp(Name, "_start") == 0;
   }
 
   bool isIntrinsicName(const GlobalContext *Ctx) const {
     bool IsIntrinsic;
     getIntrinsicInfo(Ctx, &IsIntrinsic);
     return IsIntrinsic;
   }
 
   bool validateRegularTypeSignature() const;
 
@@ skipping 197 matching lines @@
 
   /// Prints out type for initializer associated with the declaration to Stream.
   void dumpType(Ostream &Stream) const final;
 
   /// Prints out the definition of the global variable declaration (including
   /// initialization).
   void dump(GlobalContext *Ctx, Ostream &Stream) const final;
 
   /// Returns true if linkage is correct for the variable declaration.
   bool verifyLinkageCorrect(const GlobalContext *Ctx) const {
+    if (isPNaClABIExternalName()) {
+      return Linkage == llvm::GlobalValue::ExternalLinkage;
+    }
     return verifyLinkageDefault(Ctx);
   }
 
+  /// Returns true if the linkage was updated.
+  bool forcePNaClABILinkage() {
+    if (isPNaClABIExternalName()) {
+      /// Force Linkage to be External for PNaCl ABI.

[Mark Seaborn, 2016/02/26 01:56:56]

Nit: Just "//"

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

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

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

+      setLinkage(llvm::GlobalValue::ExternalLinkage);

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

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]

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.

+      return true;
+    }
+    return false;
+  }
+
   static bool classof(const GlobalDeclaration *Addr) {
     return Addr->getKind() == VariableDeclarationKind;
   }
 
   bool getSuppressMangling() const final {
     if (ForceSuppressMangling)
       return true;
     return isExternal() && !hasInitializer();
   }
 
   void setSuppressMangling() { ForceSuppressMangling = true; }
 
   void discardInitializers() { Initializers = nullptr; }
 
 private:
+  bool isPNaClABIExternalName() const {
+    const char *Name = getName().c_str();
+    return strcmp(Name, "__pnacl_pso_root") == 0;
+  }
+
   /// List of initializers for the declared variable.
   std::unique_ptr<InitializerListType> Initializers;
   bool HasInitializer;
   /// The alignment of the declared variable.
   uint32_t Alignment;
   /// True if a declared (global) constant.
   bool IsConstant;
   /// If set to true, force getSuppressMangling() to return true.
   bool ForceSuppressMangling;
 
@@ skipping 14 matching lines @@
 template <class StreamType>
 inline StreamType &operator<<(StreamType &Stream,
                               const GlobalDeclaration &Addr) {
   Addr.dump(Stream);
   return Stream;
 }
 
 } // end of namespace Ice
 
 #endif // SUBZERO_SRC_ICEGLOBALINITS_H