Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Products] Don't dlopen libraries built for an incompatible ISA #233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Products] Don't dlopen libraries built for an incompatible ISA #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "BinaryBuilderBase"
uuid = "7f725544-6523-48cd-82d1-3fa08ff4056e"
authors = ["Elliot Saba <staticfloat@gmail.com>"]
version = "1.8.0"
version = "1.8.1"

[deps]
CodecZlib = "944b1d66-785c-5afd-91f1-9de20f533193"
Expand Down
2 changes: 1 addition & 1 deletion src/Products.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ function locate(lp::LibraryProduct, prefix::Prefix; platform::AbstractPlatform =
end

# If it does, try to `dlopen()` it if the current platform is good
if (!lp.dont_dlopen && !skip_dlopen) && platforms_match(platform, HostPlatform())
if (!lp.dont_dlopen && !skip_dlopen) && platforms_match(platform, augment_microarchitecture!(HostPlatform()))
if isolate
# Isolated dlopen is a lot slower, but safer
if success(`$(Base.julia_cmd()) --startup-file=no -e "import Libdl; Libdl.dlopen(\"$dl_path\")"`)
Expand Down
48 changes: 48 additions & 0 deletions src/utils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# The functions in this file may not be used anywhere in this package but may
# needed by different modules of BinaryBuilder.jl

using Base.BinaryPlatforms: arch_march_isa_mapping, set_compare_strategy!
using Base.BinaryPlatforms.CPUID

with_logfile(f::Function, prefix::Prefix, name::String; subdir="") = with_logfile(f, joinpath(logdir(prefix; subdir), name))
function with_logfile(f::Function, logfile::String)
mkpath(dirname(logfile))
Expand Down Expand Up @@ -60,3 +63,48 @@ end
# We want the AnyPlatform to look like `default_host_platform`,
get_concrete_platform(::AnyPlatform, shards::Vector{CompilerShard}) =
get_concrete_platform(default_host_platform, shards)

function march_comparison_strategy(a::String, b::String, a_requested::Bool, b_requested::Bool)
# If both b and a requested, then we fall back to equality:
if a_requested && b_requested
return a == b
end

function get_arch_isa(isa_name::String)
for (arch, isas) in arch_march_isa_mapping
for (name, isa) in isas
name == isa_name && return arch, isa
end
end
return nothing, nothing
end

a_arch, a_isa = get_arch_isa(a)
b_arch, b_isa = get_arch_isa(b)
if any(isnothing, (a_arch, b_arch)) || a_arch != b_arch
# Architectures are definitely not compatible, exit early
return false
end

if a_requested
# ISA `b` is compatible with ISA `a` only if it's a subset of `a`
return b_isa ≤ a_isa
else
# ISA `a` is compatible with ISA `b` only if it's a subset of `b`
return a_isa ≤ b_isa
end
end

function augment_microarchitecture!(platform::Platform)
if haskey(platform, "march")
set_compare_strategy!(platform, "march", march_comparison_strategy)
return platform
end

host_arch = arch(HostPlatform())
host_isas = arch_march_isa_mapping[host_arch]
idx = findlast(((name, isa),) -> isa <= CPUID.cpu_isa(), host_isas)
platform["march"] = first(host_isas[idx])
set_compare_strategy!(platform, "march", march_comparison_strategy)
return platform
end
41 changes: 41 additions & 0 deletions test/compat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,44 @@ using BinaryBuilderBase: download_verify, list_tarball_files
end
end
end

using BinaryBuilderBase: march_comparison_strategy
using Base.BinaryPlatforms: Platform, platforms_match, set_compare_strategy!
@testset "Microarchitecture augmentation" begin
linux_x86_64 = Platform("x86_64", "linux")
linux_avx = Platform("x86_64", "linux"; march="avx")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I would call this march. llc has mcpu which has names like opteron or x86-64-v3 and mattr which allows you to set feature sets.

avx is the name of a feature set. I think following hwcaps/linux feature level and only support:

x86-64-v1
x86-64-v2
x86-64-v3
x86-64-v4

Might be the best.
x-ref: JuliaLang/julia#42073

The logic for how to map x86-64-v3 to different compilers is defined here https://github.com/archspec/archspec-json/blob/020e418c9fe991a8155e53245fde4e4bdef08382/cpu/microarchitectures.json#L135-L191

Copy link
Member Author

@giordano giordano Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He, those names and sets are in Base: https://github.com/JuliaLang/julia/blob/c50361144ce783cecf65294e09d6e7cfdc73eb3f/base/binaryplatforms.jl#L598 changing them is quite a nightmare

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...Unless you're going to suggest to move the dictionary to https://github.com/JuliaPackaging/Yggdrasil/blob/master/platforms/microarchitectures.jl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/ How are we going to deal with adding new names? E.g. power9? I would prefer for these not to be dependent on Julia releases. So yes moving them there or MicroArchitectures.jl (better than JLLWrappers?) would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still depend on julia for features detection (and for powerpc that's completely missing).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, what name do you suggest instead of march? I think we initially thought about microarchitecture but that was a bit mouthful.

I'm also mostly ok with revising the levels we have (especially if we move them to https://github.com/JuliaPackaging/Yggdrasil/blob/master/platforms/microarchitectures.jl so we don't need to worry about compatibility), but we'll also need to revise all the compiler flags and versions in

BinaryBuilderBase.jl/src/Platforms.jl

Lines 90 to 147 in 55c795d

