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

Fix avoid linking libpcre when unused #14891

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

kojix2
Copy link
Contributor

@kojix2 kojix2 commented Aug 12, 2024

As reported by galapagdos on the Crystal forum, crystal always links libpcre/libpcre2 even if you don not use regular expressions, and calls the function pcre2_config_8.

crystal build hello_world.cr
ldd hello_world # libpcre2 is linked even if regular expressions are not used

This occurs because begin is used in class_getter in the Regex::PCRE module (src/regex/pcre.cr).

I searched the Crystal code with grep command and could not find any other uses of begin in class_getters.
supplement

grep class_getter | grep begin
src/regex/pcre2.cr:  class_getter version_number : {Int32, Int32} = begin
src/regex/pcre.cr:  class_getter version_number : {Int32, Int32} = begin

According to crystal tool expand, the macro is expanded as below.

Before:

module Regex::PCRE2
  @@version_number : ::Tuple(Int32, Int32) = begin
    version = self.version
    dot = if __temp_98 = version.index('.')
            __temp_98
          else
            raise(RuntimeError.new("Invalid libpcre2 version"))
          end
    space = if __temp_105 = version.index(' ', dot)
              __temp_105
            else
              raise(RuntimeError.new("Invalid libpcre2 version"))
            end
    {(version.byte_slice(0, dot)).to_i, (version.byte_slice(dot + 1, (space - dot) - 1)).to_i(false)}
  end

  def self.version_number : ::Tuple(Int32, Int32)
    @@version_number
  end
end

After:

module Regex::PCRE2
  @@version_number : ::Tuple(Int32, Int32) | ::Nil

  def self.version_number : ::Tuple(Int32, Int32)
    if (value = @@version_number).nil?
      @@version_number = (begin
        version = self.version
        dot = (version.index('.')) || (raise(RuntimeError.new("Invalid libpcre2 version")))
        space = (version.index(' ', dot)) || (raise(RuntimeError.new("Invalid libpcre2 version")))
        {(version.byte_slice(0, dot)).to_i, (version.byte_slice(dot + 1, (space - dot) - 1)).to_i(strict: false)}
      end)
    else
      value
    end
  end
end

Could you please review if it's ok not to use begin here?
Thank you.

Resolves #5379

@crysbot
Copy link

crysbot commented Aug 12, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/issue-with-running-crystal-executable-on-another-macos-machine/7080/10

@straight-shoota straight-shoota added this to the 1.14.0 milestone Aug 12, 2024
@straight-shoota straight-shoota changed the title Fixed libpcre being linked even when not needed Fix linking libpcre when unused Aug 13, 2024
@ysbaddaden
Copy link
Contributor

Good catch!

One difference is that instead of being allocated by the runtime when the application is starting (before concurrency & parallelism have started) it will now be lazily generated on demand. It's thus possible that it will be called from 2 threads at the same time. In this specific case, the worst that can happen is that they both generate & store the version number.

Another difference is that instead of being a Tuple(Int32, Int32) it's now a nilable type, and it must be checked every time.

Both aren't an issue for this specific case.

@straight-shoota straight-shoota changed the title Fix linking libpcre when unused Fix avoid linking libpcre when unused Aug 13, 2024
@straight-shoota straight-shoota merged commit 9ecd838 into crystal-lang:master Aug 14, 2024
62 checks passed
@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:codegen labels Aug 14, 2024
@kojix2 kojix2 deleted the pcre branch August 15, 2024 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add some code only if the given lib is used
5 participants