A number of issues in BrainF example

[boris.ulasevich]

Summary

BrainF is a cool example demonstrating LLVM capabilities.
I fixed Segmentation fault issue reproduced with -jit option:

• MCJIT dependency added
• AsmPrinter initialiser added
• fflush call added to let symbols appear on console

There were a couple of minor changes in the example code for last years, but there is no person responsible for the example. Can somebody have a look to this review please. Thanks!

[apilipenko]

../llvm-trunk/examples/BrainF/BrainFDriver.cpp

162 ↗

(On Diff #76845)

NULL is obsoleted in C++11 in favour of nullptr. Moreover typical LLVM code style will be just:

if (!ee)

169–170 ↗

(On Diff #76845)

It seems like the better place for this will be in the generated code.

[apilipenko]

Posting diffs with full context makes them easier to review. To generate a diff with full context use:

git diff -U999999 other-branch

or

svn diff --diff-cmd=diff -x -U999999

http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[boris.ulasevich]

../llvm-trunk/examples/BrainF/BrainFDriver.cpp

169–170 ↗

(On Diff #76845)

Good idea, thank you!
But I stuck in stdout variable and type linkage names which depends on stdlib implementation, and I see no way to make universal solution (see code below). I propose to skip this suggestion and do not add fflush(stdout) call to generated function.

Index: ../llvm-trunk/examples/BrainF/BrainF.cpp
===================================================================
--- ../llvm-trunk/examples/BrainF/BrainF.cpp	(revision 287753)
+++ ../llvm-trunk/examples/BrainF/BrainF.cpp	(working copy)
@@ -50,6 +50,9 @@
 const char *BrainF::label   = "brainf";
 const char *BrainF::testreg = "test";

+const char *BrainF::stdout_link_name = "__stdoutp";      // Apple stdlib impl only
+const char *BrainF::stdout_link_type = "struct.__sFILE"; // Apple stdlib impl only
+
 Module *BrainF::parse(std::istream *in1, int mem, CompileFlags cf,
                       LLVMContext& Context) {
   in       = in1;
@@ -80,7 +83,25 @@
   putchar_func = cast<Function>(module->
     getOrInsertFunction("putchar", IntegerType::getInt32Ty(C),
                         IntegerType::getInt32Ty(C), NULL));
+
+  StructType *structType_sFILE = module->getTypeByName(stdout_link_type);
+  if (!structType_sFILE) {
+    structType_sFILE = StructType::create(module->getContext(), stdout_link_type);
+  }
+  pointerType_sFile = PointerType::get(structType_sFILE, 0);
+
+  gvar_stdout = new GlobalVariable(*module,
+                                   pointerType_sFile,
+                                   false, //isConstant
+                                   GlobalValue::ExternalLinkage,
+                                   0,     //Initializer
+                                   stdout_link_name);

+  //fflush i32 @fflush(%struct.__sFILE*)
+  fflush_func = cast<Function>(module->
+     getOrInsertFunction("fflush",
+                         IntegerType::getInt32Ty(C), pointerType_sFile, NULL));
+
   //Function header

   //define void @brainf()
@@ -465,6 +486,13 @@

     return;
   }
+
+  if (cursym == SYM_EOF) {
+    //call fflush(stdout)
+    llvm::Value *value = builder->CreateLoad(gvar_stdout);
+    CallInst *fflush_call = builder->CreateCall(fflush_func, { value });
+    fflush_call->setTailCall(false);
+  }

   //End of the program, so go to return block
   builder->CreateBr(endbb);

[boris.ulasevich]

“Doctor, doctor, everyone keeps ignoring me”. “Next please!”

[apilipenko]

General practice in LLVM is to make small isolated changes. In this review you seem to have at least two unrelated changes:

1. Fixing the segfault,
2. Fixing the output issue.

Both changes are fine on their own, but when they are mixed in one commit it's more difficult to review and reason about. Given BrainF is an example it's not a big deal, but in general it's considered to be a bad practice.

Having said that LGTM to the segfault fix part. After that you can post another review for the output problem.

../llvm-trunk/examples/BrainF/BrainFDriver.cpp

169–170 ↗

(On Diff #76845)

Ok, I'm fine with putting the flush here with a comment that the better place for it is in compiled code, but it's more complicated.

llvm/trunk/examples/BrainF/BrainFDriver.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "BrainF.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
 #include "llvm/ExecutionEngine/ExecutionEngine.h"
 #include "llvm/ExecutionEngine/GenericValue.h"
+#include "llvm/ExecutionEngine/MCJIT.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Value.h"
int main(int argc, char **argv) {
   if (verifyModule(*Mod)) {
     errs() << "Error: module failed verification.  This shouldn't happen.\n";
     abort();
   }
 
   //Write it out
   if (JIT) {
     InitializeNativeTarget();
+    InitializeNativeTargetAsmPrinter();
 
     outs() << "------- Running JIT -------\n";
     Module &M = *Mod;
     ExecutionEngine *ee = EngineBuilder(std::move(Mod)).create();
+    if (!ee) {
+      errs() << "Error: execution engine creation failed.\n";
+      abort();
+    }
     std::vector<GenericValue> args;
     Function *brainf_func = M.getFunction("brainf");
     GenericValue gv = ee->runFunction(brainf_func, args);
   } else {
     WriteBitcodeToFile(Mod.get(), *out);
   }
 
   //Clean up
   if (out != &outs())
     delete out;
 
   llvm_shutdown();
 
   return 0;
 }

llvm/trunk/examples/BrainF/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   BitWriter
   Core
   ExecutionEngine
   MC
+  MCJIT
   Support
   nativecodegen
   )
 
 add_llvm_example(BrainF
   BrainF.cpp
   BrainFDriver.cpp
   )