Assemble calls to constant addresses.

src/IceClFlags.h

@@ -30,8 +30,8 @@ public:
         DecorateAsm(false), DisableInternal(false), DisableIRGeneration(false),
         DisableTranslation(false), DumpStats(false), FunctionSections(false),
         GenerateUnitTestMessages(false), PhiEdgeSplit(false),
-        StubConstantCalls(false), SubzeroTimingEnabled(false),
-        TimeEachFunction(false), UseSandboxing(false),
+        SubzeroTimingEnabled(false), TimeEachFunction(false),
+        UseSandboxing(false),
         // FileType field
         OutFileType(FT_Iasm),
         // IceString fields.
@@ -87,11 +87,6 @@ public:
   bool getPhiEdgeSplit() const { return PhiEdgeSplit; }
   void setPhiEdgeSplit(bool NewValue) { PhiEdgeSplit = NewValue; }
 
-  bool getStubConstantCalls() const {
-    return !ALLOW_MINIMAL_BUILD && StubConstantCalls;
-  }
-  void setStubConstantCalls(bool NewValue) { StubConstantCalls = NewValue; }
-
   bool getSubzeroTimingEnabled() const { return SubzeroTimingEnabled; }
   void setSubzeroTimingEnabled(bool NewValue) {
     SubzeroTimingEnabled = NewValue;
@@ -154,7 +149,6 @@ private:
   bool FunctionSections;
   bool GenerateUnitTestMessages;
   bool PhiEdgeSplit;
-  bool StubConstantCalls;
   bool SubzeroTimingEnabled;
   bool TimeEachFunction;
   bool UseSandboxing;

src/IceELFObjectWriter.cpp

@@ -84,10 +84,7 @@ ELFObjectWriter::ELFObjectWriter(GlobalContext &Ctx, ELFStreamer &Out)
                 "Elf_Sym sizes cannot be derived from sizeof");
   SymTab = createSection<ELFSymbolTableSection>(SymTabName, SHT_SYMTAB, 0,
                                                 SymTabAlign, SymTabEntSize);
-  // The first entry in the symbol table should be a NULL entry.
-  const IceString NullSymName("");
-  SymTab->createDefinedSym(NullSymName, STT_NOTYPE, STB_LOCAL, NullSection, 0,
-                           0);
+  SymTab->createNullSymbol(NullSection);
 
   const IceString StrTabName(".strtab");
   StrTab =

src/IceELFSection.cpp

