My apologies @asbirlea : I must resign from review on this patch as my current employer hasn't authorized me to contribute to LLVM.

I must abandon this revision as my current employer has not authorized me to contribute to LLVM.

Hello @skatkov , I'm sorry to hear you're dealing with an intermittent crash. If you find a reproducible .ll file you can feed into opt -jump-threading -S < input.ll that crashes I would be interested in examining it. The Deferred usage of DTU causes unexpected issues and it is entirely possible there is still a bug here.

Please also add a test-case showing the failure.

Maybe it makes sense to change the TryToSimplifyUncondBranchFromEmptyBlock API so it doesn't erase the basic block, then call DeleteDeadBlock after we've cleaned up LVI etc.?

Depending on the number of call-sites of TryToSimplifyUncondBranchFromEmptyBlock this may be the way to go.

LGTM as well.

LGTM. I'd still like to hear if @kuhar has any objections.

@aemerson This patch resolves the compiler crash I reported in rL367075. Thank you for the fix.

@aemerson , this commit causes a compiler crash when compiling libcxx for aarch64. Specifically, the file filesystem/operations.cpp:

FAILED: src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o
/work/nightly/bisect/bin/aarch64-linux-clang++  -DNDEBUG -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_HAS_COMMENT_LIB_PRAGMA -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Iinclude/c++build -I/tmp/tmp.vruMese6eR/src/libcxx/include --target=aarch64-unknown-linux-gnu --sysroot=/work/nightly/bisect/aarch64-unknown-linux-gnu/sysroot --gcc-toolchain=/work/nightly/bisect/bin -fPIC   --target=aarch64-unknown-linux-gnu --sysroot=/work/nightly/bisect/aarch64-unknown-linux-gnu/sysroot --gcc-toolchain=/work/nightly/bisect/bin -DLIBCXX_BUILDING_LIBCXXABI -std=c++11 -nostdinc++ -fvisibility-inlines-hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wno-user-defined-literals -Wno-covered-switch-default -Wno-ignored-attributes -Wno-error -MD -MT src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -MF src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o.d -o src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -c /tmp/tmp.vruMese6eR/src/libcxx/src/filesystem/operations.cpp
clang-10: /tmp/tmp.vruMese6eR/src/llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp:2040: virtual bool (anonymous namespace)::AArch64InstructionSelector::select(llvm::MachineInstr &, llvm::CodeGenCoverage &) const: Assertion `(*RBI.getRegBank(DefReg, MRI, TRI)).getID() == AArch64::GPRRegBankID && "Unexpected ext regbank"' failed.
Stack dump:
0.      Program arguments: /work/nightly/bisect/bin/clang-10 -cc1 -triple aarch64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -main-file-name operations.cpp -mrelocation-model pic -pic-level 2 -mthread-model posix -mframe-pointer=all -fmath-errno -masm-verbose -mconstructor-aliases -fuse-init-array -target-cpu generic -target-feature +neon -target-abi aapcs -fallow-half-arguments-and-returns -dwarf-column-info -debugger-tuning=gdb -coverage-notes-file /tmp/tmp.vruMese6eR/build/libcxx-shared/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.gcno -nostdinc++ -resource-dir /work/nightly/bisect/lib/clang/10.0.0 -dependency-file src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o.d -sys-header-deps -MT src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -cxx-isystem /work/nightly/bisect/aarch64-unknown-linux-gnu/include/c++/8.3.0 -cxx-isystem /work/nightly/bisect/aarch64-unknown-linux-gnu/include/c++/8.3.0/aarch64-unknown-linux-gnu -D _GLIBCXX_INCLUDE_NEXT_C_HEADERS -D NDEBUG -D _LIBCPP_BUILDING_LIBRARY -D _LIBCPP_HAS_COMMENT_LIB_PRAGMA -D _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -I include/c++build -I /tmp/tmp.vruMese6eR/src/libcxx/include -D LIBCXX_BUILDING_LIBCXXABI -isysroot /work/nightly/bisect/aarch64-unknown-linux-gnu/sysroot -internal-isystem /work/nightly/bisect/aarch64-unknown-linux-gnu/sysroot/usr/local/include -internal-isystem /work/nightly/bisect/lib/clang/10.0.0/include -internal-externc-isystem /work/nightly/bisect/aarch64-unknown-linux-gnu/sysroot/include -internal-externc-isystem /work/nightly/bisect/aarch64-unknown-linux-gnu/sysroot/usr/include -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wno-user-defined-literals -Wno-covered-switch-default -Wno-ignored-attributes -Wno-error -std=c++11 -fdeprecated-macro -fdebug-compilation-dir /tmp/tmp.vruMese6eR/build/libcxx-shared -ferror-limit 19 -fmessage-length 0 -fvisibility-inlines-hidden -fno-signed-char -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -faddrsig -o src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -x c++ /tmp/tmp.vruMese6eR/src/libcxx/src/filesystem/operations.cpp
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module '/tmp/tmp.vruMese6eR/src/libcxx/src/filesystem/operations.cpp'.
4.      Running pass 'InstructionSelect' on function '@_ZNSt3__16chrono8durationInNS_5ratioILl1ELl1EEEEC2IlEERKT_PNS_9enable_ifIXaasr14is_convertibleIS6_nEE5valueooL_ZNS_17integral_constantIbLb0EE5valueEEntsr23treat_as_floating_pointIS6_EE5valueEvE4typeE'
 #0 0x00007f177532f954 PrintStackTraceSignalHandler(void*) (/work/nightly/bisect/lib/libLLVMSupport.so.10svn+0x180954)
 #1 0x00007f177532d63e llvm::sys::RunSignalHandlers() (/work/nightly/bisect/lib/libLLVMSupport.so.10svn+0x17e63e)
 #2 0x00007f177532fc08 SignalHandler(int) (/work/nightly/bisect/lib/libLLVMSupport.so.10svn+0x180c08)
 #3 0x00007f17796c2890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #4 0x00007f17727ade97 raise /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #5 0x00007f17727af801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
 #6 0x00007f177279f39a __assert_fail_base /build/glibc-OTsEL5/glibc-2.27/assert/assert.c:89:0
 #7 0x00007f177279f412 (/lib/x86_64-linux-gnu/libc.so.6+0x30412)
 #8 0x00007f177ca9206f (anonymous namespace)::AArch64InstructionSelector::select(llvm::MachineInstr&, llvm::CodeGenCoverage&) const (/work/nightly/bisect/lib/libLLVMAArch64CodeGen.so.10svn+0x12a06f)
 #9 0x00007f17721bfe13 llvm::InstructionSelect::runOnMachineFunction(llvm::MachineFunction&) (/work/nightly/bisect/lib/libLLVMGlobalISel.so.10svn+0x4ae13)
#10 0x00007f177a49440a llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/work/nightly/bisect/lib/libLLVMCodeGen.so.10svn+0x22840a)
#11 0x00007f17769f0e43 llvm::FPPassManager::runOnFunction(llvm::Function&) (/work/nightly/bisect/lib/libLLVMCore.so.10svn+0x1e1e43)
#12 0x00007f17769f1153 llvm::FPPassManager::runOnModule(llvm::Module&) (/work/nightly/bisect/lib/libLLVMCore.so.10svn+0x1e2153)
#13 0x00007f17769f17bd llvm::legacy::PassManagerImpl::run(llvm::Module&) (/work/nightly/bisect/lib/libLLVMCore.so.10svn+0x1e27bd)
#14 0x00007f17788ac45c clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/work/nightly/bisect/lib/libclangCodeGen.so.10svn+0x9445c)
#15 0x00007f1778b63de1 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/work/nightly/bisect/lib/libclangCodeGen.so.10svn+0x34bde1)
#16 0x00007f176f72cef3 clang::ParseAST(clang::Sema&, bool, bool) (/work/nightly/bisect/lib/libclangParse.so.10svn+0x2eef3)
#17 0x00007f17781224f8 clang::FrontendAction::Execute() (/work/nightly/bisect/lib/libclangFrontend.so.10svn+0xe84f8)
#18 0x00007f17780bd988 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/work/nightly/bisect/lib/libclangFrontend.so.10svn+0x83988)
#19 0x00007f1777e3708c clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/work/nightly/bisect/lib/libclangFrontendTool.so.10svn+0x408c)
#20 0x0000000000410d5c cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/work/nightly/bisect/bin/clang-10+0x410d5c)
#21 0x000000000040eae4 main (/work/nightly/bisect/bin/clang-10+0x40eae4)
#22 0x00007f1772790b97 __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:344:0
#23 0x000000000040bd9a _start (/work/nightly/bisect/bin/clang-10+0x40bd9a)
clang-10: error: unable to execute command: Aborted (core dumped)
clang-10: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 10.0.0 (/sarc-c/compiler/git/tools/llvm-project.git c07fe307b48a98a149578948b167802f7b2825cf)
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /work/nightly/bisect/bin
clang-10: note: diagnostic msg: PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
clang-10: note: diagnostic msg:
********************

Thank you @aemerson , the fix in rL366943 seems to have prevented crashes when compiling libcxx for aarch64. I successfully compiled a nightly with sha f181dd99cf1c4e .

Hi @Petar.Avramovic , I replied to your testing query in D65079. Thank you again for looking into this.

Hello @Petar.Avramovic , I have verified this patch on a recent sha 510e6fadaae95a41bee0eb99d84a146efc612365 does not crash clang when compiling the libcxx shared library. Thank you for identifying the source of the crash.

Hello @fhahn , it looks like more tests are failing on tip 7df225dfc25adc8371188dc57f3adf300b0bd697 when I tested on x86_64:

Failing Tests (4):
    LLVM :: CodeGen/X86/retpoline.ll
    LLVM :: CodeGen/X86/speculative-load-hardening-indirect.ll
    LLVM :: Transforms/ConstProp/basictest.ll
    LLVM :: Transforms/IndirectBrExpand/basic.ll

LGTM. I am assuming this passed test-suite and ninja check runs?

Hello @Petar.Avramovic , this commit is causing a compiler assert when I perform the following:

Hello @SjoerdMeijer , thank you for the review.

Hello @smeenai , this commit causes LLVM to fail to build when set set -D BUILD_SHARED_LIBS=ON. Here is the failing CMake and Ninja invocation:

/tools/build/cmake-3.14.5/bin/cmake \
    -G Ninja  \
    -D CMAKE_MAKE_PROGRAM=/tools/build/ninja-1.9.0/ninja \
    -D CMAKE_C_COMPILER=/usr/lib/llvm-7/bin/clang \
    -D CMAKE_CXX_COMPILER=/usr/lib/llvm-7/bin/clang++ \
    -D CMAKE_BUILD_TYPE=Release \
    -D CMAKE_INSTALL_PREFIX=/work/upstream/install \
    -D LLVM_TOOL_CLANG_BUILD=ON \
    -D LIBCXX_INCLUDE_BENCHMARKS=ON \
    -D BUILD_SHARED_LIBS=ON \
    -D LLVM_ENABLE_ASSERTIONS=ON \
    -D LLVM_TARGETS_TO_BUILD=X86 \
    /work/upstream/llvm-project/llvm

LGTM.

Hi @efriedma, comments are inline.

Abandoning for @efriedma 's patch in D63913.

Hi @efriedma , thank you for debugging and identifying the true root cause of PR42085. My comments are inline.

How do we actually get a miscompile?

The new test-case below, pr42085-loopheader-detection.ll exhibits the miscompile on unpatched tip LLVM. In PR42085 I've attached a test harness which runs the code using a simple main.c and a shell script. The main.c looks like:

#include <libgen.h>
#include <stdio.h>
#include <stdlib.h>

In D63629#1557870, @xbolva00 wrote:

Yes, FindFunctionBackedges. But not sure if it would not introduce new regressions..

Yes, that's my concern as well. The way that JumpThreading uses FindFunctionBackedges() is a bit unusual as a proxy for detecting loop headers without dominators or loopinfo. I'm unsure if alternate forms of loopheader detection using this analysis can remove the need to eliminate these invalid edges in F. The core problem of this IR shape is JT's analysis fails to detect the true loop headers and eventually we thread across one.

In D63629#1557847, @xbolva00 wrote:

Maybe you can use some reasonable BB size threshold to fix these regressions ?

Passes ninja check and test-suite compilation and lit execution. Compile time for some benchmarks improves for some benchmarks:

In D63629#1552868, @lebedev.ri wrote:

Folding conditional terminator instructions for basic blocks that are also loop headers can cause miscompiled code emitted from JumpThreading.

Please can you write a more detailed explanation of the problem in the patch description.
"cause miscompiled code emitted" is vague.

This code passes ninja check and a full test-suite run.

Thank you @asbirlea for the review. I'll rebase, verify everything looks ok, and commit. Hopefully in the next 7-10 days based on my current work load.

LGTM (but I'm not listed as a reviewer...)

LGTM. The number of if defined() and duplicated code regions is minimal. I'm curious to know what @vitalybuka and @kcc think of this iteration of the patch.

@sebpop welcome back! I'm glad you received clearance to work on this. :) My comments are inline.

Hello @vitalybuka , I have uploaded a WIP diff in D61766 and I appreciate any insight you can give.

Hi @vitalybuka , I am attempting to use D61401 as you suggested but have run into an issue regarding use1 used inside static member functions. This error persists regardless if I attempt to make it a member variable or a global variable:

/work/b.rzycki/upstream/llvm-project/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_primary_pair.h:76:9: error: invalid use of member 'use1' in static member function
    if (use1)
        ^~~~
/work/b.rzycki/upstream/llvm-project/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_primary_pair.h:82:9: error: invalid use of member 'use1' in static member function
    if (use1)
        ^~~~

Hello @vitalybuka , thank you for the example in D61401. Do you have a suggestion what file and namespace the DoubleAllocator template class definition should reside? The most recent diff for D60243 was authored April 3 and no longer cleanly applies (the file lsan_allocator.h has changed considerably since then). This is complicated by @sebpop changing jobs and requiring legal approval before he can help work on this again. In order to make progress I'm essentially re-writing this patch based on the new Allocator layout in lsan_allocator.h.

In D60243#1480602, @vitalybuka wrote:

I've started some refactoring there. So maybe it would be easier after that.

+1 to this patch. It fixes our nightly cross-compilers for arm and aarch64.

Rebase to tip. Reviews or comments are very much appreciated.

Hi @kcc. @sebpop and I decided on this approach because we were unable to create a base allocator class. We ran into pointer coercion issues for 32-bit or 64-bit pointers returned from some of the methods. We either had to mask this with (void *) casts at all the callsites. Even if we did this every routine would require a runtime check for 32 vs 64 on aarch64. This latest patch reduces the number of ifdefs and is still a compile-time check for every non-aarch64 platform.

Added missing check to testcase ensuring %bb1 threads to %bb7 when %c is false.

Address review comments.

I think the changes are minimal and make sense given the define() blocks for other arches. I'll defer to @kcc on approval though.

Passes make check and test-suite runs.

Thank you @sbc100 for the fast fix. I've verified my internal compiler builds succeed with tip + this patch.

Hello @sbc100, I'm seeing a build break caused by D60005 when I attempt to do the following:

1. compile llvm for aarch64 with a gnu toolchain sysroot
2. use the freshly built llvm aarch64 compiler to build libcxxabi aarch64 before building libcxx aarch64

Rebase patchset to tip.

Thank you @StephenTozer , LGTM.

In D58963#1421264, @StephenTozer wrote:

In D58963#1419867, @dblaikie wrote:

The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?

Yes; the second case seemed like it would need an additional, more complicated IR case to test, but the logic for performing the conditional-to-unconditional branch conversion is identical in both cases (could be extracted to a utility function perhaps?) and the semantic meaning is the same as well: "The condition at this state is guaranteed to be true/false, so remove the condition check".

Bump. Reviews or feedback would be appreciated.

LGTM. It's a simple and clean change.

Thank you for helping debug this @cryptoad .

In D58184#1414009, @cryptoad wrote:

Cool. Feel free to send it out if you want, or I'll do it when I am done with my current CL.
For the RHEL I thought linking against librt would do, but I don't have a way to debug it locally fast.

Good news: the patch fixes the RHEL 6.7 build issues as well.

In D58184#1413905, @cryptoad wrote:

I seemed to have fixed it locally:

add_library("RTScudoStandalone.test.${arch}" STATIC
  $<TARGET_OBJECTS:RTScudoStandalone.${arch}>)

The STATIC was missing. Let me know if that works for you.

append_list_if(COMPILER_RT_HAS_LIBRT -lrt LINK_FLAGS) did not work, nor did list(APPEND LINK_FLAGS -lrt) for RHEL 6.7.

I'm about to test it on RHEL 6.7 to see if the two are orthogonal or related (I suspect the former).

As expected, it did not fix RHEL 6.7. I am now trying your append_list_if(COMPILER_RT_HAS_LIBRT -lrt LINK_FLAGS) suggestion.

@cryptoad the following (terribly hacky) patch successfully builds for me on Ubuntu 18.04 LTS:

diff --git a/compiler-rt/cmake/base-config-ix.cmake b/compiler-rt/cmake/base-config-ix.cmake
index ee9426b715d..8cc1e922f50 100644
--- a/compiler-rt/cmake/base-config-ix.cmake
+++ b/compiler-rt/cmake/base-config-ix.cmake
@@ -161,7 +161,7 @@ macro(test_targets)
           endif()
         else()
           test_target_arch(x86_64 "" "-m64")
-          test_target_arch(i386 __i386__ "-m32")
+          #test_target_arch(i386 __i386__ "-m32")
         endif()
       else()
         if (CMAKE_SIZEOF_VOID_P EQUAL 4)

Hello @cryptoad , I am seeing compiler build failures starting with the commit for this patch (sha 41aba567d991c2bd551c ).

I finished performing functional testing using MergeBlockIntoPredecessor in JumpThreading.cpp. With this change I was able to pass make-check and test-suite. I saw no crashes and believe this to be the correct updates for DTU.

Thanks! Sounds like it will be possible to avoid DomTree recalculations then. :) I’m glad to help if there is anything involving DTU to the best of my knowledge.

I appreciate it. So far the issues I've had to deal with are all related to test cases, LVI, or the ABI change that MErgeBlockIntoPredecessor can return false and not merge blocks. I'm running test-suite today to double-check my changes. If that goes well I'll probably give you a "LGTM" on this change.

In D58444#1404380, @NutshellySima wrote:

In D58444#1404242, @brzycki wrote:

Thank you for looking into this. I was working on applying MergeBLockIntoPredecessor replacement in JumpThreading.cpp (via D48202) and ran into check failures that I haven't had time to properly debug. I suspect at least one of these is related to the change you made above.

Thanks for your comment! I am worried about the difficulty in discovering and debugging bugs involving incremental DT updating. I think it is indeed easy to violet the preconditions of applyUpdates() and I believe if it is a concern, a strict validation should be added to check if preconditions are fulfilled in DEBUG mode later.

LGTM.

In D58327#1403532, @MaskRay wrote:

@brzycki Thanks for the tip! Do you know how to create testing .bc from LLVM test-suite for benchmarks?

Hello @MaskRay, test-suite has a subdirectory named LLVMSource that contains IR variants of tests. However, I see no reference to that sub-directory in the CMake build files and the last update to that directory was in 2012. I have never tried it and have no idea if lit can run tests in there. The LLVMSource/Makefile does pull in all files in that directory so if it worked you should be able to just add a *.ll file there for it to be picked up.

+1 Thank you. LGTM.

Thank you for looking into this. I was working on applying MergeBLockIntoPredecessor replacement in JumpThreading.cpp (via D48202) and ran into check failures that I haven't had time to properly debug. I suspect at least one of these is related to the change you made above.

@NutshellySima I like the idea of the new naming scheme that makes usage more explicit.

In D58327#1402240, @kuhar wrote:

In addition, make sure that your performance governor is set to performance and that you pin the process to a single cpu core with its sibling disabled. (like in https://llvm.org/docs/Benchmarking.html#linux). Benchmarking on x86 is hard.

+1

Here's my proposed logic as actual CMake code. @mikhail.ramalho if you can get your code to emit the version string I made a TODO placeholder for it in the 3rd block below.

In D54978#1397354, @ddcc wrote:

In D54978#1397316, @mikhail.ramalho wrote:

I just sent the first prototype of the solution. All the magic happens inside the CHECK_CXX_SOURCE_RUNS call.

I think hardcoding the version into the binary is too brittle. Instead, your program can just print out the current z3 version (much like the current execution of the z3 binary), and the remaining logic can remain in CMake, with the fallbacks as suggested above by @brzycki.

Hello @masakiarai, the new patch looks better, thank you. My updated comments are inline.

My apologies for missing this review, for some reason I wasn't notified of being added. I will look into the potential replacement of MergeBlockIntoPredecessor for JumpThreading.

In D54978#1396927, @ddcc wrote:

The old version.h header isn't externally exposed, only the newer z3_version.h header starting with version 4.8.1. I built a copy of 4.7.1 from source, and I don't see it, so unfortunately I think the second check for version.h is superfluous. Instead, I think it'd be ok to still query the executable as the primary, then fallback to this header as the secondary?

Thanks @ddcc for checking on version.h, I meant to do so but got side-tracked with other items today.

In D54978#1395562, @mikhail.ramalho wrote:

Hi @brzycki, but isn't it exactly what we want? I mean, if we try to cross-compile and it fails for any reason (non-native library, wrong version), then Z3_FOUND shouldn't be set.

I'm not sure, cross-compilation is tricky to get right.

recalculate() can be deprecated. But there are still edge cases, for example, when we only insert a new block to be the entry block or move a block up to be the entry block, it is still needed to call flush() to detect this change. (But I don't see usage like the above scenario I mentioned currently.)

If there are still edge cases then we need to keep it as a last-ditch method of resetting the DTU state without creating a new class instance. It might be a good idea to add documentation stating the interface isn't meant to be used except as a last-ditch fix that cannot be addressed by any other means.

This is much cleaner, I like this very much. Originally adding the method recalculate() was a compromise. With this change do we even need the public recalculate()method? Does it ever make sense to make a new entry BB that has zero successors?

In D57953#1395870, @masakiarai wrote:

I uploaded my patch with some fixes.

Thank you for the comments and re-upload @masakiarai. I will review the patch again either today or tomorrow.

perhaps something like this:

if(Z3_INCLUDE_DIR AND EXISTS "${Z3_INCLUDE_DIR }/z3_version.h")
  file(STRINGS "${Z3_INCLUDE_DIR }/z3_version.h" z3_version_str REGEX "^#define[\t ]+Z3_FULL_VERSION[\t ]+\".*\"")

  string(REGEX REPLACE "^.*Z3_FULL_VERSION[\t ]+\"Z3 ([0-9\.]+)\".*$" "\\1" Z3_VERSION_STRING "${z3_version_str}")
  unset(z3_version_str)
endif()

In D54978#1395476, @mikhail.ramalho wrote:

I'm wondering if we can remove the binary requirement all together: is it possible to run a small program that would return EXIT_SUCCESS if the library is the correct version?

Hi @mikhail.ramalho, I don't think this is feasible unfortunately. If we're using a cross-compiler the emitted binary won't be native to the platform. In other words, you cannot test for Z3 as a run-time property.

In D54978#1395288, @ddcc wrote:

In D54978#1395268, @brzycki wrote:

I think I found why others are struggling to recreate this issue. I did not have the package z3/bionic installed. This provides the /usr/bin/z3 executable which llvm/cmake/modules/FindZ3.cmake relies upon to determine the correct Z3_VERSION_STRING.

Yeah, that's what I meant by best-effort only. Due to upstream Z3's design, without the binary, there is no easy way to retrieve the current installed version, so when I originally wrote the Z3 integration, it seemed fine to let the version check pass. Given the issues that have come up, it might make more sense to at least emit a warning, or explicitly fail if the version can't be determined, and prompt the user to install the binary.

I think I found why others are struggling to recreate this issue. I did not have the package z3/bionic installed. This provides the /usr/bin/z3 executable which llvm/cmake/modules/FindZ3.cmake relies upon to determine the correct Z3_VERSION_STRING.

The following patch:

diff --git a/llvm/cmake/modules/CrossCompile.cmake b/llvm/cmake/modules/CrossCompile.cmake
index bc3b210f018..0c30b88f80f 100644
--- a/llvm/cmake/modules/CrossCompile.cmake
+++ b/llvm/cmake/modules/CrossCompile.cmake
@@ -53,6 +53,7 @@ function(llvm_create_cross_target_internal target_name toolchain buildtype)
         -DLLVM_DEFAULT_TARGET_TRIPLE="${TARGET_TRIPLE}"
         -DLLVM_TARGET_ARCH="${LLVM_TARGET_ARCH}"
         -DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN="${LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN}"
+        -DLLVM_ENABLE_Z3_SOLVER="${LLVM_ENABLE_Z3_SOLVER}"
         ${build_type_flags} ${linker_flag} ${external_clang_dir}
     WORKING_DIRECTORY ${LLVM_${target_name}_BUILD}
     DEPENDS CREATE_LLVM_${target_name}