[PATCH] D112753: [llvm] [Support] Add CURL HTTP Client.

Sat Dec 4 12:00:15 PST 2021

noajshu added a comment.

In D112753#3171414 <https://reviews.llvm.org/D112753#3171414>, @hans wrote:

> In Chromium we just noticed that our clang binary grew a dependency on libcurl-gnutls.so.4 after this change.
>
> That seems odd since clang doesn't have any business talking to the network. I assume this is really for Debuginfod?

Thanks for bringing this up. Yes, this is for the debuginfod client library (D112758 <https://reviews.llvm.org/D112758>). The default value for `LLVM_ENABLE_CURL` is `ON`. So if it finds libcurl on your system, it will use it.
If your clang build were configured with `-DLLVM_ENABLE_CURL=OFF`, the dependency on libcurl would disappear.
Do you think we should change the default value to `OFF`?

> If I understand correctly, the HTTPClient::initialize() call in InitLLVM::InitLLVM() forces the library into *all* LLVM tools by default. The extra dependency seems odd for some tools ("did clang start phoning home with my source?") and doing the initialization as part of each compile step also seems wasteful. Couldn't the initialization happen lazily on the first call to HTTPClient from tools that need it instead?

Curl's global initialization is not thread-safe and should be called <https://curl.se/libcurl/c/curl_global_init.html> at the beginning of the program. This is why we added it to `InitLLVM`. There is some relevant discussion on this here: https://reviews.llvm.org/D112753#inline-1082560
If `LLVM_ENABLE_CURL` is `OFF` then the `initialize` is a no-op. However, if built with curl, tools that do not use it will still call the initialization step. As you point out, this is wasteful. So do you think we should remove it from `InitLLVM` and require user tools to separately init/deinit? Or, should we add a default-false argument `InitializeHTTPClient` to `InitLLVM`'s ctor?

Thanks again for bringing this up.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112753/new/

https://reviews.llvm.org/D112753