@@ -80,6 +80,15 @@ size_t ELFRelocationSection::getSectionDataSize() const {
 
 // Symbol tables.
 
+void ELFSymbolTableSection::createNullSymbol(ELFSection *NullSection) {
+  // The first entry in the symbol table should be a NULL entry,
+  // so make sure the map is still empty.
+  assert(LocalSymbols.empty());
+  const IceString NullSymName("");
+  createDefinedSym(NullSymName, STT_NOTYPE, STB_LOCAL, NullSection, 0, 0);
+  NullSymbol = findSymbol(NullSymName);
+}
+
 void ELFSymbolTableSection::createDefinedSym(const IceString &Name,
                                              uint8_t Type, uint8_t Binding,
                                              ELFSection *Section,
@@ -105,7 +114,9 @@ void ELFSymbolTableSection::noteUndefinedSym(const IceString &Name,
   NewSymbol.Sym.setBindingAndType(STB_GLOBAL, STT_NOTYPE);
   NewSymbol.Section = NullSection;
   NewSymbol.Number = ELFSym::UnknownNumber;
-  GlobalSymbols.insert(std::make_pair(Name, NewSymbol));
+  bool Unique = GlobalSymbols.insert(std::make_pair(Name, NewSymbol)).second;
+  assert(Unique);
+  (void)Unique;
 }
 
 const ELFSym *ELFSymbolTableSection::findSymbol(const IceString &Name) const {

src/IceELFSection.h

@@ -163,7 +163,11 @@ class ELFSymbolTableSection : public ELFSection {
   ELFSymbolTableSection &operator=(const ELFSymbolTableSection &) = delete;
 
 public:
-  using ELFSection::ELFSection;
+  ELFSymbolTableSection(const IceString &Name, Elf64_Word ShType,
+                        Elf64_Xword ShFlags, Elf64_Xword ShAddralign,
+                        Elf64_Xword ShEntsize)
+      : ELFSection(Name, ShType, ShFlags, ShAddralign, ShEntsize),
+        NullSymbol(nullptr) {}
 
   // Create initial entry for a symbol when it is defined.
   // Each entry should only be defined once.
@@ -179,6 +183,9 @@ public:
 
   const ELFSym *findSymbol(const IceString &Name) const;
 
+  void createNullSymbol(ELFSection *NullSection);
+  const ELFSym *getNullSymbol() const { return NullSymbol; }
+
   size_t getSectionDataSize() const {
     return (LocalSymbols.size() + GlobalSymbols.size()) * Header.sh_entsize;
   }
@@ -198,6 +205,7 @@ private:
   template <bool IsELF64>
   void writeSymbolMap(ELFStreamer &Str, const SymMap &Map);
 
+  const ELFSym *NullSymbol;
   // Keep Local and Global symbols separate, since the sh_info needs to
   // know the index of the last LOCAL.
   SymMap LocalSymbols;
@@ -211,7 +219,11 @@ class ELFRelocationSection : public ELFSection {
   ELFRelocationSection &operator=(const ELFRelocationSection &) = delete;
 
 public:
-  using ELFSection::ELFSection;
+  ELFRelocationSection(const IceString &Name, Elf64_Word ShType,
+                       Elf64_Xword ShFlags, Elf64_Xword ShAddralign,
+                       Elf64_Xword ShEntsize)
+      : ELFSection(Name, ShType, ShFlags, ShAddralign, ShEntsize),
+        RelatedSection(nullptr) {}
 
   const ELFSection *getRelatedSection() const { return RelatedSection; }
   void setRelatedSection(const ELFSection *Section) {
@@ -340,7 +352,11 @@ template <bool IsELF64>
 void ELFRelocationSection::writeData(const GlobalContext &Ctx, ELFStreamer &Str,
                                      const ELFSymbolTableSection *SymTab) {
   for (const AssemblerFixup &Fixup : Fixups) {
-    const ELFSym *Symbol = SymTab->findSymbol(Fixup.symbol(&Ctx));
+    const ELFSym *Symbol;
+    if (Fixup.isNullSymbol())
+      Symbol = SymTab->getNullSymbol();
+    else
+      Symbol = SymTab->findSymbol(Fixup.symbol(&Ctx));
     if (!Symbol)
       llvm::report_fatal_error("Missing symbol mentioned in reloc");
 

src/IceFixups.cpp

@@ -17,7 +17,11 @@
 
 namespace Ice {
 
+const Constant *AssemblerFixup::NullSymbol = nullptr;
+
 RelocOffsetT AssemblerFixup::offset() const {
+  if (isNullSymbol())
+    return 0;
   if (const auto CR = llvm::dyn_cast<ConstantRelocatable>(value_))
     return CR->getOffset();
   return 0;
@@ -27,6 +31,7 @@ IceString AssemblerFixup::symbol(const GlobalContext *Ctx) const {
   std::string Buffer;
   llvm::raw_string_ostream Str(Buffer);
   const Constant *C = value_;
+  assert(!isNullSymbol());
   if (const auto CR = llvm::dyn_cast<ConstantRelocatable>(C)) {
     if (CR->getSuppressMangling())
       Str << CR->getName();
@@ -43,6 +48,9 @@ IceString AssemblerFixup::symbol(const GlobalContext *Ctx) const {
 
 void AssemblerFixup::emit(GlobalContext *Ctx) const {
   Ostream &Str = Ctx->getStrEmit();
+  // TODO(jvoung): Not yet clear how to emit a relocation against
+  // the null symbol.
+  assert(!isNullSymbol());
   Str << symbol(Ctx);
   RelocOffsetT Offset = offset();
   if (Offset)

src/IceFixups.h

@@ -38,6 +38,10 @@ public:
 
   RelocOffsetT offset() const;
   IceString symbol(const GlobalContext *Ctx) const;
+
+  static const Constant *NullSymbol;
+  bool isNullSymbol() const { return value_ == NullSymbol; }
+
   void set_value(const Constant *Value) { value_ = Value; }
 
   void emit(GlobalContext *Ctx) const;

src/IceInstX8632.cpp

@@ -489,11 +489,10 @@ void InstX8632Jmp::emitIAS(const Cfg *Func) const {
   } else if (const auto Imm = llvm::dyn_cast<ConstantInteger32>(Target)) {
     // NaCl trampoline calls refer to an address within the sandbox directly.
     // This is usually only needed for non-IRT builds and otherwise not
-    // very portable or stable. For this, we would use the 0xE8 opcode
-    // (relative version of call) and there should be a PC32 reloc too.
-    // The PC32 reloc will have symbol index 0, and the absolute address
-    // would be encoded as an offset relative to the next instruction.
-    // TODO(jvoung): Do we need to support this?
+    // very portable or stable. Usually this is only done for "calls"
+    // and not jumps.
+    // TODO(jvoung): Support this when there is a lowering that
+    // actually triggers this case.
     (void)Imm;
     llvm::report_fatal_error("Unexpected jmp to absolute address");
   } else {
@@ -515,10 +514,7 @@ void InstX8632Call::emit(const Cfg *Func) const {
   Ostream &Str = Func->getContext()->getStrEmit();
   assert(getSrcSize() == 1);
   Str << "\tcall\t";
-  if (const auto CallTarget =
-          llvm::dyn_cast<ConstantRelocatable>(getCallTarget())) {
-    // TODO(stichnot): All constant targets should suppress the '$',
-    // not just relocatables.
+  if (const auto CallTarget = llvm::dyn_cast<Constant>(getCallTarget())) {
     CallTarget->emitWithoutDollar(Func->getContext());
   } else {
     Str << "*";
@@ -544,15 +540,7 @@ void InstX8632Call::emitIAS(const Cfg *Func) const {
     assert(CR->getOffset() == 0 && "We only support calling a function");
     Asm->call(CR);
   } else if (const auto Imm = llvm::dyn_cast<ConstantInteger32>(Target)) {
-    // NaCl trampoline calls refer to an address within the sandbox directly.
-    // This is usually only needed for non-IRT builds and otherwise not
-    // very portable or stable. For this, we would use the 0xE8 opcode
-    // (relative version of call) and there should be a PC32 reloc too.
-    // The PC32 reloc will have symbol index 0, and the absolute address
-    // would be encoded as an offset relative to the next instruction.
-    // TODO(jvoung): Do we need to support this?
-    (void)Imm;
-    llvm_unreachable("Unexpected call to absolute address");
+    Asm->call(x86::Immediate(Imm->getValue()));
   } else {
     llvm_unreachable("Unexpected operand type");
   }

src/IceOperand.h

@@ -108,6 +108,7 @@ public:
   }
   void emit(const Cfg *Func) const override { emit(Func->getContext()); }
   virtual void emit(GlobalContext *Ctx) const = 0;
+  virtual void emitWithoutDollar(GlobalContext *Ctx) const = 0;
 
   static bool classof(const Operand *Operand) {
     OperandKind Kind = Operand->getKind();
@@ -149,6 +150,7 @@ public:
   // The target needs to implement this for each ConstantPrimitive
   // specialization.
   void emit(GlobalContext *Ctx) const override;
+  void emitWithoutDollar(GlobalContext *Ctx) const override;
   using Constant::dump;
   void dump(const Cfg *, Ostream &Str) const override {
     if (ALLOW_DUMP)
@@ -234,7 +236,7 @@ public:
   using Constant::emit;
   using Constant::dump;
   void emit(GlobalContext *Ctx) const override;
-  void emitWithoutDollar(GlobalContext *Ctx) const;
+  void emitWithoutDollar(GlobalContext *Ctx) const override;
   void dump(const Cfg *Func, Ostream &Str) const override;
 
   static bool classof(const Operand *Operand) {
@@ -273,6 +275,7 @@ public:
   using Constant::dump;
   // The target needs to implement this.
   void emit(GlobalContext *Ctx) const override;
+  void emitWithoutDollar(GlobalContext *Ctx) const override;
   void dump(const Cfg *, Ostream &Str) const override {
     if (ALLOW_DUMP)
       Str << "undef";

src/IceTargetLoweringX8632.cpp

@@ -4599,17 +4599,34 @@ void TargetX8632::makeRandomRegisterPermutation(
   }
 }
 
+template <>
+void ConstantInteger32::emitWithoutDollar(GlobalContext *Ctx) const {
+  if (!ALLOW_DUMP)
+    return;
+  Ostream &Str = Ctx->getStrEmit();
+  Str << (int32_t)getValue();
+}
+
 template <> void ConstantInteger32::emit(GlobalContext *Ctx) const {
   if (!ALLOW_DUMP)
     return;
   Ostream &Str = Ctx->getStrEmit();
-  Str << "$" << (int32_t)getValue();
+  Str << "$";
+  emitWithoutDollar(Ctx);
+}
+
+template <> void ConstantInteger64::emitWithoutDollar(GlobalContext *) const {
+  llvm_unreachable("Not expecting to emitWithoutDollar 64-bit integers");
 }
 
 template <> void ConstantInteger64::emit(GlobalContext *) const {
   llvm_unreachable("Not expecting to emit 64-bit integers");
 }
 
+template <> void ConstantFloat::emitWithoutDollar(GlobalContext *) const {
+  llvm_unreachable("Not expecting to emitWithoutDollar floats");
+}
+
 template <> void ConstantFloat::emit(GlobalContext *Ctx) const {
   if (!ALLOW_DUMP)
     return;
@@ -4617,6 +4634,10 @@ template <> void ConstantFloat::emit(GlobalContext *Ctx) const {
   emitPoolLabel(Str);
 }
 
+template <> void ConstantDouble::emitWithoutDollar(GlobalContext *) const {
+  llvm_unreachable("Not expecting to emitWithoutDollar doubles");
+}
+
 template <> void ConstantDouble::emit(GlobalContext *Ctx) const {
   if (!ALLOW_DUMP)
     return;
@@ -4624,6 +4645,10 @@ template <> void ConstantDouble::emit(GlobalContext *Ctx) const {
   emitPoolLabel(Str);
 }
 
+void ConstantUndef::emitWithoutDollar(GlobalContext *) const {
+  llvm_unreachable("Not expecting to emitWithoutDollar undef");
+}
+
 void ConstantUndef::emit(GlobalContext *) const {
   llvm_unreachable("undef value encountered by emitter.");
 }

src/PNaClTranslator.cpp

@@ -167,7 +167,7 @@ public:
       : NaClBitcodeParser(Cursor), Translator(Translator),
         ErrorStatus(ErrorStatus), NumErrors(0), NextDefiningFunctionID(0),
         VariableDeclarations(new Ice::VariableDeclarationList()),
-        BlockParser(nullptr), StubbedConstCallValue(nullptr) {}
+        BlockParser(nullptr) {}
 
   ~TopLevelParser() override {}
 
@@ -276,24 +276,6 @@ public:
     createValueIDsForGlobalVars();
   }
 
-  /// Returns a defined function reference to be used in place of
-  /// called constant addresses. Returns the corresponding operand
-  /// to replace the calling address with. Reports an error if
-  /// a stub could not be found, returning the CallValue.
-  Ice::Operand *getStubbedConstCallValue(Ice::Operand *CallValue) {
-    if (StubbedConstCallValue)
-      return StubbedConstCallValue;
-    for (unsigned i = 0; i < getNumFunctionIDs(); ++i) {
-      Ice::FunctionDeclaration *Func = getFunctionByID(i);
-      if (!Func->isProto()) {
-        StubbedConstCallValue = getGlobalConstantByID(i);
-        return StubbedConstCallValue;
-      }
-    }
-    Error("Unable to find function definition to stub constant calls with");
-    return CallValue;
-  }
-
   /// Returns the number of function declarations in the bitcode file.
   unsigned getNumFunctionIDs() const { return FunctionDeclarationList.size(); }
 
@@ -383,8 +365,6 @@ private:
   // The block parser currently being applied. Used for error
   // reporting.
   BlockParserBaseClass *BlockParser;
-  // Value to use to stub constant calls.
-  Ice::Operand *StubbedConstCallValue;
 
   bool ParseBlock(unsigned BlockID) override;
 
@@ -510,8 +490,8 @@ bool TopLevelParser::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
   raw_ostream &OldErrStream = setErrStream(Context->getStrDump());
   NaClBitcodeParser::ErrorAt(Level, Bit, Message);
   setErrStream(OldErrStream);
-  if (Level >= naclbitc::Error
-      && !Translator.getFlags().getAllowErrorRecovery())
+  if (Level >= naclbitc::Error &&
+      !Translator.getFlags().getAllowErrorRecovery())
     Fatal();
   return true;
 }
@@ -677,8 +657,8 @@ bool TopLevelParser::BlockError(const std::string &Message) {
 }
 
 // Generates an error Message with the bit address prefixed to it.
-bool BlockParserBaseClass::ErrorAt(
-    naclbitc::ErrorLevel Level, uint64_t Bit, const std::string &Message) {
+bool BlockParserBaseClass::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
+                                   const std::string &Message) {
   std::string Buffer;
   raw_string_ostream StrBuf(Buffer);
   // Note: If dump routines have been turned off, the error messages
@@ -2507,10 +2487,6 @@ void FunctionParser::ProcessRecord() {
         return;
       }
     } else {
-      if (getFlags().getStubConstantCalls() &&
-          llvm::isa<Ice::ConstantInteger32>(Callee)) {
-        Callee = Context->getStubbedConstCallValue(Callee);
-      }
       ReturnType = Context->getSimpleTypeByID(Values[2]);
     }
 

src/assembler_ia32.cpp

@@ -112,6 +112,17 @@ void AssemblerX86::call(const ConstantRelocatable *label) {
   (void)call_start;
 }
 
+void AssemblerX86::call(const Immediate &abs_address) {
+  AssemblerBuffer::EnsureCapacity ensured(&buffer_);
+  intptr_t call_start = buffer_.GetPosition();
+  EmitUint8(0xE8);
+  EmitFixup(
+      this->createFixup(llvm::ELF::R_386_PC32, AssemblerFixup::NullSymbol));
+  EmitInt32(abs_address.value() - 4);
+  assert((buffer_.GetPosition() - call_start) == kCallExternalLabelSize);
+  (void)call_start;
+}
+
 void AssemblerX86::pushl(GPRRegister reg) {
   AssemblerBuffer::EnsureCapacity ensured(&buffer_);
   EmitUint8(0x50 + reg);

src/assembler_ia32.h

@@ -475,6 +475,7 @@ public:
   void call(GPRRegister reg);
   void call(const Address &address);
   void call(const ConstantRelocatable *label);
+  void call(const Immediate &abs_address);
 
   static const intptr_t kCallExternalLabelSize = 5;
 

src/main.cpp

@@ -176,12 +176,6 @@ static cl::opt<bool> AllowErrorRecovery(
     cl::desc("Allow error recovery when reading PNaCl bitcode."),
     cl::init(false));
 
-// TODO(kschimpf) Remove once the emitter handles these cases.
-static cl::opt<bool>
-    StubConstantCalls("stub-const-calls",
-                      cl::desc("Stub indirect calls to constants."),
-                      cl::init(false));
-
 static cl::opt<bool> LLVMVerboseErrors(
     "verbose-llvm-parse-errors",
     cl::desc("Print out more descriptive PNaCl bitcode parse errors when "
@@ -311,7 +305,6 @@ int main(int argc, char **argv) {
   Flags.setFunctionSections(FunctionSections);
   Flags.setNumTranslationThreads(NumThreads);
   Flags.setPhiEdgeSplit(EnablePhiEdgeSplit);
-  Flags.setStubConstantCalls(StubConstantCalls);
   Flags.setSubzeroTimingEnabled(SubzeroTimingEnabled);
   Flags.setTimeEachFunction(TimeEachFunction);
   Flags.setTimingFocusOn(TimingFocusOn);

tests_lit/llvm2ice_tests/callindirect.pnacl.ll

@@ -4,6 +4,9 @@
 
 ; RUN: %p2i --filetype=obj --disassemble -i %s --args -O2 \
 ; RUN:   | FileCheck %s
+; RUN: %if --need=allow_dump --command %p2i --filetype=asm --assemble \
+; RUN:     --disassemble -i %s --args -O2 \
+; RUN:   | %if --need=allow_dump --command FileCheck %s
 ; RUN: %p2i --filetype=obj --disassemble -i %s --args -Om1 \
 ; RUN:   | FileCheck --check-prefix=OPTM1 %s
 
@@ -62,14 +65,23 @@ entry:
 ; OPTM1: call [[TARGET]]
 
 ; Calling an absolute address is used for non-IRT PNaCl pexes to directly
-; access syscall trampolines. Do we need to support this?
-; define internal void @CallIndirectConst() {
-; entry:
-;   %__1 = inttoptr i32 66496 to void ()*
-;   call void %__1()
-;   call void %__1()
-;   call void %__1()
-;   call void %__1()
-;   call void %__1()
-;   ret void
-; }
+; access syscall trampolines. This is not really an indirect call, but
+; there is a cast from int to pointer first.
+define internal void @CallConst() {
+entry:
+  %__1 = inttoptr i32 66496 to void ()*
+  call void %__1()
+  call void %__1()
+  call void %__1()
+  ret void
+}
+
+; CHECK-LABEL: CallConst
+; CHECK: e8 bc 03 01 00 call {{[0-9a-f]+}} {{.*}} R_386_PC32 *ABS*
+; CHECK: e8 bc 03 01 00 call {{[0-9a-f]+}} {{.*}} R_386_PC32 *ABS*
+; CHECK: e8 bc 03 01 00 call {{[0-9a-f]+}} {{.*}} R_386_PC32 *ABS*
+;
+; OPTM1-LABEL: CallConst
+; OPTM1: e8 bc 03 01 00 call {{[0-9a-f]+}} {{.*}} R_386_PC32 *ABS*
+; OPTM1: e8 bc 03 01 00 call {{[0-9a-f]+}} {{.*}} R_386_PC32 *ABS*
+; OPTM1: e8 bc 03 01 00 call {{[0-9a-f]+}} {{.*}} R_386_PC32 *ABS*