Support for LLVM 3.8 #900

jirislaby · 2018-05-24T13:25:51Z

The story never ends...

kren1 · 2018-05-24T13:56:51Z

.travis/install-llvm-and-runtime-compiler.sh

-  sudo apt-get install -y llvm-${LLVM_VERSION}-tools clang-${LLVM_VERSION}
+  sudo apt-get install -y clang-${LLVM_VERSION}
+  if [ "${LLVM_VERSION}" != "3.8" ]; then
+      sudo apt-get install -y llvm-${LLVM_VERSION}-tools


Why is llvm-3.8-tools not needed anymore?

Hmm, this was needed before we started using llvm-toolchain repos. It should not be needed now. I will try to drop it.

For 3.8, this is still needed, because:

Some packages could not be installed. This may mean that you have requested an impossible situation or if you are using the unstable distribution that some required packages have not yet been created or been moved out of Incoming. The following information may help to resolve the situation: The following packages have unmet dependencies: llvm-3.8-tools : Depends: llvm-3.8-dev (= 1:3.8~svn278388-1~exp1) but 1:3.8-2ubuntu3~trusty5 is to be installed E: Unable to correct problems, you have held broken packages.

I see, thanks for having a look. Do you know if it's needed at all, considering llvm-3.8 works without it?

@kren1 llvm-tools contains FileCheck and not, so you we need for testing.

So this means no testing for llvm-3.8? I guess for those two programs we could use an different llvm-tools version?

But tests are run and succeed AFAICT. So the tools are maybe in different package in 3.8?

Actually, FileCheck and not are downloaded if not available, right?

@jirislaby Yes, exactly. And we can either use lit provided with LLVM or the python package.

So what't the solution here? I feel like we don't need to special case this for 3.8.

kren1 · 2018-05-24T14:24:28Z

lib/Core/Executor.cpp

@@ -2210,8 +2210,13 @@ void Executor::executeInstruction(ExecutionState &state, KInstruction *ki) {
        !fpWidthToSemantics(right->getWidth()))
      return terminateStateOnExecError(state, "Unsupported FRem operation");
    llvm::APFloat Res(*fpWidthToSemantics(left->getWidth()), left->getAPValue());
+#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 8)
+    Res.mod(
+        APFloat(*fpWidthToSemantics(right->getWidth()), right->getAPValue()));


The rounding was removed because it was never needed: llvm-mirror/llvm@ff278be

codecov · 2018-05-24T14:32:47Z

Codecov Report

Merging #900 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
- Coverage   65.91%   65.91%   -0.01%     
==========================================
  Files         141      141              
  Lines       16152    16154       +2     
  Branches     3739     3739              
==========================================
+ Hits        10647    10648       +1     
- Misses       3816     3817       +1     
  Partials     1689     1689

