This is an archive of the discontinued LLVM Phabricator instance.

A number of issues in BrainF example
ClosedPublic

Authored by boris.ulasevich on Nov 3 2016, 5:47 AM.

Details

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!

Diff Detail

Repository
rL LLVM

Event Timeline

boris.ulasevich retitled this revision from to A number of issues in BrainF example.
boris.ulasevich updated this object.
boris.ulasevich added a reviewer: apilipenko.
apilipenko added inline comments.Nov 18 2016, 2:42 AM
../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 edited edge metadata.Nov 18 2016, 2:54 AM

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 marked an inline comment as done.Nov 24 2016, 1:19 AM
boris.ulasevich added inline comments.
../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);

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

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.

This revision was automatically updated to reflect the committed changes.