PNaCl Dynamic Linking: 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 53 matching lines @@
   GlobalDeclarationKind getKind() const { return Kind; }
   const IceString &getName() const { return Name; }
   void setName(const IceString &NewName) {
     Name = getSuppressMangling() ? NewName : mangleName(NewName);
   }
   bool hasName() const { return !Name.empty(); }
   bool isInternal() const {
     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;
   }
   virtual ~GlobalDeclaration() = default;
 
   /// Prints out type of the global declaration.
   virtual void dumpType(Ostream &Stream) const = 0;
 
   /// Prints out the global declaration.
   virtual void dump(Ostream &Stream) const = 0;
 
   /// Returns true if when emitting names, we should suppress mangling.
   virtual bool getSuppressMangling() const = 0;
 
   /// Returns textual name of linkage.
   const char *getLinkageName() const {
     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)
       : Kind(Kind), Linkage(Linkage) {}
 
   /// Returns true if linkage is defined correctly for the global declaration,
   /// based on default rules.
   bool verifyLinkageDefault(const GlobalContext *Ctx) const {
     switch (Linkage) {
     default:
       return false;
     case llvm::GlobalValue::InternalLinkage:
       return true;
     case llvm::GlobalValue::ExternalLinkage:
       return Ctx->getFlags().getAllowExternDefinedSymbols();
     }
   }
 
   const GlobalDeclarationKind Kind;
-  const llvm::GlobalValue::LinkageTypes Linkage;
+  llvm::GlobalValue::LinkageTypes Linkage;
   IceString Name;
 };
 
 /// Models a function declaration. This includes the type signature of the
 /// function, its calling conventions, and its linkage.
 class FunctionDeclaration : public GlobalDeclaration {
   FunctionDeclaration() = delete;
   FunctionDeclaration(const FunctionDeclaration &) = delete;
   FunctionDeclaration &operator=(const FunctionDeclaration &) = delete;
 
@@ skipping 12 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(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))
+    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
   /// if valid.
   bool validateTypeSignature(const GlobalContext *Ctx) const {
     bool IsIntrinsic;
     if (const Intrinsics::FullIntrinsicInfo *Info =
             getIntrinsicInfo(Ctx, &IsIntrinsic))
       return validateIntrinsicTypeSignature(Info);
     return !IsIntrinsic && validateRegularTypeSignature();
@@ skipping 19 matching lines @@
   const Ice::FuncSigType Signature;
   llvm::CallingConv::ID CallingConv;
   const 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;
+  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 {
     bool IsIntrinsic;
     getIntrinsicInfo(Ctx, &IsIntrinsic);
     return IsIntrinsic;
   }
 
   bool validateRegularTypeSignature() const;
 
@@ skipping 41 matching lines @@
     DataInitializer &operator=(const DataInitializer &) = delete;
 
   public:
     template <class... Args>
     static std::unique_ptr<DataInitializer> create(Args &&... TheArgs) {
       return makeUnique<DataInitializer>(std::forward<Args>(TheArgs)...);
     }
 
     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;
     }
 
   private:
     ENABLE_MAKE_UNIQUE;
 
     DataInitializer(const llvm::NaClBitcodeRecord::RecordVector &Values)
         : Initializer(DataInitializerKind), Contents(Values.size()) {
       for (SizeT I = 0; I < Values.size(); ++I)
@@ skipping 135 matching lines @@
   /// Adds Initializer to the list of initializers. Takes ownership of the
   /// initializer.
   void addInitializer(std::unique_ptr<Initializer> Initializer) {
     const bool OldSuppressMangling = getSuppressMangling();
     Initializers->emplace_back(std::move(Initializer));
     HasInitializer = true;
     // The getSuppressMangling() logic depends on whether the global variable
     // has initializers.  If its value changed as a result of adding an
     // initializer, then make sure we haven't previously set the name based on
     // faulty SuppressMangling logic.
     const bool SameMangling = (OldSuppressMangling == getSuppressMangling());

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

Just FYI.

The cross test framework involves mangling global identifier names with some
prefix so that pnacl-llc and pnacl-sz can each translate the same file and the
results can be linked together with a driver without multiply defined symbol
errors.  There is logic that determines during parsing whether a symbol should
be mangled.  The mangling happens when setName() is called.  For global
variables, the logic depends on whether or not the global variable has
initializers, and so there is a check here that the mangling decision doesn't
change after the variable name is set.

There was no such check for function names, because the logic depended only on
const members that were set in the ctor.  However, now you are allowing the
linkage to change, possibly after the name has been set and the mangling
decision determined.  As such, I wonder if setLinkage() should do a similar
check to be safe.

I don't like how fragile this is, but I couldn't find a better way without
re-architecting the way this kind of parsing is done.

(Actually, maybe - in a separate CL - an RAII helper could be added to every
setter.)

In this case, though, a cross test should never define the entry point, so this
wouldn't be an issue, but if setLinkage() is exposed, future CLs might be
tempted to use it "incorrectly".

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

I'll add an assert to "setLinkage" that the name is empty. This is still a
little fragile, but it's better than leaving "setLinkage" fully freely available
for anyone to use.

     (void)SameMangling;
     assert(!Name.empty() || SameMangling);
   }
 
   /// 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).
-  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);
   }
 
   static bool classof(const GlobalDeclaration *Addr) {
     return Addr->getKind() == VariableDeclarationKind;
   }
 
   bool getSuppressMangling() const final {
     if (ForceSuppressMangling)
       return true;
     return isExternal() && !hasInitializer();
   }
 
   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;
   bool HasInitializer = false;
   /// The alignment of the declared variable.
   uint32_t Alignment = 0;
   /// True if a declared (global) constant.
   bool IsConstant = false;
   /// If set to true, force getSuppressMangling() to return true.
   const bool ForceSuppressMangling;
@@ skipping 15 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