https://github.com/weliveindetail created https://github.com/llvm/llvm-project/pull/187687
SymbolLocatorSymStore used a simple local implementation of HTTPResponseHandler so far. That was fine for basic usage, but it would cause issues down the line. This patch hoists the StreamedHTTPResponseHandler class from libDebuginfod to SupportHTTP and integrates it in SymbolLocatorSymStore. PDB file downloads will now be buffered on disk, which is necessary since they can be huge. We use the opportunity an stop logging 404 responses (file not found on server) and print warnings for all other erroneous HTTP responses. It was more complicated before, because the old response handler created the underlying file in any case. The new one does that only one the first content package comes in. From ec60e43a6b568c6437d685b1d22487d32721abc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Gr=C3=A4nitz?= <[email protected]> Date: Fri, 20 Mar 2026 11:48:05 +0100 Subject: [PATCH 1/2] [lldb] Hoist StreamedHTTPResponseHandler to SupportHTTP and use it in SymbolLocatorSymStore --- .../SymStore/SymbolLocatorSymStore.cpp | 48 ++++++++---------- .../HTTP/StreamedHTTPResponseHandler.h | 49 +++++++++++++++++++ llvm/lib/Debuginfod/Debuginfod.cpp | 48 +----------------- llvm/lib/Support/HTTP/CMakeLists.txt | 1 + .../HTTP/StreamedHTTPResponseHandler.cpp | 34 +++++++++++++ 5 files changed, 106 insertions(+), 74 deletions(-) create mode 100644 llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h create mode 100644 llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp diff --git a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp index 884519b54acfd..f1b931d894aa7 100644 --- a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp +++ b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp @@ -18,11 +18,14 @@ #include "lldb/Utility/UUID.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/Support/Caching.h" #include "llvm/Support/Endian.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/HTTP/HTTPClient.h" +#include "llvm/Support/HTTP/StreamedHTTPResponseHandler.h" #include "llvm/Support/Path.h" +#include "llvm/Support/raw_ostream.h" using namespace lldb; using namespace lldb_private; @@ -114,36 +117,11 @@ std::string formatSymStoreKey(const UUID &uuid) { return llvm::toHex(bytes.slice(0, 16), LowerCase) + std::to_string(age); } -// This is a simple version of Debuginfod's StreamedHTTPResponseHandler. We -// should consider reusing that once we introduce caching. -class FileDownloadHandler : public llvm::HTTPResponseHandler { -private: - std::error_code m_ec; - llvm::raw_fd_ostream m_stream; - -public: - FileDownloadHandler(llvm::StringRef file) : m_stream(file.str(), m_ec) {} - virtual ~FileDownloadHandler() = default; - - llvm::Error handleBodyChunk(llvm::StringRef data) override { - // Propagate error from ctor. - if (m_ec) - return llvm::createStringError(m_ec, "Failed to open file for writing"); - m_stream.write(data.data(), data.size()); - if (std::error_code ec = m_stream.error()) - return llvm::createStringError(ec, "Error writing to file"); - - return llvm::Error::success(); - } -}; - -llvm::Error downloadFileHTTP(llvm::StringRef url, FileDownloadHandler dest) { +llvm::Error downloadFileHTTP(llvm::StringRef url, llvm::StringRef destPath) { if (!llvm::HTTPClient::isAvailable()) return llvm::createStringError( std::make_error_code(std::errc::not_supported), "HTTP client is not available"); - llvm::HTTPRequest Request(url); - Request.FollowRedirects = true; llvm::HTTPClient Client; @@ -151,7 +129,23 @@ llvm::Error downloadFileHTTP(llvm::StringRef url, FileDownloadHandler dest) { // connect, send and receive. Client.setTimeout(std::chrono::seconds(60)); - if (llvm::Error Err = Client.perform(Request, dest)) + llvm::StreamedHTTPResponseHandler Handler( + [&]() -> llvm::Expected<std::unique_ptr<llvm::CachedFileStream>> { + std::error_code EC; + auto FDStream = std::make_unique<llvm::raw_fd_ostream>(destPath, EC); + if (EC) + return llvm::createStringError(EC, "Failed to open file for writing"); + return std::make_unique<llvm::CachedFileStream>(std::move(FDStream), + destPath.str()); + }, + Client); + + llvm::HTTPRequest Request(url); + Request.FollowRedirects = true; + + if (llvm::Error Err = Client.perform(Request, Handler)) + return Err; + if (llvm::Error Err = Handler.commit()) return Err; unsigned ResponseCode = Client.responseCode(); diff --git a/llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h b/llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h new file mode 100644 index 0000000000000..14ffab8aa1be3 --- /dev/null +++ b/llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h @@ -0,0 +1,49 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// An HTTPResponseHandler that streams the response body to a CachedFileStream. +/// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_SUPPORT_HTTP_STREAMEDHTTPRESPONSEHANDLER_H +#define LLVM_SUPPORT_HTTP_STREAMEDHTTPRESPONSEHANDLER_H + +#include "llvm/Support/Caching.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/HTTP/HTTPClient.h" +#include <functional> +#include <memory> + +namespace llvm { + +/// A handler which streams the returned data to a CachedFileStream. The cache +/// file is only created if a 200 OK status is observed. +class StreamedHTTPResponseHandler : public HTTPResponseHandler { + using CreateStreamFn = + std::function<Expected<std::unique_ptr<CachedFileStream>>()>; + CreateStreamFn CreateStream; + HTTPClient &Client; + std::unique_ptr<CachedFileStream> FileStream; + +public: + StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client) + : CreateStream(std::move(CreateStream)), Client(Client) {} + + /// Must be called exactly once after the writes have been completed + /// but before the StreamedHTTPResponseHandler object is destroyed. + Error commit(); + + virtual ~StreamedHTTPResponseHandler() = default; + + Error handleBodyChunk(StringRef BodyChunk) override; +}; + +} // end namespace llvm + +#endif // LLVM_SUPPORT_HTTP_STREAMEDHTTPRESPONSEHANDLER_H diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp index 6847dc092dfdb..77de51d90a4eb 100644 --- a/llvm/lib/Debuginfod/Debuginfod.cpp +++ b/llvm/lib/Debuginfod/Debuginfod.cpp @@ -35,6 +35,7 @@ #include "llvm/Support/Error.h" #include "llvm/Support/FileUtilities.h" #include "llvm/Support/HTTP/HTTPClient.h" +#include "llvm/Support/HTTP/StreamedHTTPResponseHandler.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/Path.h" #include "llvm/Support/ThreadPool.h" @@ -174,53 +175,6 @@ Expected<std::string> getCachedOrDownloadArtifact(StringRef UniqueKey, getDefaultDebuginfodTimeout()); } -namespace { - -/// A simple handler which streams the returned data to a cache file. The cache -/// file is only created if a 200 OK status is observed. -class StreamedHTTPResponseHandler : public HTTPResponseHandler { - using CreateStreamFn = - std::function<Expected<std::unique_ptr<CachedFileStream>>()>; - CreateStreamFn CreateStream; - HTTPClient &Client; - std::unique_ptr<CachedFileStream> FileStream; - -public: - StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client) - : CreateStream(CreateStream), Client(Client) {} - - /// Must be called exactly once after the writes have been completed - /// but before the StreamedHTTPResponseHandler object is destroyed. - Error commit(); - - virtual ~StreamedHTTPResponseHandler() = default; - - Error handleBodyChunk(StringRef BodyChunk) override; -}; - -} // namespace - -Error StreamedHTTPResponseHandler::handleBodyChunk(StringRef BodyChunk) { - if (!FileStream) { - unsigned Code = Client.responseCode(); - if (Code && Code != 200) - return Error::success(); - Expected<std::unique_ptr<CachedFileStream>> FileStreamOrError = - CreateStream(); - if (!FileStreamOrError) - return FileStreamOrError.takeError(); - FileStream = std::move(*FileStreamOrError); - } - *FileStream->OS << BodyChunk; - return Error::success(); -} - -Error StreamedHTTPResponseHandler::commit() { - if (FileStream) - return FileStream->commit(); - return Error::success(); -} - // An over-accepting simplification of the HTTP RFC 7230 spec. static bool isHeader(StringRef S) { StringRef Name; diff --git a/llvm/lib/Support/HTTP/CMakeLists.txt b/llvm/lib/Support/HTTP/CMakeLists.txt index e7a0e6fe34110..f8e4b87539931 100644 --- a/llvm/lib/Support/HTTP/CMakeLists.txt +++ b/llvm/lib/Support/HTTP/CMakeLists.txt @@ -16,6 +16,7 @@ endif() add_llvm_component_library(LLVMSupportHTTP HTTPClient.cpp HTTPServer.cpp + StreamedHTTPResponseHandler.cpp LINK_LIBS ${imported_libs} diff --git a/llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp b/llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp new file mode 100644 index 0000000000000..5a0e0e6355684 --- /dev/null +++ b/llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp @@ -0,0 +1,34 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "llvm/Support/HTTP/StreamedHTTPResponseHandler.h" + +namespace llvm { + +Error StreamedHTTPResponseHandler::handleBodyChunk(StringRef BodyChunk) { + if (!FileStream) { + unsigned Code = Client.responseCode(); + if (Code && Code != 200) + return Error::success(); + Expected<std::unique_ptr<CachedFileStream>> FileStreamOrError = + CreateStream(); + if (!FileStreamOrError) + return FileStreamOrError.takeError(); + FileStream = std::move(*FileStreamOrError); + } + *FileStream->OS << BodyChunk; + return Error::success(); +} + +Error StreamedHTTPResponseHandler::commit() { + if (FileStream) + return FileStream->commit(); + return Error::success(); +} + +} // namespace llvm From c3d5adb2572143516ec4824d037c3f2f1722f017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Gr=C3=A4nitz?= <[email protected]> Date: Fri, 20 Mar 2026 12:20:23 +0100 Subject: [PATCH 2/2] Handle file not found separately and print warnings for failures --- .../SymStore/SymbolLocatorSymStore.cpp | 93 ++++++++++--------- lldb/test/API/symstore/TestSymStore.py | 11 +++ 2 files changed, 59 insertions(+), 45 deletions(-) diff --git a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp index f1b931d894aa7..02c568816f706 100644 --- a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp +++ b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp @@ -117,47 +117,6 @@ std::string formatSymStoreKey(const UUID &uuid) { return llvm::toHex(bytes.slice(0, 16), LowerCase) + std::to_string(age); } -llvm::Error downloadFileHTTP(llvm::StringRef url, llvm::StringRef destPath) { - if (!llvm::HTTPClient::isAvailable()) - return llvm::createStringError( - std::make_error_code(std::errc::not_supported), - "HTTP client is not available"); - - llvm::HTTPClient Client; - - // TODO: Since PDBs can be huge, we should distinguish between resolve, - // connect, send and receive. - Client.setTimeout(std::chrono::seconds(60)); - - llvm::StreamedHTTPResponseHandler Handler( - [&]() -> llvm::Expected<std::unique_ptr<llvm::CachedFileStream>> { - std::error_code EC; - auto FDStream = std::make_unique<llvm::raw_fd_ostream>(destPath, EC); - if (EC) - return llvm::createStringError(EC, "Failed to open file for writing"); - return std::make_unique<llvm::CachedFileStream>(std::move(FDStream), - destPath.str()); - }, - Client); - - llvm::HTTPRequest Request(url); - Request.FollowRedirects = true; - - if (llvm::Error Err = Client.perform(Request, Handler)) - return Err; - if (llvm::Error Err = Handler.commit()) - return Err; - - unsigned ResponseCode = Client.responseCode(); - if (ResponseCode != 200) { - return llvm::createStringError(std::make_error_code(std::errc::io_error), - "HTTP request failed with status code " + - std::to_string(ResponseCode)); - } - - return llvm::Error::success(); -} - bool has_unsafe_characters(llvm::StringRef s) { for (unsigned char c : s) { // RFC 3986 unreserved characters are safe for file names and URLs. @@ -210,13 +169,57 @@ requestFileFromSymStoreServerHTTP(llvm::StringRef base_url, llvm::StringRef key, // Server has same directory structure with forward slashes as separators. std::string source_url = llvm::formatv("{0}/{1}/{2}/{1}", base_url, pdb_name, key); - if (llvm::Error err = downloadFileHTTP(source_url, cache_file.str())) { - LLDB_LOG_ERROR(log, std::move(err), - "Failed to download from SymStore '{1}': {0}", source_url); + + if (!llvm::HTTPClient::isAvailable()) { + Debugger::ReportWarning("HTTP client is not available"); + return {}; + } + + llvm::HTTPClient Client; + // TODO: Since PDBs can be huge, we should distinguish between resolve, + // connect, send and receive. + Client.setTimeout(std::chrono::seconds(60)); + + llvm::StreamedHTTPResponseHandler Handler( + [dest = cache_file.str().str()]() + -> llvm::Expected<std::unique_ptr<llvm::CachedFileStream>> { + std::error_code EC; + auto FDStream = std::make_unique<llvm::raw_fd_ostream>(dest, EC); + if (EC) + return llvm::createStringError(EC, "Failed to open file for writing"); + return std::make_unique<llvm::CachedFileStream>(std::move(FDStream), + dest); + }, + Client); + + llvm::HTTPRequest Request(source_url); + Request.FollowRedirects = true; + + if (llvm::Error Err = Client.perform(Request, Handler)) { + Debugger::ReportWarning( + llvm::formatv("Failed to download from SymStore '{0}': {1}", source_url, + llvm::toString(std::move(Err)))); + return {}; + } + if (llvm::Error Err = Handler.commit()) { + Debugger::ReportWarning( + llvm::formatv("Failed to download from SymStore '{0}': {1}", source_url, + llvm::toString(std::move(Err)))); return {}; } - return FileSpec(cache_file.str()); + unsigned ResponseCode = Client.responseCode(); + switch (ResponseCode) { + case 404: + return {}; // file not found + case 200: + return FileSpec(cache_file.str()); // success + default: + Debugger::ReportWarning(llvm::formatv( + "Failed to download from SymStore '{0}': response code {1}", source_url, + ResponseCode)); + return {}; + } } std::optional<FileSpec> findFileInLocalSymStore(llvm::StringRef root_dir, diff --git a/lldb/test/API/symstore/TestSymStore.py b/lldb/test/API/symstore/TestSymStore.py index 13d0cc1666c84..63f7f10a13de7 100644 --- a/lldb/test/API/symstore/TestSymStore.py +++ b/lldb/test/API/symstore/TestSymStore.py @@ -140,6 +140,17 @@ def test_local_dir(self): self.runCmd(f"settings set plugin.symbol-locator.symstore.urls {dir}") self.try_breakpoint(exe, should_have_loc=True) + def test_http_not_found(self): + """ + Check that a 404 response from an HTTP SymStore is handled gracefully. + """ + exe, sym = self.build_inferior() + with MockedSymStore(self, exe, sym) as symstore_dir: + os.makedirs(f"{symstore_dir}_empty", exist_ok=False) + with HTTPServer(f"{symstore_dir}_empty") as url: + self.runCmd(f"settings set plugin.symbol-locator.symstore.urls {url}") + self.try_breakpoint(exe, should_have_loc=False) + # TODO: Add test coverage for common HTTPS security scenarios, e.g. self-signed # certs, non-HTTPS redirects, etc. def test_http(self): _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
