LLVM 4 #18
LLVM 4 #18
Conversation
… like so far it's line and file information, i.e diagnostic so..
…ysisUtil looks more like I might miss some diagnostic info
…vs X* in the node references, but let me take a look at this before rushing right in (I can do that this afternoon)
…arification of pointers or double pointers in WPASolver
…ngRef, inserting a few NodeRef, etc, and now compiling a fair amount of the project, but still more to go...
…e some new virtual methods in 4.0 aren't implemented...
…back to Yulei (with notes to help him...)
…ing the LLVMGetGlobalContext rather than getGlobalContext
…est should be pretty much ready to go
|
Jared, Thanks for your help! I am pretty busy these few days, and will start looking into your pull request from next week :) |
|
Hi, Jared. In overall, I think your code works well. wpa: Unknown command line argument '-svfg'. Try: 'wpa -help' Since I am not sure whether who will fix it, I leave my comment here. PS. nix7965 referenced this pull request 5 days ago -> Did I make a mistake something? Why that appears... |
|
I believe you need to apply Jared's patch on the most recent version of SVF. Try "git pull" before merging his patch. |
|
Let me know if you need anything. I don't recall changing an argument for wpa, but it might need a change with the AliasAnalysis change (requiring TargetLibraryInfo in any constructor call). |
|
From my experience, I applied "only" Jared's patch to the recent SVF. Then, it works. Also, there is some minor issue. I don't know why but SVF with @jcarlson23 's patch can be compiled only with "cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=true". |
| @@ -6,7 +6,7 @@ | |||
| // Copyright (C) <2013-2016> <Jingling Xue> | |||
|
|
|||
| // This program is free software: you can redistribute it and/or modify | |||
| // it under the terms of the GNU General Public License as published by | |||
| // it under the terms of the GNU Gg12eneral Public License as published by | |||
There was a problem hiding this comment.
oh wow, sorry, I can update that if need be.. obviously just a typo... :(
|
Merging this with current SVF master yields: dtzWill@ee5aead Which seems to work pretty well overall, haven't stressed it too much yet :). I'll note that #17 happens 100% of the time for me with these changes, please see my comment on that issue for a suggest workaround (or a discussion of why a workaround is not needed and I'm doing something wrong! ;)). Either way, probably worth being aware of. Thanks for the LLVM 4 port and of course thanks for SVF! 👍 |
|
Haha yeah no problem at all, just thought I'd point it out so it can be
fixed.
…On Tue, Apr 25, 2017, 8:17 AM Jared Carlson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In include/WPA/WPASolver.h
<#18 (comment)>:
> @@ -6,7 +6,7 @@
// Copyright (C) <2013-2016> <Jingling Xue>
// This program is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License as published by
+// it under the terms of the GNU Gg12eneral Public License as published by
oh wow, sorry, I can update that if need be.. obviously just a typo... :(
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#18 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAx4sqoeoaYcKSc7Pt6EQBtfT4xRQaCyks5rzfJVgaJpZM4M_0Gk>
.
|
|
I think #17 probably happens because the fix in master was after when I started the port to llvm-4, so I think that'll be solved with a proper merge... At least I hope so, but I'm not surprised by it, fwiw... |
|
That was my initial guess as well but AFAICT the issue happens on latest master (without your changes) but happens to not crash. As I mentioned on #17 running under valgrind shows it's doing the same invalid memory accesses but apparently they're not a problem for whatever reason. We'll see what the SVF folks say though, and regardless it'd be good to have some of this confirmed by others :). |
|
Hopefully, I will start testing Jared's patch from tomorrow:) |
| @@ -51,6 +51,12 @@ set(SOURCES | |||
| add_llvm_loadable_module(Svf ${SOURCES}) | |||
| add_llvm_Library(LLVMSvf ${SOURCES}) | |||
|
|
|||
| # It seems that the Svf library needs to explictly require the CUDD alloc/init/etc so LLVMCudd | |||
There was a problem hiding this comment.
The newly added part causes an error when linking "Svf.so" in my ubuntu machine (completely ok on mac), see below:
/usr/bin/ld: CUDD/libLLVMCudd.a(cuddInit.c.o): relocation R_X86_64_32S against `Cudd_OutOfMem' can not be used when making a shared object; recompile with -fPIC
CUDD/libLLVMCudd.a: error adding symbols: Bad value
If I delete those five lines, it will be all good on ubuntu, but causing a linking error on mac os ;(
If possible, could you please take a look at it?
There was a problem hiding this comment.
Sure, this ought to be straightforward I think. I just moved computers but let me create a VM to test in and put in a architecture check and I think this can be resolved... (I'll try to update tonight).
There was a problem hiding this comment.
I believe I have a fix but have been in VM purgatory today so but basically just used CMAKE_SYSTEM_NAME and a little if-else.. so just will test again, today got away from me but this is the easy part...
There was a problem hiding this comment.
Alright, this built for me on Ubuntu16.04 LT with the following changes:
I can reissue the pull request if that's easier but this is trivial so I'm going to focus on the iterator issue below.
| @@ -189,7 +189,7 @@ bool SymbolTableInfo::computeGepOffset(const llvm::User *V, LocationSet& ls) { | |||
| // Handling array types, skipe array handling here | |||
| // We treat whole array as one, then we can distinguish different field of an array of struct | |||
| // e.g. s[1].f1 is differet from s[0].f2 | |||
| if(isa<ArrayType>(*gi)) | |||
| if(isa<ArrayType>(gi.getIndexedType())) | |||
There was a problem hiding this comment.
We found that the change made from "*gi" to "gi.getIndexedType" causes quite a few of our test cases failed.
The llvm-4.0.0 says "gi.getIndexedType" is the same as "operator*() " (see below in llvm-4.0.0/include/llvm/IR/GetElementPtrTypeItertor.h)
// FIXME: Make this the iterator's operator*() after the 4.0 release.
// operator*() had a different meaning in earlier releases, so we're
// temporarily not giving this iterator an operator*() to avoid a subtle
// semantics break.
However, I found the semantic of get_type_iterator was actually changed in LLVM-4.0 against LLVM-3.8.0. They are not semantic equivalent anymore.
Suppose we have a LLVM Value V
%arrayidx3 = getelementptr inbounds [2 x %struct.MyStruct], [2 x %struct.MyStruct]* %s, i64 0, i64 %idxprom, !dbg !67
if we use gep_type_iterator to iterate the types of the above value
for (gep_type_iterator gi = gep_type_begin(*V), ge = gep_type_end(*V); gi != ge; ++gi) {
(*gi)->dump() // in llvm-3.8
gi.getIndexedType()->dump() // in llvm 4.0
gi.getOperand()->dump()
}
We will get two elements (two types), but different results with respect to different llvm versions, as follows:
llvm-3.8.0
[2 x %struct.MyStruct]*
i64 0
[2 x %struct.MyStruct]
%idxprom = sext i32 %0 to i64, !dbg !67
llvm-4.0.0
[2 x %struct.MyStruct]
i64 0
%struct.MyStruct = type { i32*, i32* }
%idxprom = sext i32 %0 to i64, !dbg !32
@jcarlson23
@dtzWill
Any idea on how to get the type of an operand instead of its container type using type iterator (or using some other ways)?
I don't prefer to change the code in the memoryModel.
Thanks
There was a problem hiding this comment.
Hmm... A bit trickier than I had hoped :(
So just to make sure I understand the issue is the Operand's type? So in your code example
(*gi)->dump() is equivalent to gi.getIndexedType()->dump() from 3.8 to 4.0, but in the iterator the getOperand() in 4.0 is now pointing to the container's type rather than directly to the operand's type?
From what I saw in the code, I agree that changing the memory model would be very low in my preferences... Let me dig in a bit later tonight... See if we can't find a better solution.
There was a problem hiding this comment.
(*gi)->dump() are not equivalent to gi.getIndexedType()->dump() in 4.0 now.
Yes, it needs to be the operand's type rather than its container's
There was a problem hiding this comment.
Hmm, darn. For what it's worth, here's a bit of discussion about the commit that caused this functionality change:
https://reviews.llvm.org/D26594
Maybe the information there will help understand the changes and how to accommodate them properly.
I've run into this while migrating another project to 4.0 but haven't had time to sort out the changes sorry :(.
There was a problem hiding this comment.
Oh cool, thanks @dtzWill ! I was offline for a while today so I didn't get a chance to look at the commit/discussion log.
I'll dig in a bit more here.. If I get stuck I'll ask around or post to the mailing list in a day or so...
There was a problem hiding this comment.
@dtzWill Yeah, thanks, I did that and still had some serious issues... Some of it, I think, is just the mess of Xcode and multiple llvm builds. Also I think some of the expected directories coming out of the build I had renamed... eh.. it's a new machine, I kind of dumped some of the old apps/tools in and added some new and haven't quite had time to sort the cruft, but I'll get there.
I'm curious what you mean regarding your last paragraph. Are you referring to building a CFG based model to dynamically check pointers? Something akin to CFI (Control Flow Integrity) but with more analytical depth? Do you care how the source might be altered/appended to, something like Seahorn or Pagai? Anyway, just genuinely curious as to what you're after there...
As this is starting to get off tangent, perhaps email is best, but whatever works... Thanks again.
There was a problem hiding this comment.
I should have said, I'll give the NIX stuff a crack as that help me clean up what I've got going, thanks for sharing!
There was a problem hiding this comment.
Jared,
Alternatively, I've also attached my scripts to build and setup SVF (llvm-4.0). Hopefully, it will be helpful for your to test bridgedGepIterator against PTABen.
You PR is behind the master repo. For a clean merge, could you please give a new PR after passing all the micro-benchmarks using your bridgedGepIteraor?
There was a problem hiding this comment.
@rockysui BridgedGepIterator it is! I'm going to close this PR out then and work locally for a few days and get you a new one. I'll aim for Monday, using your scripts or the NIX setup from Will to right my testing setup...
See ongoing discussion for context: SVF-tools#18 Mostly I'm just curious what this does to results on our programs, this is likely not the right thing to do.
Optimise node retrieval in CHG.
Refactor boilerplate code of disjunctive predicates of "isa()"
This should be relatively easy to integrate - although I would put this into a separate (a staging or local) branch and then merge back into the GitHub master from there. The WPAPass no longer inheriting from AliasAnalysis is probably the place I would suggest to be looked at the most. The "SVF Alterations.rtf" gives a summary of the changes, etc...
Let me know what else I can do to help. I've gotta move on to a few other things the rest of the week but next week I'm going to start outline a few pivots I'm hoping to expand on :-)