[Auditor] Make the auditor multithreaded (#1364)

* [Auditor] Make running the auditor multithreaded

* [Auditor] Put lock around use of sandbox mounted on shared directories

* [Auditor] More parallelism

* Require more thread-safe BinaryBuilderBase

* [Auditor] Remove legacy code

* [Auditor] Use `patchelf` from `Patchelf_jll` instead the one inside the sandbox

* Adapt test to error being thrown inside a task

* [AutoBuild] Properly quote `timer` as a string

* [Auditor] Put lock around all logging macros

* [Auditor] Use `ldid_jll` to avoid calling the executable inside the sandbox

* Revert "[Auditor] Use `ldid_jll` to avoid calling the executable inside the sandbox"

This reverts commit 583af47.

* [Auditor] Remove threading from libtool pass

There seems to be some issues with threading, tests don't pass, but this should
also not be a time-critical pass, so making it serial shouldn't be _too_ bad.

* [Auditor] Use `@spawn` instead of `@threads` for better load balancing

* Revert "[Auditor] Use `@spawn` instead of `@threads` for better load balancing"

This reverts commit bfbe34e.

* [Auditor] Introduce helper function for running external commands

* Bump version number

Author: giordano

Project.toml

@@ -1,7 +1,7 @@
 name = "BinaryBuilder"
 uuid = "12aac903-9f7c-5d81-afc2-d9565ea332ae"
 authors = ["Elliot Saba <[email protected]>"]
-version = "0.6.3"
+version = "0.6.4"
 
 [deps]
 ArgParse = "c7e460c6-2fb9-53a9-8c5b-16f535851c63"
@@ -18,6 +18,7 @@ Logging = "56ddb016-857b-54e1-b83d-db4d58db5568"
 LoggingExtras = "e6f89c97-d47a-5376-807f-9c37f3926c36"
 ObjectFile = "d8793406-e978-5875-9003-1fc021f44a92"
 OutputCollectors = "6c11c7d4-943b-4e2b-80de-f2cfc2930a8c"
+Patchelf_jll = "f2cf89d6-2bfd-5c44-bd2c-068eea195c0c"
 Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
 PkgLicenses = "fc669557-7ec9-5e45-bca9-462afbc28879"
 REPL = "3fa0cd96-eef1-5676-8a61-b3b8758bbffb"
@@ -33,7 +34,7 @@ ghr_jll = "07c12ed4-43bc-5495-8a2a-d5838ef8d533"
 
 [compat]
 ArgParse = "1.1"
-BinaryBuilderBase = "1.34"
+BinaryBuilderBase = "1.35.2"
 Downloads = "1"
 GitHub = "5.1"
 HTTP = "0.8, 0.9, 1"
@@ -43,6 +44,7 @@ JSON = "0.21"
 LoggingExtras = "0.4, 1"
 ObjectFile = "0.4.3"
 OutputCollectors = "0.1"
+Patchelf_jll = "0.14.3"
 PkgLicenses = "0.2"
 Registrator = "1.1"
 RegistryTools = "2.1"

azure-pipelines.yml

@@ -30,6 +30,8 @@ variables:
   BINARYBUILDER_AUTOMATIC_APPLE: true
   BINARYBUILDER_USE_CCACHE: true
   CI: true
+  # Auditor is now multi-threaded, run tests with multiple threads
+  JULIA_NUM_THREADS: 3
 
 jobs:
 - job: Info

src/Auditor.jl

@@ -1033,7 +1033,7 @@ function autobuild(dir::AbstractString,
         if isempty(readdir(build_path))
             rm(build_path; recursive=true)
         end
-        verbose && @info timer
+        verbose && @info "$(timer)"
     end
 
     # Return our product hashes

src/AutoBuild.jl

@@ -5,6 +5,7 @@ function check_codesigned(path::AbstractString, platform::AbstractPlatform)
     end
 
     ur = preferred_runner()(dirname(path); cwd="/workspace/", platform=platform)
+    # TODO: can we run directly `ldid` with the JLL without entering the sandbox?
     return run(ur, `/usr/local/bin/ldid -d $(basename(path))`)
 end
 
@@ -18,6 +19,7 @@ function ensure_codesigned(path::AbstractString, prefix::Prefix, platform::Abstr
     rel_path = relpath(path, prefix.path)
     ur = preferred_runner()(prefix.path; cwd="/workspace/", platform=platform)
     with_logfile(prefix, "ldid_$(basename(rel_path)).log"; subdir) do io
-        run(ur, `/usr/local/bin/ldid -S -d $(rel_path)`, io; verbose=verbose)
+        # TODO: can we run directly `ldid` with the JLL without entering the sandbox?
+        @lock AUDITOR_SANDBOX_LOCK run(ur, `/usr/local/bin/ldid -S -d $(rel_path)`, io; verbose=verbose)
     end
 end

src/auditor/codesigning.jl

@@ -1,7 +1,7 @@
 import Base.BinaryPlatforms: detect_libstdcxx_version, detect_cxxstring_abi
 using ObjectFile
 
-csl_warning(lib) = @warn(
+csl_warning(lib) = @lock AUDITOR_LOGGING_LOCK @warn(
     """
     To ensure that the correct version of $(lib) is found at runtime, add the following entry to the list of dependencies of this builder
 
@@ -37,12 +37,12 @@ function check_libgfortran_version(oh::ObjectHandle, platform::AbstractPlatform;
         if isa(e, InterruptException)
             rethrow(e)
         end
-        @warn "$(path(oh)) could not be scanned for libgfortran dependency!" exception=(e, catch_backtrace())
+        @lock AUDITOR_LOGGING_LOCK @warn "$(path(oh)) could not be scanned for libgfortran dependency!" exception=(e, catch_backtrace())
         return true
     end
 
     if verbose && version !== nothing
-        @info("$(path(oh)) locks us to libgfortran v$(version)")
+        @lock AUDITOR_LOGGING_LOCK @info("$(path(oh)) locks us to libgfortran v$(version)")
     end
 
     if !has_csl && version !== nothing
@@ -57,7 +57,7 @@ function check_libgfortran_version(oh::ObjectHandle, platform::AbstractPlatform;
         definition in your `build_tarballs.jl` file, add the line:
         """, '\n' => ' '))
         msg *= "\n\n    platforms = expand_gfortran_versions(platforms)"
-        @warn(msg)
+        @lock AUDITOR_LOGGING_LOCK @warn(msg)
         return false
     end
 
@@ -67,7 +67,7 @@ function check_libgfortran_version(oh::ObjectHandle, platform::AbstractPlatform;
         for libgfortran$(libgfortran_version(platform).major). This usually indicates that
         the build system is somehow ignoring our choice of compiler!
         """, '\n' => ' '))
-        @warn(msg)
+        @lock AUDITOR_LOGGING_LOCK @warn(msg)
         return false
     end
     return true
@@ -87,7 +87,7 @@ function check_csl_libs(oh::ObjectHandle, platform::AbstractPlatform; verbose::B
         if isa(e, InterruptException)
             rethrow(e)
         end
-        @warn "$(path(oh)) could not be scanned for $(lib) dependency!" exception=(e, catch_backtrace())
+        @lock AUDITOR_LOGGING_LOCK @warn "$(path(oh)) could not be scanned for $(lib) dependency!" exception=(e, catch_backtrace())
         return true
     end
 
@@ -140,12 +140,12 @@ function check_libstdcxx_version(oh::ObjectHandle, platform::AbstractPlatform; v
         if isa(e, InterruptException)
             rethrow(e)
         end
-        @warn "$(path(oh)) could not be scanned for libstdcxx dependency!" exception=(e, catch_backtrace())
+        @lock AUDITOR_LOGGING_LOCK @warn "$(path(oh)) could not be scanned for libstdcxx dependency!" exception=(e, catch_backtrace())
         return true
     end
 
     if verbose && libstdcxx_version != nothing
-        @info("$(path(oh)) locks us to libstdc++ v$(libstdcxx_version)+")
+        @lock AUDITOR_LOGGING_LOCK @info("$(path(oh)) locks us to libstdc++ v$(libstdcxx_version)+")
     end
 
     # This actually isn't critical, so we don't complain.  Yet.
@@ -172,6 +172,8 @@ function cppfilt(symbol_names::Vector, platform::AbstractPlatform; strip_undersc
 
     output = IOBuffer()
     mktempdir() do dir
+        # No need to acquire a sandbox lock here because we use a (hopefully)
+        # different temporary directory for each run.
         ur = preferred_runner()(dir; cwd="/workspace/", platform=platform)
         cmd = Cmd(`/opt/bin/$(triplet(ur.platform))/c++filt`; ignorestatus=true)
         if strip_underscore
@@ -216,7 +218,7 @@ function detect_cxxstring_abi(oh::ObjectHandle, platform::AbstractPlatform)
         if isa(e, InterruptException)
             rethrow(e)
         end
-        @warn "$(path(oh)) could not be scanned for cxx11 ABI!" exception=(e, catch_backtrace())
+        @lock AUDITOR_LOGGING_LOCK @warn "$(path(oh)) could not be scanned for cxx11 ABI!" exception=(e, catch_backtrace())
     end
     return nothing
 end
@@ -233,7 +235,7 @@ function check_cxxstring_abi(oh::ObjectHandle, platform::AbstractPlatform; io::I
     end
 
     if verbose && cxx_abi != nothing
-        @info("$(path(oh)) locks us to $(cxx_abi)")
+        @lock AUDITOR_LOGGING_LOCK @info("$(path(oh)) locks us to $(cxx_abi)")
     end
 
     if cxxstring_abi(platform) == nothing && cxx_abi != nothing
@@ -244,7 +246,7 @@ function check_cxxstring_abi(oh::ObjectHandle, platform::AbstractPlatform; io::I
         definition in your `build_tarballs.jl` file, add the line:
         """, '\n' => ' '))
         msg *= "\n\n    platforms = expand_cxxstring_abis(platforms)"
-        @warn(msg)
+        @lock AUDITOR_LOGGING_LOCK @warn(msg)
         return false
     end
 
@@ -255,7 +257,7 @@ function check_cxxstring_abi(oh::ObjectHandle, platform::AbstractPlatform; io::I
         indicates that the build system is somehow ignoring our choice of compiler, as we manually
         insert the correct compiler flags for this ABI choice!
         """, '\n' => ' '))
-        @warn(msg)
+        @lock AUDITOR_LOGGING_LOCK @warn(msg)
         return false
     end
     return true

src/auditor/compiler_abi.jl

@@ -1,4 +1,5 @@
 using ObjectFile.ELF
+using Patchelf_jll: patchelf
 
 """
     platform_for_object(oh::ObjectHandle)
@@ -346,17 +347,16 @@ function relink_to_rpath(prefix::Prefix, platform::AbstractPlatform, path::Abstr
     libname = basename(old_libpath)
     relink_cmd = ``
 
-    if Sys.isapple(platform)
-        install_name_tool = "/opt/bin/$(triplet(ur.platform))/install_name_tool"
-        relink_cmd = `$install_name_tool -change $(old_libpath) @rpath/$(libname) $(rel_path)`
-    elseif Sys.islinux(platform) || Sys.isbsd(platform)
-        patchelf = "/usr/bin/patchelf"
-        relink_cmd = `$patchelf $(patchelf_flags(platform)) --replace-needed $(old_libpath) $(libname) $(rel_path)`
-    end
-
     # Create a new linkage that looks like @rpath/$lib on OSX
     with_logfile(prefix, "relink_to_rpath_$(basename(rel_path)).log"; subdir) do io
-        run(ur, relink_cmd, io; verbose=verbose)
+        if Sys.isapple(platform)
+            ur = preferred_runner()(prefix.path; cwd="/workspace/", platform=platform)
+            install_name_tool = "/opt/bin/$(triplet(ur.platform))/install_name_tool"
+            relink_cmd = `$install_name_tool -change $(old_libpath) @rpath/$(libname) $(rel_path)`
+            @lock AUDITOR_SANDBOX_LOCK run(ur, relink_cmd, io; verbose=verbose)
+        elseif Sys.islinux(platform) || Sys.isbsd(platform)
+            run_with_io(io, `$(patchelf()) $(patchelf_flags(platform)) --replace-needed $(old_libpath) $(libname) $(path)`)
+        end
     end
 end
 
@@ -380,7 +380,7 @@ function fix_identity_mismatch(prefix::Prefix, platform::AbstractPlatform, path:
     end
 
     if verbose
-        @info("Modifying dylib id from \"$(old_id)\" to \"$(new_id)\"")
+        @lock AUDITOR_LOGGING_LOCK @info("Modifying dylib id from \"$(old_id)\" to \"$(new_id)\"")
     end
 
     ur = preferred_runner()(prefix.path; cwd="/workspace/", platform=platform)
@@ -389,7 +389,7 @@ function fix_identity_mismatch(prefix::Prefix, platform::AbstractPlatform, path:
 
     # Create a new linkage that looks like @rpath/$lib on OSX,
     with_logfile(prefix, "fix_identity_mismatch_$(basename(rel_path)).log"; subdir) do io
-        run(ur, id_cmd, io; verbose=verbose)
+        @lock AUDITOR_SANDBOX_LOCK run(ur, id_cmd, io; verbose=verbose)
     end
 end
 
@@ -417,7 +417,6 @@ function update_linkage(prefix::Prefix, platform::AbstractPlatform, path::Abstra
     normalize_rpath = rp -> rp
     add_rpath = x -> ``
     relink = (x, y) -> ``
-    patchelf = "/usr/bin/patchelf"
     install_name_tool = "/opt/bin/$(triplet(ur.platform))/install_name_tool"
     if Sys.isapple(platform)
         normalize_rpath = rp -> begin
@@ -459,9 +458,9 @@ function update_linkage(prefix::Prefix, platform::AbstractPlatform, path::Abstra
             filter!(rp -> !startswith(rp, "/workspace"), rpaths)
 
             rpath_str = join(rpaths, ':')
-            return `$patchelf $(patchelf_flags(platform)) --set-rpath $(rpath_str) $(rel_path)`
+            return `$(patchelf()) $(patchelf_flags(platform)) --set-rpath $(rpath_str) $(path)`
         end
-        relink = (op, np) -> `$patchelf $(patchelf_flags(platform)) --replace-needed $(op) $(np) $(rel_path)`
+        relink = (op, np) -> `$(patchelf()) $(patchelf_flags(platform)) --replace-needed $(op) $(np) $(path)`
     end
 
     # If the relative directory doesn't already exist within the RPATH of this
@@ -471,7 +470,11 @@ function update_linkage(prefix::Prefix, platform::AbstractPlatform, path::Abstra
         libname = basename(old_libpath)
         cmd = add_rpath(normalize_rpath(relpath(new_libdir, dirname(path))))
         with_logfile(prefix, "update_rpath_$(basename(path))_$(libname).log"; subdir) do io
-            run(ur, cmd, io; verbose=verbose)
+            if Sys.isapple(platform)
+                @lock AUDITOR_SANDBOX_LOCK run(ur, cmd, io; verbose=verbose)
+            elseif Sys.islinux(platform) || Sys.isbsd(platform)
+                run_with_io(io, cmd)
+            end
         end
     end
 
@@ -490,7 +493,11 @@ function update_linkage(prefix::Prefix, platform::AbstractPlatform, path::Abstra
     end
     cmd = relink(old_libpath, new_libpath)
     with_logfile(prefix, "update_linkage_$(basename(path))_$(basename(old_libpath)).log"; subdir) do io
-        run(ur, cmd, io; verbose=verbose)
+        if Sys.isapple(platform)
+            @lock AUDITOR_SANDBOX_LOCK run(ur, cmd, io; verbose=verbose)
+        elseif Sys.islinux(platform) || Sys.isbsd(platform)
+            run_with_io(io, cmd)
+        end
     end
 
     return new_libpath

src/auditor/dynamic_linkage.jl

@@ -13,7 +13,7 @@ function check_os_abi(oh::ObjectHandle, p::AbstractPlatform, rest...; verbose::B
                 $(basename(path(oh))) has an ELF header OS/ABI value that is not set to FreeBSD
                 ($(ELF.ELFOSABI_FREEBSD)), this may be an issue at link time
                 """, '\n' => ' ')
-                @warn(strip(msg))
+                @lock AUDITOR_LOGGING_LOCK @warn(strip(msg))
             end
             return false
         end
@@ -24,7 +24,7 @@ function check_os_abi(oh::ObjectHandle, p::AbstractPlatform, rest...; verbose::B
         # means "no specific float ABI", `0x400` == EF_ARM_ABI_FLOAT_HARD.
         if header(oh).e_flags & 0xF00 ∉ (0x000, 0x400)
             if verbose
-                @error("$(basename(path(oh))) does not match the hard-float ABI")
+                @lock AUDITOR_LOGGING_LOCK @error("$(basename(path(oh))) does not match the hard-float ABI")
             end
             return false
         end

src/auditor/extra_checks.jl

@@ -6,7 +6,7 @@ function check_case_sensitivity(prefix::Prefix)
         for f in list
             lf = lowercase(f)
             if lf in lowered
-                @warn("$(relpath(joinpath(root, f), prefix.path)) causes a case-sensitivity ambiguity!")
+                @lock AUDITOR_LOGGING_LOCK @warn("$(relpath(joinpath(root, f), prefix.path)) causes a case-sensitivity ambiguity!")
                 all_ok = false
             end
             push!(lowered, lf)
@@ -31,12 +31,12 @@ function check_absolute_paths(prefix::Prefix, all_files::Vector; silent::Bool =
             file_contents = String(read(f))
             if occursin(prefix.path, file_contents)
                 if !silent
-                    @warn("$(relpath(f, prefix.path)) contains an absolute path")
+                    @lock AUDITOR_LOGGING_LOCK @warn("$(relpath(f, prefix.path)) contains an absolute path")
                 end
             end
         catch
             if !silent
-                @warn("Skipping abspath scanning of $(f), as we can't open it")
+                @lock AUDITOR_LOGGING_LOCK @warn("Skipping abspath scanning of $(f), as we can't open it")
             end
         end
     end
@@ -51,7 +51,7 @@ function ensure_executability(oh::ObjectHandle; verbose::Bool=false, silent::Boo
     # Check whether the file has executable permission for all
     if old_mode & read_mask != read_mask
         if verbose
-            @info "Making $(path(oh)) executable"
+            @lock AUDITOR_LOGGING_LOCK @info "Making $(path(oh)) executable"
         end
         try
             # Add executable permission for all users that can read the file
@@ -61,7 +61,7 @@ function ensure_executability(oh::ObjectHandle; verbose::Bool=false, silent::Boo
                 rethrow(e)
             end
             if !silent
-                @warn "$(path(oh)) could not be made executable!" exception=(e, catch_backtrace())
+                @lock AUDITOR_LOGGING_LOCK @warn "$(path(oh)) could not be made executable!" exception=(e, catch_backtrace())
             end
         end
     end

src/auditor/filesystems.jl

@@ -41,7 +41,7 @@ function instruction_mnemonics(path::AbstractString, platform::AbstractPlatform)
     else
         objdump_cmd = "\${target}-objdump -d $(basename(path))"
     end
-    run_interactive(ur, Cmd(`/bin/bash -c "$(objdump_cmd)"`; ignorestatus=true); stdout=output, stderr=devnull)
+    @lock AUDITOR_SANDBOX_LOCK run_interactive(ur, Cmd(`/bin/bash -c "$(objdump_cmd)"`; ignorestatus=true); stdout=output, stderr=devnull)
     seekstart(output)
 
     for line in eachline(output)
@@ -162,7 +162,7 @@ function analyze_instruction_set(oh::ObjectHandle, platform::AbstractPlatform; v
             the proper instruction set internally.  Would have chosen
             $(min_march), instead choosing $(generic_march(platform)).
             """, '\n' => ' ')
-            @warn(strip(msg))
+            @lock AUDITOR_LOGGING_LOCK @warn(strip(msg))
         end
         return generic_march(platform)
     end