Flag	Coverage Δ
#systemtests	`65.16% <0%> (-0.01%)`	⬇️
#unittests	`10.74% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
lib/Core/Executor.cpp	`75.7% <ø> (ø)`	⬆️
lib/Module/IntrinsicCleaner.cpp	`87.87% <ø> (ø)`	⬆️
lib/Core/StatsTracker.cpp	`87.19% <ø> (ø)`	⬆️
lib/Module/Optimize.cpp	`88.28% <ø> (ø)`	⬆️
lib/Module/ModuleUtil.cpp	`60.48% <ø> (ø)`	⬆️
lib/Module/LowerSwitch.cpp	`77.77% <ø> (ø)`	⬆️
tools/klee/main.cpp	`64.22% <0%> (-0.19%)`	⬇️
lib/Core/AddressSpace.cpp	`58.33% <0%> (+0.59%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3eece12...9d054d6. Read the comment docs.

kren1 · 2018-05-24T14:38:09Z

lib/Module/IntrinsicCleaner.cpp

@@ -110,8 +114,12 @@ bool IntrinsicCleanerPass::runOnBasicBlock(BasicBlock &b, Module &M) {
      case Intrinsic::uadd_with_overflow:
      case Intrinsic::usub_with_overflow:
      case Intrinsic::umul_with_overflow: {
+#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 8)
+        // ctor needs the iterator, but we already increased ours
+        IRBuilder<> builder(ii->getParent(), i_);


Am I missing something, but I can't see why this would be necessary in 3.8, the constructros doesn't seem to have changed?

kren1 · 2018-05-24T14:44:34Z

lib/Module/Optimize.cpp

@@ -102,7 +102,12 @@ static void AddStandardCompilePasses(klee::LegacyLLVMPassManagerTy &PM) {
  addPass(PM, createCFGSimplificationPass());    // Clean up after IPCP & DAE

  addPass(PM, createPruneEHPass());              // Remove dead EH info
-  addPass(PM, createFunctionAttrsPass());        // Deduce function attrs
+#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 8)
+  addPass(PM, createPostOrderFunctionAttrsPass());


Seems to be a refactoring in llvm-mirror/llvm@e96fb9a

kren1 · 2018-05-24T14:44:56Z

lib/Module/Optimize.cpp

+#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 8)
+    addPass(Passes, createPostOrderFunctionAttrsPass());
+    addPass(Passes, createReversePostOrderFunctionAttrsPass());
+    // addPass(Passes, createGlobalsAAWrapperPass());


Stray comment.

kren1 · 2018-05-24T14:50:36Z

tools/klee/main.cpp

-      raw_svector_ostream ds(d); ds << i; ds.flush();
+      raw_svector_ostream ds(d);
+      ds << i;
+// SmallString is always up-to-date, no need to flush. See Support/raw_ostream.h


Done in llvm-mirror/llvm@d4177b2

ccadar · 2018-06-29T10:21:11Z

@jirislaby , what's the status with this one? I guess it's ready for a rebase?

MartinNowack · 2018-07-09T15:21:05Z

@jirislaby Do you have time to rebase your patch? Otherwise I can try to do it.

jirislaby · 2018-07-09T15:52:37Z

@MartinNowack coincidently I have been working on this today. It was tough due to rework in ef90f1e but it's done.

MartinNowack · 2018-07-09T15:54:35Z

@jirislaby Fantastic!! I assumed there was some pain involved - sorry for that.

ccadar · 2018-07-09T21:17:22Z

@MartinNowack , do you know why the Travis logs are so verbose since the switch to Docker? It would be nice to reduce verbosity a bit if possible.

MartinNowack · 2018-07-09T21:46:53Z

@ccadar Yes, that't true - but that is only when they are building LLVM. Otherwise it should be much shorter.

ccadar · 2018-07-10T21:39:09Z

But why was LLVM built? I thought we don't do this. Also, what was the source of the initial Travis CI failures? (I see you restarted it and it worked the second time)

MartinNowack · 2018-07-10T23:20:59Z

@ccadar The llvm images were not built/rebuild to fix a bug with the coverage.
The original source of the Travis CI build was a timeout of the build it's restricted to 50min (and cannot be extended). The actual run can be extended.

MartinNowack

@jirislaby I like the changes.
Please:

fix my minor comments
rebase
merge commits

Beside that, I think we are ready to go.

MartinNowack · 2018-07-12T15:33:20Z

lib/Module/ModuleUtil.cpp

@@ -162,8 +162,12 @@ static bool linkTwoModules(llvm::Module *Dest,
 #if LLVM_VERSION_CODE >= LLVM_VERSION(3, 6)


This needs to be rearranged as the error message is not propagated anymore.
Therefore we have to generate our own.

#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 8) // Get the potential error message (Src is moved and won't be available later) errorMsg = "Linking module " + src->getModuleIdentifier() + " failed"; auto linkResult = Linker::linkModules(*Dest, std::move(Src)); #else if LLVM_VERSION_CODE >= LLVM_VERSION(3,6) ...

MartinNowack · 2018-07-12T15:50:48Z

lib/Module/ModuleUtil.cpp

-        std::error_code ec = memberNameErr.getError();
+	std::error_code ec;
+#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 8)
+	ErrorOr<object::Archive::Child> childErr = *AI;


Use auto if possible.
I would prefere childOrErr to indicate that it contains a child or an error.

MartinNowack · 2018-07-12T15:52:45Z

lib/Module/ModuleUtil.cpp

+		return false;
+	}
+#else
+	object::Archive::child_iterator childErr = AI;


auto as well.

The rounding was removed because it was never needed: llvm-mirror/llvm@ff278be Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

createFunctionAttrsPass was split to createPostOrderFunctionAttrsPass and createReversePostOrderFunctionAttrsPass in LLVM commit e96fb9ab15d4. createGlobalsModRefPass was changed to createGlobalsAAWrapperPass in LLVM commit 9146833fa313. Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

No need to flush it, see llvm-mirror/llvm@d4177b2 Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

jirislaby · 2018-07-12T17:08:40Z

@MartinNowack rebaed and fixed. What do you mean by merging commits? You mean squash them together? But as we did for llvm 3.7, the multiple commits are easier to follow (even in history).

MartinNowack · 2018-07-12T17:14:30Z

@jirislaby Yes, i meant merging them but looking at your messages. I'm fine with it.

MartinNowack · 2018-07-12T17:21:08Z

@jirislaby
Sorry, typo - capital S.

"Linking module " + src->getModuleIdentifier() + " failed";

Should be:

 "Linking module " + Src->getModuleIdentifier() + " failed";

LLVM commit d912be98f8eb changed the prototype of linkModules to accept std::unique_ptr. Adapt to that. Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

After LLVM commit 25569fdcdab0, archive iterator returns object::Archive::Child instead of child_iterator, adapt to that. Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

LLVM commit eac309550f25 removed implicit iterator conversions. So we have to get the iterators explicitly now. Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

alias in LLVM 3.8 has a new format, it adds an AliaseeTy parameter. So handle this in the tests. [v2] add comments about what was changed and why Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

MartinNowack · 2018-07-12T22:25:42Z

@jirislaby Thanks a lot for your hard work. The zero change in coverage for diff is expected as LLVM 3.8 code is not exercised for coverage runs. Reduction in general is also expected, new code is added but additional code not tested.
Anyway, first coreutils tests run well. Let's give it a go. Going to merge.

kren1 reviewed May 24, 2018

View reviewed changes

jirislaby force-pushed the llvm_38 branch 2 times, most recently from e7100fa to 41c0eb6 Compare June 15, 2018 13:38

ccadar changed the title ~~support for llvm 3.8~~ Support for LLVM 3.8 Jun 29, 2018

ccadar added the LLVM upgrade label Jun 29, 2018

jirislaby force-pushed the llvm_38 branch from 41c0eb6 to 8b536da Compare July 9, 2018 15:46

jirislaby force-pushed the llvm_38 branch from 8b536da to 5ecd1f8 Compare July 9, 2018 16:22

MartinNowack requested changes Jul 12, 2018

View reviewed changes

jirislaby added 3 commits July 12, 2018 18:37

llvm38: no rounding in APFloat

834f9c9

The rounding was removed because it was never needed: llvm-mirror/llvm@ff278be Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

llvm38: SmallString is always up-to-date

e880b5c

No need to flush it, see llvm-mirror/llvm@d4177b2 Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

jirislaby force-pushed the llvm_38 branch from 5ecd1f8 to 46f50ea Compare July 12, 2018 17:06

llvm38: adapt to new Linker::linkModules

a2dff4d

LLVM commit d912be98f8eb changed the prototype of linkModules to accept std::unique_ptr. Adapt to that. Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

jirislaby force-pushed the llvm_38 branch from 46f50ea to cd246f8 Compare July 12, 2018 20:19

jirislaby and others added 4 commits July 12, 2018 22:26

llvm38: archive child iterator changes

3c19b31

After LLVM commit 25569fdcdab0, archive iterator returns object::Archive::Child instead of child_iterator, adapt to that. Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

llvm38: no more implicit iterators

fad40b6

LLVM commit eac309550f25 removed implicit iterator conversions. So we have to get the iterators explicitly now. Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

llvm38: test, change some tests

be382d5

alias in LLVM 3.8 has a new format, it adds an AliaseeTy parameter. So handle this in the tests. [v2] add comments about what was changed and why Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

travis CI: add LLVM 3.8 build tests

9d054d6

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

jirislaby force-pushed the llvm_38 branch from cd246f8 to 9d054d6 Compare July 12, 2018 20:26

MartinNowack approved these changes Jul 12, 2018

View reviewed changes

MartinNowack merged commit 79ac709 into klee:master Jul 12, 2018

jirislaby deleted the llvm_38 branch July 13, 2018 06:44

jbuening mentioned this pull request Jul 20, 2018

LLVM 3.8 support #385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for LLVM 3.8 #900

Support for LLVM 3.8 #900

jirislaby commented May 24, 2018

kren1 May 24, 2018

jirislaby May 24, 2018

jirislaby May 26, 2018

kren1 May 26, 2018

MartinNowack May 26, 2018

kren1 May 26, 2018

jirislaby Jun 15, 2018

jirislaby Jun 15, 2018

MartinNowack Jun 15, 2018

kren1 Jun 15, 2018

kren1 May 24, 2018

codecov bot commented May 24, 2018 •

edited

Loading

kren1 May 24, 2018

kren1 May 24, 2018

kren1 May 24, 2018

kren1 May 24, 2018

ccadar commented Jun 29, 2018

MartinNowack commented Jul 9, 2018

jirislaby commented Jul 9, 2018

MartinNowack commented Jul 9, 2018

ccadar commented Jul 9, 2018

MartinNowack commented Jul 9, 2018

ccadar commented Jul 10, 2018

MartinNowack commented Jul 10, 2018

MartinNowack left a comment

MartinNowack Jul 12, 2018

MartinNowack Jul 12, 2018

MartinNowack Jul 12, 2018

jirislaby commented Jul 12, 2018

MartinNowack commented Jul 12, 2018

MartinNowack commented Jul 12, 2018

MartinNowack commented Jul 12, 2018

		@@ -162,8 +162,12 @@ static bool linkTwoModules(llvm::Module *Dest,
		#if LLVM_VERSION_CODE >= LLVM_VERSION(3, 6)

Support for LLVM 3.8 #900

Support for LLVM 3.8 #900

Conversation

jirislaby commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccadar commented Jun 29, 2018

MartinNowack commented Jul 9, 2018

jirislaby commented Jul 9, 2018

MartinNowack commented Jul 9, 2018

ccadar commented Jul 9, 2018

MartinNowack commented Jul 9, 2018

ccadar commented Jul 10, 2018

MartinNowack commented Jul 10, 2018

MartinNowack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirislaby commented Jul 12, 2018

MartinNowack commented Jul 12, 2018

MartinNowack commented Jul 12, 2018

MartinNowack commented Jul 12, 2018

codecov bot commented May 24, 2018 •

edited

Loading