# NOTE: This needs to be kept in sync with `ISAs_by_family` in `Base.BinaryPlatforms.CPUID`
# This will allow us to detect these names at runtime and select artifacts accordingly.
const ARCHITECTURE_FLAGS = Dict(
# Many compiler flags are the same across clang and gcc, store those in "common"
"common" => Dict(
"i686" => OrderedDict(
"pentium4" => ["-march=pentium4", "-mtune=generic"],
"prescott" => ["-march=prescott", "-mtune=prescott"],
),
"x86_64" => OrderedDict(
# Better be always explicit about `-march` & `-mtune`:
# https://lemire.me/blog/2018/07/25/it-is-more-complicated-than-i-thought-mtune-march-in-gcc/
"x86_64" => ["-march=x86-64", "-mtune=generic"],
"avx" => ["-march=sandybridge", "-mtune=sandybridge"],
"avx2" => ["-march=haswell", "-mtune=haswell"],
"avx512" => ["-march=skylake-avx512", "-mtune=skylake-avx512"],
),
"armv6l" => OrderedDict(
# This is the only known armv6l chip that runs Julia, so it's the only one we care about.
"arm1176jzfs" => ["-mcpu=arm1176jzf-s", "-mfpu=vfp", "-mfloat-abi=hard"],
),
"armv7l" => OrderedDict(
# Base armv7l architecture, with the most basic of FPU's
"armv7l" => ["-march=armv7-a", "-mtune=generic-armv7-a", "-mfpu=vfpv3", "-mfloat-abi=hard"],
# armv7l plus NEON and vfpv4, (Raspberry Pi 2B+, RK3328, most boards Elliot has access to)
"neonvfpv4" => ["-march=armv7-a", "-mtune=cortex-a53", "-mfpu=neon-vfpv4", "-mfloat-abi=hard"],
),
"aarch64" => OrderedDict(
# For GCC, see: <https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html>. For
# LLVM, for the list of features see
# <https://github.com/llvm/llvm-project/blob/1bcc28b884ff4fbe2ecc011b8ea2b84e7987167b/llvm/include/llvm/Support/AArch64TargetParser.def>
# and
# <https://github.com/llvm/llvm-project/blob/85e9b2687a13d1908aa86d1b89c5ce398a06cd39/llvm/lib/Target/AArch64/AArch64.td>.
# Run `clang --print-supported-cpus` for the list of values of `-mtune`.
"armv8_0" => ["-march=armv8-a", "-mtune=cortex-a57"],
"armv8_1" => ["-march=armv8.1-a", "-mtune=thunderx2t99"],
"armv8_2_crypto" => ["-march=armv8.2-a+aes+sha2", "-mtune=cortex-a76"],
"a64fx" => ["-march=armv8.2-a+aes+sha2+fp16+sve", "-mtune=a64fx"],
),
"powerpc64le" => OrderedDict(
"power8" => ["-mcpu=power8", "-mtune=power8"],
# Note that power9 requires GCC 6+, and we need CPUID for this
#"power9" => ["-mcpu=power9", "-mtune=power9"],
# Eventually, we'll support power10, once we have compilers that support it.
#"power10" => ["-mcpu=power10", "-mtune=power10"],
)
),
"gcc" => Dict(
"aarch64" => OrderedDict(
"apple_m1" => ["-march=armv8.5-a+aes+sha2+sha3+fp16fml+fp16+rcpc+dotprod", "-mtune=cortex-a76"],
),
),
"clang" => Dict(
"aarch64" => OrderedDict(
"apple_m1" => ["-march=armv8.5-a+aes+sha2+sha3+fp16fml+fp16+rcpc+dotprod", "-mtune=apple-a12"],
),
),
)
which I wasn't exactly looking forward to 😁 Side note, I'm moderately sure we settled on the sets we now have in Julia Base before the glibc x86-64 levels were widely advertised 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think march is fine, I am more opposed to calling avx a micro-architecture since it is not, but a micro-architecture feature.

Yeah which is why I pointed out archspec, we could parse that JSON instead of maintaining the same thing in BB.

linux_avx2 = Platform("x86_64", "linux"; march="avx2")
linux_avx512 = Platform("x86_64", "linux"; march="avx512")
# Platform with non-existing microarchitecture
linux_bad = Platform("x86_64", "linux"; march="bad")

# Without any custom comparison strategy, the base platform without march matches
# everything, but the others are all incompatible
@test platforms_match(linux_x86_64, linux_avx)
@test platforms_match(linux_x86_64, linux_avx2)
@test platforms_match(linux_x86_64, linux_avx512)
@test platforms_match(linux_x86_64, linux_bad)
@test !platforms_match(linux_avx, linux_avx2)
@test !platforms_match(linux_avx, linux_avx512)
@test !platforms_match(linux_avx, linux_bad)
@test !platforms_match(linux_avx2, linux_bad)
@test !platforms_match(linux_avx2, linux_avx512)
@test !platforms_match(linux_avx512, linux_bad)

# Teach AVX2 platform how to compare the others
set_compare_strategy!(linux_avx2, "march", march_comparison_strategy)
for compatible_p in (linux_x86_64, linux_avx)
@test platforms_match(compatible_p, linux_avx2)
@test platforms_match(linux_avx2, compatible_p)
end
for incompatible_p in (linux_avx512, linux_bad)
@test !platforms_match(incompatible_p, linux_avx2)
@test !platforms_match(linux_avx2, incompatible_p)
end

# Teach also AVX platform how to compare
set_compare_strategy!(linux_avx, "march", march_comparison_strategy)
# Now when we compare AVX and AVX2, they must be equal
@test !platforms_match(linux_avx, linux_avx2)
@test !platforms_match(linux_avx2, linux_avx)
end