Initial LLVM 7 port #149
Initial LLVM 7 port #149
Conversation
|
Thanks! yes, I think ifdefs will be needed so it can compile on llvm 5 (which we've been doing on Ubuntu), llvm 6 (which I know is commonly available in another major environment) and llvm 7. It's been suggested that the LLVM C API is more stable than the C++ one, and might help with this, but I haven't investigated switching to it yet. |
|
Have you tried this without I'm having failures like: I've noted that my LLVM is compiled without RTTI (which I'm is the default), but bpftrace is compiled with RTTI enabled (it uses it in some parser related code). Maybe that's behind this issue. |
|
@cmarcelo I'm using it with the default flags and have no problem building it. I'm currently using Arch, which appears to ship typeinfo. Which distro are you using? @brendangregg I chatted with a friend who did a lot of development with LLVM and it appears that the only way to support multiple major versions is to use ifdefs. This leads to another issue: codegen specs compare generated LLIL with expected output, which simply doesn't work with different LLVM versions. One way to solve it would be to have separate codegen specs for each major. |
|
Ok, thanks, looks like this is the best solution for now. And yes, we've already discussed that tests/codegen.cpp is tied to the LLVM version: it'll need ifdefs. Since that's a lot of work, I'd be fine with an initial PR that did the ifdefs for src/ast like you just did, then follow up with another for the tests. BTW, how I do those tests is to delete the LLVM output from the test and then run bpftrace_tests, and the diff warning tells me what I need to copy-n-paste into tests/codegen.cpp to make it pass (need to delete the /^+/ and extra 's from the diff output.) |
|
I tested this on llvm 5.0, and that environment still works. |
|
@yonghong-song did you want to check this out for your environment? |
|
@ajor might want to confirm this is the way to go as well... |
|
My cmake command does not have |
|
@yonghong-song are they the same failures? Can you copy-n-paste some? It may be that there's changes between llvm 7 and 8, and we need more ifdefs. |
|
Yeah this approach seems fine to me. The codegen tests are a bit of a mess at the moment (I don't think they pass for any LLVM version) so I wouldn't bother converting them yet. I'm planning to split up the massive codegen.cpp, so we could also split out the tests by LLVM version at the same time.
Just to add: make sure you check the new IR output is sensible before copying it in! |
|
@mlen the issue was lack of RTTI in LLVM. It worked for me after I've compiled LLVM with that. I'm using Clear Linux. |
|
The error is the same as I posted in #159. For your record, I did not compile llvm with turning on RTTI. |
|
@yonghong-song are you sure you're using the code from my fork? It seems you're using current master of iovisor/bpftrace by looking at the error message. |
|
I tested this with the following llvm build: and it builds.. but things don't run: These changes might be enough for llvm 7 (I haven't tested that yet), but don't seem enough for llvm latest. If the changes do fix llvm 7, then that's an improvement and we should merge them. Just need extra work for latest. |
|
@brendangregg please let me clean this branch up a bit and fix the tests first – I should have time to do this tomorrow |
|
I tested on llvm 5 & 6, seems ok. On llvm 7 it builds ok, and most things work, but this core dumps: stack: FWIW, I added these lines to /etc/apt/sources.list to get llvm 7: -d does print out debug info before core dumping: |
|
... and it is the args->bytes as the histogram value that is causing the crash. Other values work ok. |
|
Another case I found with LLVM 7: |
|
Looks like llvm 7 is doing extra int type checks, breaking passing the 32-bit args->bytes to hist() (or lhist(), sum(), etc). Here's how I fixed it (tested on llvm 7 and llvm 5): diff --git a/src/ast/codegen_llvm.cpp b/src/ast/codegen_llvm.cpp
index 40cf71a..e05cd83 100644
--- a/src/ast/codegen_llvm.cpp
+++ b/src/ast/codegen_llvm.cpp
@@ -164,6 +164,7 @@ void CodegenLLVM::visit(Call &call)
AllocaInst *newval = b_.CreateAllocaBPF(map.type, map.ident + "_val");
call.vargs->front()->accept(*this);
+ expr_ = b_.CreateIntCast(expr_, b_.getInt64Ty(), false); // promote int to 64-bit
b_.CreateStore(b_.CreateAdd(expr_, oldval), newval);
b_.CreateMapUpdateElem(map, key, newval);
@@ -183,6 +184,7 @@ void CodegenLLVM::visit(Call &call)
// elements will always store on the first occurrance. Revent this later when printing.
Function *parent = b_.GetInsertBlock()->getParent();
call.vargs->front()->accept(*this);
+ expr_ = b_.CreateIntCast(expr_, b_.getInt64Ty(), false); // promote int to 64-bit
Value *inverted = b_.CreateSub(b_.getInt64(0xffffffff), expr_);
BasicBlock *lt = BasicBlock::Create(module_->getContext(), "min.lt", parent);
BasicBlock *ge = BasicBlock::Create(module_->getContext(), "min.ge", parent);
@@ -207,6 +209,7 @@ void CodegenLLVM::visit(Call &call)
Function *parent = b_.GetInsertBlock()->getParent();
call.vargs->front()->accept(*this);
+ expr_ = b_.CreateIntCast(expr_, b_.getInt64Ty(), false); // promote int to 64-bit
BasicBlock *lt = BasicBlock::Create(module_->getContext(), "min.lt", parent);
BasicBlock *ge = BasicBlock::Create(module_->getContext(), "min.ge", parent);
b_.CreateCondBr(b_.CreateICmpSGE(expr_, oldval), ge, lt);
@@ -239,6 +242,7 @@ void CodegenLLVM::visit(Call &call)
Value *total_old = b_.CreateMapLookupElem(map, total_key);
AllocaInst *total_new = b_.CreateAllocaBPF(map.type, map.ident + "_val");
call.vargs->front()->accept(*this);
+ expr_ = b_.CreateIntCast(expr_, b_.getInt64Ty(), false); // promote int to 64-bit
b_.CreateStore(b_.CreateAdd(expr_, total_old), total_new);
b_.CreateMapUpdateElem(map, total_key, total_new);
b_.CreateLifetimeEnd(total_key);
@@ -250,6 +254,7 @@ void CodegenLLVM::visit(Call &call)
{
Map &map = *call.map;
call.vargs->front()->accept(*this);
+ expr_ = b_.CreateIntCast(expr_, b_.getInt64Ty(), false); // promote int to 64-bit
Function *log2_func = module_->getFunction("log2");
Value *log2 = b_.CreateCall(log2_func, expr_, "log2");
AllocaInst *key = getHistMapKey(map, log2);
@@ -285,6 +290,12 @@ void CodegenLLVM::visit(Call &call)
step_arg.accept(*this);
step = expr_;
+ // promote int to 64-bit
+ value = b_.CreateIntCast(value, b_.getInt64Ty(), false);
+ min = b_.CreateIntCast(min, b_.getInt64Ty(), false);
+ max = b_.CreateIntCast(max, b_.getInt64Ty(), false);
+ step = b_.CreateIntCast(step, b_.getInt64Ty(), false);
+
Value *linear = b_.CreateCall(linear_func, {value, min, max, step} , "linear");
AllocaInst *key = getHistMapKey(map, linear);The "promote int to 64-bit" comment is deliberate, so we can find these later (in case we rewrite that all in a better way). @ajor how's that look? @mlen, if this also seems reasonable to you, can you include the above and test? thanks! With #149 + the above diff, llvm 7 is working for me. CC @yonghong-song |
|
Looks good to me |
Apply Brendan's patch
|
@brendangregg everything seems to work just fine after applying your patch |
|
Great, thanks, I'll merge unless someone says not to. Yes, I know we need to cleanup the codegen tests later (which have to deal with different llvm versions, too)... |
This CL consists of the minimal number of changes to compile bpftrace on LLVM 7.
I checked few oneliners and they all appear to work fine, but codegen specs are broken (probably due to different LLVM version).
LLVM 7 API is incompatible with previous versions, so few ifdefs will be needed to support both.
To Do: