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.

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

formula_cellar_checks: check for cpuid instruction when needed #11644

Merged
merged 3 commits into from
Jul 6, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions Library/Homebrew/extend/os/mac/keg.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,10 @@ class Keg
GENERIC_MUST_BE_WRITABLE_DIRECTORIES +
[HOMEBREW_PREFIX/"Frameworks"]
).sort.uniq.freeze

undef binary_executable_or_library_files

def binary_executable_or_library_files
mach_o_files
end
end
52 changes: 52 additions & 0 deletions Library/Homebrew/formula_cellar_checks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,37 @@ def check_service_command(formula)
"Service command does not exist" unless File.exist?(formula.service.command.first)
end

def check_cpuid_instruction(formula)
return unless formula.prefix.directory?
# TODO: add methods to `utils/ast` to allow checking for method use
return unless formula.path.read.include? "ENV.runtime_cpu_detection"
# Checking for `cpuid` only makes sense on Intel:
# https://en.wikipedia.org/wiki/CPUID
return unless Hardware::CPU.intel?

# macOS `objdump` is a bit slow, so we prioritise llvm's `llvm-objdump` (~5.7x faster)
# or binutils' `objdump` (~1.8x faster) if they are installed.
Copy link
Member

Choose a reason for hiding this comment

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

Do these times include the installation time of llvm or binutils? Even if so: I'm not sure it's worth installing binutils just for this rare edge-case (but could be convinced otherwise) given the potential to mess with builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

They do not include installation time -- this is the time difference reported from running hyperfine on [llvm-]objdump -d libopenblas.dylib.

I'm fine with returning an audit failure if there is no objdump on the system -- our macOS runners have it, and I believe our Linux runners do as well.

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 it's worth benchmarking the installation time as part of the "faster" times. Otherwise it's not a great comparison.

I'm fine with returning an audit failure if there is no objdump on the system -- our macOS runners have it, and I believe our Linux runners do as well.

🆒.

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 it's worth benchmarking the installation time as part of the "faster" times.

I think benchmarking this would be important if we were installing something in this audit, but we no longer are. I agree that it would be a useful benchmark to have, though -- especially if we end up considering installing something for this audit down the road.

Copy link
Member

Choose a reason for hiding this comment

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

@carlocab Fine with me 👍🏻

objdump = Formula["llvm"].opt_bin/"llvm-objdump" if Formula["llvm"].any_version_installed?
objdump ||= Formula["binutils"].opt_bin/"objdump" if Formula["binutils"].any_version_installed?
objdump ||= which("objdump")
objdump ||= which("objdump", ENV["HOMEBREW_PATH"])

unless objdump
return <<~EOS
No `objdump` found, so cannot check for a `cpuid` instruction. Install `objdump` with
brew install binutils
EOS
end

keg = Keg.new(formula.prefix)
has_cpuid_instruction = keg.binary_executable_or_library_files.any? do |file|
cpuid_instruction?(file, objdump)
end
return if has_cpuid_instruction
carlocab marked this conversation as resolved.
Show resolved Hide resolved

"No `cpuid` instruction detected. #{formula} should not use `ENV.runtime_cpu_detection`."
end

def audit_installed
@new_formula ||= false

Expand All @@ -303,6 +334,7 @@ def audit_installed
problem_if_output(check_shim_references(formula.prefix))
problem_if_output(check_plist(formula.prefix, formula.plist))
problem_if_output(check_python_symlinks(formula.name, formula.keg_only?))
problem_if_output(check_cpuid_instruction(formula))
end
alias generic_audit_installed audit_installed

Expand All @@ -311,6 +343,26 @@ def audit_installed
def relative_glob(dir, pattern)
File.directory?(dir) ? Dir.chdir(dir) { Dir[pattern] } : []
end

def cpuid_instruction?(file, objdump = "objdump")
@instruction_column_index ||= {}
@instruction_column_index[objdump] ||= if Utils.popen_read(objdump, "--version").include? "LLVM"
1 # `llvm-objdump` or macOS `objdump`
else
2 # GNU binutils `objdump`
end

has_cpuid_instruction = false
Utils.popen_read(objdump, "--disassemble", file) do |io|
until io.eof?
instruction = io.readline.split("\t")[@instruction_column_index[objdump]]&.chomp
has_cpuid_instruction = instruction.match?(/^cpuid(\s+|$)/) if instruction
carlocab marked this conversation as resolved.
Show resolved Hide resolved
break if has_cpuid_instruction
end
end

has_cpuid_instruction
end
end

require "extend/os/formula_cellar_checks"
4 changes: 4 additions & 0 deletions Library/Homebrew/keg.rb
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,10 @@ def delete_pyc_files!
find { |pn| FileUtils.rm_rf pn if pn.basename.to_s == "__pycache__" }
end

def binary_executable_or_library_files
elf_files
end

private

def resolve_any_conflicts(dst, dry_run: false, verbose: false, overwrite: false)
Expand Down