PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration.

src/IceGlobalInits.h

Index: src/IceGlobalInits.h
diff --git a/src/IceGlobalInits.h b/src/IceGlobalInits.h
index 419702d9cc867fc960fb66efa1b8fd0cae15c78c..63c2df8464ba5f98c67bead123a4a8af08e3c948 100644
--- a/src/IceGlobalInits.h
+++ b/src/IceGlobalInits.h
@@ -71,6 +71,7 @@ public:
     return Linkage == llvm::GlobalValue::InternalLinkage;
   }
   llvm::GlobalValue::LinkageTypes getLinkage() const { return Linkage; }
+  void setLinkage(llvm::GlobalValue::LinkageTypes l) { Linkage = l; }

[Mark Seaborn, 2016/03/09 03:37:15]

Style nit: should be "L"

[Sean Klein, 2016/03/10 23:45:22]

Done.

   bool isExternal() const {
     return Linkage == llvm::GlobalValue::ExternalLinkage;
   }
@@ -90,6 +91,10 @@ public:
     return isInternal() ? "internal" : "external";
   }
 
+  /// Returns true if the name of this GlobalDeclaration indicates that it
+  /// should have ExternalLinkage (as a special case).
+  virtual bool isPNaClABIExternalName() const = 0;
+
 protected:
   GlobalDeclaration(GlobalDeclarationKind Kind,
                     llvm::GlobalValue::LinkageTypes Linkage)
@@ -109,7 +114,7 @@ protected:
   }
 
   const GlobalDeclarationKind Kind;
-  const llvm::GlobalValue::LinkageTypes Linkage;
+  llvm::GlobalValue::LinkageTypes Linkage;
   IceString Name;
 };
 
@@ -142,9 +147,13 @@ public:
 
   /// Returns true if linkage is correct for the function declaration.
   bool verifyLinkageCorrect(const GlobalContext *Ctx) const {
-    if (isPNaClABIExternalName() || isIntrinsicName(Ctx))
+    if (isIntrinsicName(Ctx)) {
       return Linkage == llvm::GlobalValue::ExternalLinkage;
-    return verifyLinkageDefault(Ctx);
+    } else if (isPNaClABIExternalName()) {

[Mark Seaborn, 2016/03/09 03:37:15]

Isn't this equivalent to what this function was doing before?

[Sean Klein, 2016/03/10 23:45:22]

Agreed -- sorry, changed back.

+      return Linkage == llvm::GlobalValue::ExternalLinkage;
+    } else {
+      return verifyLinkageDefault(Ctx);
+    }
   }
 
   /// Validates that the type signature of the function is correct. Returns true
@@ -184,9 +193,9 @@ private:
       : 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;
+  bool isPNaClABIExternalName() const override {
+    static constexpr char Start[] = "_start";

[Mark Seaborn, 2016/03/09 03:37:15]

Why not just write "_start" on the following line?

[Sean Klein, 2016/03/10 23:45:22]

Done, and also with "__pnacl_pso_root".

+    return getName() == Start;
   }
 
   bool isIntrinsicName(const GlobalContext *Ctx) const {
@@ -248,7 +257,7 @@ public:
 
     const DataVecType &getContents() const { return Contents; }
     SizeT getNumBytes() const final { return Contents.size(); }
-    void dump(Ostream &Stream) const final;
+    void dump(Ostream &Stream) const override final;

[Karl, 2016/03/09 17:06:50]

Doesn't final imply override? Why are both necessary?

[Sean Klein, 2016/03/10 23:45:22]

This was causing the following clang error (hence the other 'dump' being marked
override):

error: 'dump' overrides a member function but is not marked 'override'
[-Werror,-Winconsistent-missing-override]

But in this instance, I agree, I think it is unnecessary with 'final'.

Updated.

     static bool classof(const Initializer *D) {
       return D->getKind() == DataInitializerKind;
     }
@@ -414,10 +423,13 @@ public:
 
   /// Prints out the definition of the global variable declaration (including
   /// initialization).
-  virtual void dump(Ostream &Stream) const;
+  virtual void dump(Ostream &Stream) const override;
 
   /// 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);
   }
 
@@ -433,6 +445,11 @@ public:
 
   void discardInitializers() { Initializers = nullptr; }
 
+  bool isPNaClABIExternalName() const override {
+    static constexpr char PnaclPsoRoot[] = "__pnacl_pso_root";
+    return getName() == PnaclPsoRoot;

[Jim Stichnoth, 2016/03/09 16:29:35]

Should this be conditional on GlobalContext::getFlags().getUseNonsfi() ?

[Sean Klein, 2016/03/10 23:45:22]

Is there a reason dynamic linking couldn't be supported in an SFI application?

[Jim Stichnoth, 2016/03/11 01:20:29]

Sorry, I got confused about the scope of this change.  I was thinking it was for
Non-SFI, but that's orthgonal to PSOs.

+  }
+
 private:
   /// List of initializers for the declared variable.
   std::unique_ptr<InitializerListType> Initializers;